Following is a small code-snippet to showcase how "states are preserved" / "contract is maintained":

import sktime.classification.deep_learning as dl
from sktime.base import load, load_dl
import numpy as np
from pprint import pprint
from pathlib import Path


if __name__ == "__main__":
    save_folder_location = "temp_saved"
    train_X = np.random.randn(15, 24, 16)
    train_y = np.random.randint(0, 2, size=(15, ))
    test_X = np.random.randn(5, 24, 16)

    cnn = dl.cnn.CNNClassifier(n_epochs=1)
    cnn.fit(train_X, train_y)

    cnn.save(save_folder_location)

    new_cnn = load_dl(save_folder_location)

    pprint(cnn.__dict__)
    pprint(cnn.model_.optimizer)
    pprint(cnn.optimizer)
    pprint(cnn.optimizer_)
    pprint(cnn.history)

    print("--" * 50)

    pprint(new_cnn.__dict__)
    pprint(new_cnn.model_.optimizer)
    pprint(new_cnn.optimizer)
    pprint(new_cnn.optimizer_)
    pprint(new_cnn.history)

    print("Does both give same result to same data?", np.allclose(
        cnn.predict(X=test_X),
        new_cnn.predict(X=test_X)
    ))

The output:

{'_X_metadata': {'has_nans': False,
                 'is_empty': False,
                 'is_equal_length': True,
                 'is_equally_spaced': True,
                 'is_one_series': False,
                 'is_univariate': False,
                 'mtype': 'numpy3D',
                 'n_instances': 15,
                 'scitype': 'Panel'},
 '_callbacks': [],
 '_class_dictionary': {0: 0, 1: 1},
 '_estimator_type': 'classifier',
 '_is_fitted': True,
 '_network': CNNNetwork(),
 '_tags_dynamic': {},
 '_threads_to_use': 1,
 'activation': 'sigmoid',
 'avg_pool_size': 3,
 'batch_size': 16,
 'callbacks': None,
 'classes_': array([0, 1]),
 'fit_time_': 3424,
 'history': <keras.callbacks.History object at 0x000001F77ECD9BB0>,
 'input_shape': (16, 24),
 'kernel_size': 7,
 'label_encoder': LabelEncoder(),
 'loss': 'mean_squared_error',
 'metrics': None,
 'model_': <keras.engine.functional.Functional object at 0x000001F778B355E0>,
 'n_classes_': 2,
 'n_conv_layers': 2,
 'n_epochs': 1,
 'onehot_encoder': OneHotEncoder(sparse=False),
 'optimizer': None,
 'optimizer_': <keras.optimizers.optimizer_v2.adam.Adam object at 0x000001F778B95760>,
 'random_state': None,
 'use_bias': True,
 'verbose': False}
<keras.optimizers.optimizer_v2.adam.Adam object at 0x000001F778B95760>
None
<keras.optimizers.optimizer_v2.adam.Adam object at 0x000001F778B95760>
<keras.callbacks.History object at 0x000001F77ECD9BB0>
----------------------------------------------------------------------------------------------------
{'_X_metadata': {'has_nans': False,
                 'is_empty': False,
                 'is_equal_length': True,
                 'is_equally_spaced': True,
                 'is_one_series': False,
                 'is_univariate': False,
                 'mtype': 'numpy3D',
                 'n_instances': 15,
                 'scitype': 'Panel'},
 '_callbacks': [],
 '_class_dictionary': {0: 0, 1: 1},
 '_estimator_type': 'classifier',
 '_is_fitted': True,
 '_network': CNNNetwork(),
 '_tags_dynamic': {},
 '_threads_to_use': 1,
 'activation': 'sigmoid',
 'avg_pool_size': 3,
 'batch_size': 16,
 'callbacks': None,
 'classes_': array([0, 1]),
 'fit_time_': 3424,
 'history': <keras.callbacks.History object at 0x000001F7022DC430>,
 'input_shape': (16, 24),
 'kernel_size': 7,
 'label_encoder': LabelEncoder(),
 'loss': 'mean_squared_error',
 'metrics': None,
 'model_': <keras.engine.functional.Functional object at 0x000001F70113F850>,
 'n_classes_': 2,
 'n_conv_layers': 2,
 'n_epochs': 1,
 'onehot_encoder': OneHotEncoder(sparse=False),
 'optimizer': None,
 'optimizer_': <keras.optimizers.optimizer_v2.adam.Adam object at 0x000001F70300C0A0>,
 'random_state': None,
 'use_bias': True,
 'verbose': False}
<keras.optimizers.optimizer_v2.adam.Adam object at 0x000001F70300C0A0>
None
<keras.optimizers.optimizer_v2.adam.Adam object at 0x000001F70300C0A0>
<keras.callbacks.History object at 0x000001F7022DC430>
1/1 [==============================] - 0s 253ms/step
1/1 [==============================] - 0s 174ms/step
Does both give same result to same data? True

@AurumnPegasus I would also like for you to code-review this PR and give some suggestion for possible tests.

Hey @achieveordie ,
Thanks for picking this up! I was not able to find a solution to adding keras history and loading it from Zip file (from Franz's PR #3336 ) and am glad you did.
In case they would be helpful, I had a design doc in work for the same as a STEP Document, though as mentioned above, there were some solutions I was not able to find.

I went through your PR and I think the code is good. I agree with the __getstate__ and __setstate__ point, and think that it is neccessary (generally required because you cannot pickle lambda functions, and keras.optimizers use them in there implementation, and hence there is no way around it.) I like the idea of having a seperate load_dl as well (since I think working with Zip files was proving troublesome for DL)

About the tests, there were a few I had in mind:

1. Idempotent test: Compare the predictions of the fitted model before and after saving the model. This will ensure that the model loaded is the one which was saved.
   This is an existing test which had been ignored for all DL Estimators because we were not getting exact results before and after saving (this was done before optimizer was added as a parameter, and hence we could work this out with pickle). The reason it was ignored was although the answers were very similar, they were not able to satisfy the threshold for similarity (which was around 1e-6) due to randomness of DL Estimators.
2. Saving with other parameters: Although I do not think any of the current parameters will give this issue, I still think we should test by sending some value of all parameters (and not taking None in any of them) so as to check they dont give the lambda issue optimizer gives us. It might be more useful for a future use case where someone adds a parameter which uses the lambda function.

Thanks for the review!

My only problem with a separate load_dl() was that we'd have to modify tests to call load_dl() instead of load() for relevant cases, not to mention it might cause a bit of confusion on the user's end.
The easiest solution that I can think of is if we modify the code to only have load() as an entry point which would either call _load_classical() or _load_dl() depending if path is a file or a directory, or simply add another parameter where the user passes in that information.
I think @fkiraly would be against the latter solution since it would require the user to have some pre-information about the model.

As for tests for the current implementation, I agree with your points. However, the idempotent test seems to be testing for fitting model twice as opposed to save/load. I might be wrong since I just glanced at it. For the second point, I was wondering if there is already an implementation that I could look at in the existing tests?

Edit: Alternatively, I think you might be referring to test_for_pickle_persistence() in the second point since that seems to fit your description. We'd need to change that, it's a work in progress in #3413.

The easiest solution that I can think of is if we modify the code to only have load() as an entry point which would either call _load_classical() or _load_dl() depending if path is a file or a directory

This looks quite easy to do, and should not require user to know anything about either the estimator or the saved file/directory. Maybe the python command os.path.isdir(path) would be of help? I am sure it works as required for unix-like systems, and unsure about windows since I have not used it on that.
This would allow an easy (although "hacky") solution to the problem

The easiest solution that I can think of is if we modify the code to only have load() as an entry point which would either call _load_classical() or _load_dl() depending if path is a file or a directory

This is the same idea that I was pursuing, I was just attatching these as overrides to the class they are relevant for.

This is following the strategy oop pattern, with one of the goals to localize the extension burden to the class. Otherwise, if you add new estimators with specific loading, you have two loci of extension - the load functions and the estimator class. But there should be only one (the estimator class).

So, yes, but _load_classical already is in the base class in my design, as methods load_from, etc, and _load_dl should simply override the load_from_ methods in the neural network base class.

load should be universal and not require extension or modification - if at current state it does, then it means we need to move more in the class specific loaders.

If we add new estimators that require a different logic than already provided in base classes(having custom objects for example), then the author would have to expand the class method load_from_path() / load_from_serial(). The same is true for your design. The private functions that I've added are simply modularized, done to reduce boilerplate code. These external functions call class methods similar to your original design.

Hm, can you explain why you added more code in load then?

My concern is that subject to your design, the same might be necessary if, say, we add a pytorch adapter.

Though I admit I have perhaps not fully understood how you see the extension contract, so I'm happy to be convinced that this is the last time something has to be added to load.

I've updated the STEP to clarify what I had in mind:
sktime/enhancement-proposals#27

Perhaps we don't understand each other. The additional code is due to the fact that non-DL and DL estimators are being stored in different ways. For the sake of discussion, if we were to save non-DL estimators in a directory (not zip them), then we can get rid of _load_classical() and _load_dl() functions since they'd have the same workflow.

Going from zip to directory-based for non-DL estimators seems easy, whereas going from directory to file-based storage (for DL estimators) needs more exploration. It might be easy/difficult/impossible. We need to discuss this further.

Perhaps we don't understand each other.

Yes, I suppose so.

The additional code is due to the fact that non-DL and DL estimators are being stored in different ways.

I'm not saying "there should not be additional/different code for DL estimators".

I'm saying "there will likely be different/additional code, and it should live in the base class of the estimator, not in the load function, because we want to go with the strategy object orientation pattern".

We need to discuss this further.

Yes, let's chat tomorrow when we are meeting anyway.

question: is there a way to avoid the creation of the temp folder?
This may upset virus scanners etc.

question: is there a way to avoid the creation of the temp folder? This may upset virus scanners etc.

Wouldn't this be a problem with keras's folder creation too? Or it's problematic because the code creates AND deletes it?

Or it's problematic because the code creates AND deletes it?

I was just thinking to minimize file write/delete actions, as heuristic detection may find more steps more odd, but that's just a gut feeling.

After taking care of the issue with optimizer, there are two kinds of failures:

• TapNet failures are due to the known issue [BUG] Unable to save TapNet estimators because of Lambda layers #3539
• Failures from the problematic test_fit_idempotent() as mentioned above

If you think that either of them is in the scope of this PR, LMK, otherwise one can skip these tests and work on it in another PR.

@fkiraly It seems that the only errors remaining are the AssertionError from test_fit_idempotent(). Could you look into them and tell me how you would like them to be handled? If it is beyond the scope we'd be better off creating an issue, if not then we'd have to look more into it.

Do leave some comments about the added tests for base deep classifiers, these tests were meant to check if __getstate__() and __setstate__() work properly in edge cases or not. I might add more tests in future.

Thanks! looks great!

Did you check (by deactivating --matrixdesign locally) whether TapNet and FCN are the only estimators affected?

@fkiraly It seems that the only errors remaining are the AssertionError from test_fit_idempotent().

These look like genuine bugs to me, i.e., random_state not working properly for the two estimators TapNet and FCN.
I would open two bug issues, skip the tests in _config, and reference the bug issues in the skip comment.

Do leave some comments about the added tests for base deep classifiers, these tests were meant to check if __getstate__() and __setstate__() work properly in edge cases or not. I might add more tests in future.

Tests look good to me!

Stupid question: now that we got set/getstate to work, and therefore pickle, do we really still need a custom save/load for deep learning estimators? I might be missing something obvious.

Did you check (by deactivating --matrixdesign locally) whether TapNet and FCN are the only estimators affected?

If you remember in one of the conversations I mentioned that my system fails to run the entire suite. It just hangs till all the worker crash. So I couldn't try this out. I think that all the deep estimators are affected (see CNNClassifier failing on this in the latest ubuntu runner).

deal somehow with the issue that test suite should not break

I don't know the feasibility of doing this here's a possible solution - We create a fixture which runs only when certain soft-deps are included in the built. Otherwise, the tests in this fixture are just skipped. For now, we might have to skip all those tests regardless.

do we really still need a custom save/load

Yes. We are deleting the most essential part of the object - the keras model - before pickling. So the custom save/load ties up the work that __getstate__() / __setstate__() is doing (by using pickle) along with saving the model/history/optimizer associated with that model.

So if I were to re-iterate, our revised remaining items will be:

• Skipping tests that require soft dependency till there's a solution for that.
• Skip all the deep estimators for test_fit_idempotent().
• Add ImportError in case soft dep h5py is missing during import

If we agree, then I can move forward and make these changes in the next commit.

Skipping tests that require soft dependency till there's a solution for that.

Not sure what you mean by this, can you explain?

Skip all the deep estimators for test_fit_idempotent().

I thought with getstate solved issus for most. Are we having AssertionError for all deep learning models?

That would mean that random_state is not working at all for these, no?

Add ImportError in case soft dep h5py is missing during import

Yes

Not sure what you mean by this, can you explain?

As you said that user's test suite may not have soft dependencies installed. So we can skip those tests. Specifically, any test that uses h5py (so in-memory serialization tests etc). I don't have a concrete idea if that's the best thing to do or not, but I'll leave that decision-making to you.

I thought with getstate solved issus for most. Are we having AssertionError for all deep learning models?

The updated getstate solves the Attribute Failure. AssertionFailure seems more of an issue with the test function / scenario rather than the estimators, but that's just my initial hypothesis. In any case, it is an entirely different issue and is not concerned with save/load code.

Not sure what you mean by this, can you explain?

As you said that user's test suite may not have soft dependencies installed. So we can skip those tests. Specifically, any test that uses h5py (so in-memory serialization tests etc). I don't have a concrete idea if that's the best thing to do or not, but I'll leave that decision-making to you.

I'd rather only skip the parts that require h5py (in-memory serialization), not the full test.
E.g., only (a) in-memory serialization, not file serialization, and (b) only deep learning estimators, not all estimators

The updated getstate solves the #3425 (comment). AssertionFailure seems more of an issue with the test function / scenario rather than the estimators, but that's just my initial hypothesis. In any case, it is an entirely different issue and is not concerned with save/load code.

I don't get what you mean, would appreciate an explanation.

@fkiraly I hope things are clear after our last conversation. LMK if something still needs to be cleared.

I know the PR is closed and merged, but I have some opinions about the implementation. Firstly, great contribution, thanks for this @achieveordie .

I was testing this in my local machine and found out that it doesn't work as expected in the composite models. For example, if we apply make_reduction to keras models, their appropriate save and load functions don't invoke, instead BaseObject.save and load function is called. This is also the case for pipelines that has forecaster in it. Since TransformedTargetForecaster doesn't override save load methods of BaseObject, keras model save/load calls can't be accessed.

I think we need some propagation mechanism for this to work. I mean, if a forecaster has an estimator inside, when save, load function called, it should also called for inside estimator load save. If requested, I can write a reproducible example to show the current issue. @fkiraly

@ilkersigirci thanks for the much-needed input. I think we were expecting to find some bugs since this has just been implemented. What I would suggest is that we should tackle this on a new issue. So maybe open a new issue (possibly with reproducible code) and let's take on it from there.

Thanks!

Ofc, I will open a bug issue. BTW, in the sktime.BaseObject instead of using pickle, if we use cloudpickle or dill, all of this issues will be solved. This is because, we won't need to modify __get__state__ and __set__state__. cloudpickle and dill can successfully serialize optimizer and lambda functions. Hence, I am in favor for this approach which changes pickle to cloudpickle as discussed in #3672