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

DOC persistence page revamp #28889

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

adrinjalali
Copy link
Member

This revamps our persistence documentation page to make it coherent and closer to what we actually expect users to do.

Two points:

  1. I've removed PMML since I don't see it being very relevant these days.
  2. I've included cloudpickle since I've seen it in cases where people need to persist arbitrary user defined functions.

cc @glemaitre @ArturoAmorQ

Copy link

github-actions bot commented Apr 25, 2024

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: 9848f2e. Link to the linter CI: here

@adrinjalali adrinjalali added this to the 1.5 milestone Apr 25, 2024
@adrinjalali
Copy link
Member Author

Putting this on the milestone since it'd be very nice to have the revamped page on the release.

Copy link
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

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

I like this new documentation revamping. Here a couple of comments.

doc/model_persistence.rst Outdated Show resolved Hide resolved
doc/model_persistence.rst Outdated Show resolved Hide resolved
doc/model_persistence.rst Outdated Show resolved Hide resolved
doc/model_persistence.rst Outdated Show resolved Hide resolved
doc/model_persistence.rst Outdated Show resolved Hide resolved
doc/model_persistence.rst Outdated Show resolved Hide resolved
doc/model_persistence.rst Show resolved Hide resolved
doc/model_persistence.rst Outdated Show resolved Hide resolved

|details-start|
**Using ``pickle``, ``joblib``, or ``cloudpickle``**
Copy link
Member

Choose a reason for hiding this comment

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

The rendering will be better.

Suggested change
**Using ``pickle``, ``joblib``, or ``cloudpickle``**
**Using pickle, joblib, or cloudpickle**

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, but then they have to be un-bolded

Copy link
Member

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

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

I didn't review it all yet but here is a bit of feedback before I disconnect and the forget to complete this PR.

doc/model_persistence.rst Outdated Show resolved Hide resolved
doc/model_persistence.rst Outdated Show resolved Hide resolved
doc/model_persistence.rst Outdated Show resolved Hide resolved
doc/model_persistence.rst Show resolved Hide resolved
doc/model_persistence.rst Outdated Show resolved Hide resolved
Copy link
Member

@lorentzenchr lorentzenchr left a comment

Choose a reason for hiding this comment

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

AFAIK, cloudpickle is used by mlflow which is used quite often, in particular with databricks.

doc/model_persistence.rst Outdated Show resolved Hide resolved
doc/model_persistence.rst Outdated Show resolved Hide resolved
doc/model_persistence.rst Show resolved Hide resolved
:mod:`skops.io` avoids using :mod:`pickle` and only loads files which have types
and references to functions which are trusted either by default or by the user.
Therefore it provides a more secure format than :mod:`pickle`, :mod:`joblib`,
Copy link
Member

Choose a reason for hiding this comment

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

Is it a human-readable format?

Copy link
Member Author

Choose a reason for hiding this comment

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

Inside the zipfile, there's a human readable schema, but one needs to know a bunch about the format to look at it.

doc/model_persistence.rst Outdated Show resolved Hide resolved
doc/model_persistence.rst Outdated Show resolved Hide resolved
doc/model_persistence.rst Outdated Show resolved Hide resolved
doc/model_persistence.rst Outdated Show resolved Hide resolved
doc/model_persistence.rst Outdated Show resolved Hide resolved
doc/model_persistence.rst Outdated Show resolved Hide resolved
@adrinjalali
Copy link
Member Author

@ogrisel is this good to merge?

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

Successfully merging this pull request may close these issues.

None yet

4 participants