-
-
Notifications
You must be signed in to change notification settings - Fork 25.9k
DOC persistence page revamp #28889
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
Conversation
Putting this on the milestone since it'd be very nice to have the revamped page on the release. |
There was a problem hiding this 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
|
||
|details-start| | ||
**Using ``pickle``, ``joblib``, or ``cloudpickle``** |
There was a problem hiding this comment.
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.
**Using ``pickle``, ``joblib``, or ``cloudpickle``** | |
**Using pickle, joblib, or cloudpickle** |
There was a problem hiding this comment.
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
There was a problem hiding this 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.
There was a problem hiding this 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.
: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`, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
@ogrisel is this good to merge? |
I'm removing the milestone because it's just doc and can be backported in the appropriate branch anytime. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. @ogrisel could you have a last look regarding the cloudpickle
section.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise LGTM :)
Co-authored-by: Arturo Amor <86408019+ArturoAmorQ@users.noreply.github.com>
This revamps our persistence documentation page to make it coherent and closer to what we actually expect users to do.
Two points:
PMML
since I don't see it being very relevant these days.cloudpickle
since I've seen it in cases where people need to persist arbitrary user defined functions.cc @glemaitre @ArturoAmorQ