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] Introduce Serialization Support #154

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

achieveordie
Copy link
Contributor

Part of #145 - Introduces serialization functionality.

Currently, this is a direct port from the functionality we have in sktime. I'll iteratively improve the code as per requirements and inputs.

What does this implement/fix? Explain your changes.

  • Port serialization methods (load as a standalone function, save as BaseObject method) from sktime
  • Add tests to ensure the functionalities added work properly and are consistent with the expected behaviour.
  • Improve/add code to ensure this functionality can be generally applied and extended.

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

None

What should a reviewer concentrate their feedback on?

  • since bandit complains about the usage of pickle. I've excluded two tests (import and load) and will add a warning in load's docstring regarding it. We should try to find safer alternatives to replace it in the future.
  • What improvements should we bring to the current code? Some nice-to-have functionalities etc

Any other comments?

PR checklist

For all contributions
  • I've reviewed the project documentation on contributing
  • I've added myself to the list of contributors.
  • The PR title starts with either [ENH], [CI/CD], [MNT], [DOC], or [BUG] indicating whether
    the PR topic is related to enhancement, CI/CD, maintenance, documentation, or a bug.
For code contributions
  • Unit tests have been added covering code functionality
  • Appropriate docstrings have been added (see documentation standards)
  • New public functionality has been added to the API Reference

path = Path(path) if isinstance(path, str) else path
path.mkdir()

pickle.dump(type(self), open(path / "_metadata", "wb"))

Check warning

Code scanning / CodeQL

File is not always closed

File is opened but is not closed.
path.mkdir()

pickle.dump(type(self), open(path / "_metadata", "wb"))
pickle.dump(self, open(path / "_obj", "wb"))

Check warning

Code scanning / CodeQL

File is not always closed

File is opened but is not closed.
Copy link
Contributor

@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.

Looks great!

Before I go into detail, I wanted to say that the tests are failing because skbase uses itself as a test case for the crawling utilities, and requires you to enter new files, modules, classes, etc, in tests.conftest (the lists).

Personally, I think this is very tedious for a contributor and wasn't a great design decision, so mid-term we need to get away from that. For now, I'd recommend you just add to the lists.

@codecov-commenter
Copy link

codecov-commenter commented Apr 6, 2023

Codecov Report

Merging #154 (aea9904) into main (18435de) will decrease coverage by 1.37%.
The diff coverage is 16.32%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##             main     #154      +/-   ##
==========================================
- Coverage   82.68%   81.31%   -1.37%     
==========================================
  Files          32       33       +1     
  Lines        2327     2376      +49     
==========================================
+ Hits         1924     1932       +8     
- Misses        403      444      +41     
Impacted Files Coverage Δ
skbase/base/_serialize.py 15.78% <15.78%> (ø)
skbase/base/_base.py 75.66% <16.66%> (-6.56%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@fkiraly
Copy link
Contributor

fkiraly commented Apr 9, 2023

let me know when this is ready

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants