Skip to content

[BUG] Possible solution for pickle persistency fail for DL models #3413

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 2 commits into from
Closed

[BUG] Possible solution for pickle persistency fail for DL models #3413

wants to merge 2 commits into from

Conversation

achieveordie
Copy link
Collaborator

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Initially, test_persistence_via_pickle() of test_all_estimators.py was skipped for all DL models since there were problems correctly serializing the objects. So, added possibly corrected __getstate__() function for both BaseDeepRegressor() and BaseDeepClassifier().

The current implementation seems to be working locally for CNNClassifier

Does your contribution introduce a new dependency? If yes, which one?

No

What should a reviewer concentrate their feedback on?

Any other comments?

PR checklist

For all contributions
  • I've added myself to the list of contributors.
  • The PR title starts with either [ENH], [MNT], [DOC], or [BUG] indicating whether the PR topic is related to enhancement, maintenance, documentation, or bug.

@achieveordie
Copy link
Collaborator Author

The failures are due to the known issue of not having the optimizer stored. Will fix it in the upcoming days.

@fkiraly
Copy link
Collaborator

fkiraly commented Sep 13, 2022

yes, this is part of the interface contract.

Note, we do not need to rely on pickle, we could rely on a save/load interface instead that replaces pickle with sth else.

@achieveordie
Copy link
Collaborator Author

This PR can only be developed further after #3425 gets accepted since that deals with .save() / .load() class methods that will be used instead of pickle's .dump() / .load() for test_persistence_via_pickle().

@achieveordie achieveordie mentioned this pull request Sep 16, 2022
Merged
1 task
@fkiraly
Copy link
Collaborator

fkiraly commented Sep 16, 2022

hm, what I would do is to leave the save/load tests skipped, and instead write a test for pickling?
So we test the functionality added by this PR with a test also added in this PR.
Not as part of the generic test suite, but for one example of neural networks.

What do you think?

@achieveordie
Copy link
Collaborator Author

The functionality in this PR is an outdated version of #3425. We still need to improve/correct/add test functionality which is the aim of this PR.
So after that PR is merged, we can continue updating the tests here. I could also copy-paste the relevant functions from that PR to this one but I think the former way is better.

@achieveordie
Copy link
Collaborator Author

Closing in favour of #3425. This feature will be implemented there.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benHeid benHeid Awaiting requested review from benHeid benHeid will be requested when the pull request is marked ready for review benHeid is a code owner

@felipeangelimvieira felipeangelimvieira Awaiting requested review from felipeangelimvieira felipeangelimvieira will be requested when the pull request is marked ready for review felipeangelimvieira is a code owner

@fkiraly fkiraly Awaiting requested review from fkiraly fkiraly will be requested when the pull request is marked ready for review fkiraly is a code owner

@fnhirwa fnhirwa Awaiting requested review from fnhirwa fnhirwa will be requested when the pull request is marked ready for review fnhirwa is a code owner

@geetu040 geetu040 Awaiting requested review from geetu040 geetu040 will be requested when the pull request is marked ready for review geetu040 is a code owner

@yarnabrina yarnabrina Awaiting requested review from yarnabrina yarnabrina will be requested when the pull request is marked ready for review yarnabrina is a code owner

Assignees

@achieveordie achieveordie

Labels
module:classification classification module: time series classification module:regression regression module: time series regression
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@achieveordie @fkiraly @TonyBagnall