Skip to content
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.

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

[ENH] estimator serialization: user choice of serialization_format, support for cloudpickle #5486

Merged
merged 12 commits into from Nov 3, 2023

Conversation

achieveordie
Copy link
Collaborator

@achieveordie achieveordie commented Oct 24, 2023

Reference Issues/PRs

In reference to #5324

What does this implement/fix? Explain your changes.

  • Add support for serialization_format (currently pickle and cloudpickle are supported) for all estimators.
  • Update sktime_mlflow to also be able to save DL estimators.

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

None

What should a reviewer concentrate their feedback on?

Did you add any tests for the change?

Yes, I've added tests for all the added functionalities, I'm open to suggestions for adding more tests.

Any other comments?

PR checklist

For all contributions
  • I've added myself to the list of contributors with any new badges I've earned :-)
    How to: add yourself to the all-contributors file in the sktime root directory (not the CONTRIBUTORS.md). Common badges: code - fixing a bug, or adding code logic. doc - writing or improving documentation or docstrings. bug - reporting or diagnosing a bug (get this plus code if you also fixed the bug in the PR).maintenance - CI, test framework, release.
    See here for full badge reference
  • Optionally, I've added myself and possibly others to the CODEOWNERS file - do this if you want to become the owner or maintainer of an estimator you added.
    See here for further details on the algorithm maintainer role.
  • The PR title starts with either [ENH], [MNT], [DOC], or [BUG]. [BUG] - bugfix, [MNT] - CI, test framework, [ENH] - adding or improving code, [DOC] - writing or improving documentation or docstrings.
For new estimators
  • I've added the estimator to the API reference - in docs/source/api_reference/taskname.rst, follow the pattern.
  • I've added one or more illustrative usage examples to the docstring, in a pydocstyle compliant Examples section.
  • If the estimator relies on a soft dependency, I've set the python_dependencies tag and ensured
    dependency isolation, see the estimator dependencies guide.

@achieveordie
Copy link
Collaborator Author

The errors seem unrelated, rerunning it to confirm.

@achieveordie achieveordie marked this pull request as ready for review November 1, 2023 14:15
@achieveordie
Copy link
Collaborator Author

The current CI failure is due to a known issue #5026.

sktime/base/_base.py Outdated Show resolved Hide resolved
@@ -6,6 +6,12 @@
from sktime.tests.test_switch import run_test_for_class


# A reference to this issue is also present inside sktime/tests/_config.py,
# and needs to be removed from `EXCLUDED_TESTS` upon resolution.
@pytest.mark.skip(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is sporadic, we should not skip it

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the general consensus on dealing with such sporadic errors?

Copy link
Collaborator

@fkiraly fkiraly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

Small blocking things:

  • I would spell the available formats out in the docstring directly. There are just two options.
  • I would split this PR up in two, one with the serialization format, one for mlflow extension.

Non-blocking, out of scope for this PR but perhaps worth a look: @benjaminbluhm has continued to develop the mlflow plugin as part of mlflavours. I am not sure of its status, but perhaps you want a quick look whether adding this in one of the two or both places makes sense, whether we want to backport sth, or outsource sth to mlflavours.

@achieveordie
Copy link
Collaborator Author

mlflavors seems to have been updated quite a while ago. Ideally, this change should be present in both places since if one uses mlflavors with sktime DL estimators, they will get the same error as #5324.

If @benjaminbluhm doesn't mind, I'll open a similar PR there as well.

@achieveordie achieveordie changed the title [ENH][BUG] Add support for serialization_format and add support for DL estimators with mlflow [ENH] Add support for serialization_format Nov 2, 2023
@achieveordie
Copy link
Collaborator Author

I'll make a follow-up PR for mlflow changes after this has been merged.

@achieveordie
Copy link
Collaborator Author

Two errors are related to #5488 and one error is from a tacky HTTPS connection, all unrelated to this PR.

fkiraly
fkiraly previously approved these changes Nov 3, 2023
Copy link
Collaborator

@fkiraly fkiraly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@fkiraly fkiraly changed the title [ENH] Add support for serialization_format [ENH] estimator serialization: user choice of serialization_format, support for cloudpickle Nov 3, 2023
@fkiraly fkiraly added enhancement Adding new functionality module:base-framework BaseObject, registry, base framework labels Nov 3, 2023
fkiraly
fkiraly previously approved these changes Nov 3, 2023
Copy link
Collaborator

@fkiraly fkiraly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good to me now!

@fkiraly fkiraly merged commit 9aac291 into sktime:main Nov 3, 2023
63 checks passed
fkiraly pushed a commit that referenced this pull request Nov 4, 2023
Fixes #5324 - should be merged after #5486 which this PR is built on.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Adding new functionality module:base-framework BaseObject, registry, base framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants