Added new file format param to skops snippet script by merveenoyan · Pull Request #417 · huggingface/hub-docs

merveenoyan · 2022-10-17T12:53:00Z

As discussed with @rajshah4 in Slack, it's better to state explicitly to load a model using pickle if it's a pickle file. Also cc @adrinjalali

osanseviero · 2022-12-06T14:14:53Z

The current code snippet does not work I think. E.g. go to https://huggingface.co/Ramos-Ramos/emb-gam-dino-resnet

The snippet is

from skops.hub_utils import download
from skops.io import load

download("Ramos-Ramos/emb-gam-dino-resnet", "path_to_folder")
# make sure model file is in skops format
# if model is a pickle file, make sure it's from a source you trust
model = load("path_to_folder/model.pkl")

If you try the above, it will give an error

 TypeError: download() takes 2 positional arguments but 1 was given

The expectation is actually to use named arguments

download(repo_id="Ramos-Ramos/emb-gam-dino-resnet", dst="emb-gam-dino-resnet")

But then

model = load("path_to_folder/model.pkl")

Gives me an error

BadZipFile: File is not a zip file

What did work for me was

import pickle

with open("emb-gam-dino-resnet/model.pkl", "rb") as file: 
  logistic_regression = pickle.load(file)

Could we update the auto-snippet for it to work end to end?

merveenoyan · 2022-12-08T10:55:18Z

@osanseviero I'm working on this PR: skops-dev/skops#242 (my work is done, waiting for review)
Once it's merged, we can make this work end to end.
Since we made a decision to add a key to config instead, I'll edit the snippet such that this info is taken from config.

osanseviero · 2022-12-08T11:25:28Z

+		const skopssaveFormat = model.config?.sklearn?.save_format;
+		if (skopssaveFormat == "pickle")
+		{
+			return `download("${model.id}", "path_to_folder")


Please add the imports at the top, and also with from skops.hub_utils import download which is not in this one

merveenoyan · 2022-12-08T11:29:30Z

@osanseviero please don't review until I can test it 😅 (had a problem in codespaces)

merveenoyan · 2022-12-09T18:12:18Z

model uploaded using skops with pickle

2. model uploaded using skops with skops as file format

3. vanilla sklearn model

merveenoyan · 2022-12-09T18:13:36Z

The work on this is done: skops-dev/skops#242 (just made a mistake on changelog, fixed it) we can merge this one after that one.

osanseviero

The results look nice! I think we might want to:

Keep showing security message for joblib as we were doing before
Looking at the file (

hub-docs/js/src/lib/interfaces/Libraries.ts

Line 236 in 02dc01d

const skopssaveFormat = model.config?.sklearn?.model_format;

), there are some minor indentation/formatting issues in the sklearn function. Please try to be consistent (e.g. opening curly braces in same line - 237/238; double indent in lines 247 and 254)

osanseviero · 2022-12-10T20:09:17Z

 model = load("path_to_folder/${skopsmodelFile}")`;
-	} else {
+	}
+ } else {


Should we bring the security warning back?

brought it back

osanseviero · 2022-12-10T20:09:39Z

 model = load("path_to_folder/${skopsmodelFile}")`;
-	} else {
+	}
+ } else {


Please unindent the closing ) in the vanilla sklearn code snippet

adrinjalali · 2022-12-12T12:40:48Z

This looks good to me, but I'm more comfortable if @osanseviero has a final look before merging.

merveenoyan · 2022-12-12T17:39:32Z

@adrinjalali we need to merge the one on skops and moon-landing first that's why I didn't merge it yet.

@osanseviero
I feel like I'm going crazy so I'll stop working today 😂 (for pickle model)

BUT

osanseviero · 2022-12-12T17:48:28Z

In your code snippet you do

const skopssaveFormat = model.config?.sklearn?.model_format;

But there is no model_format, there is fileformat. Hence, you don't enter the condition of line 237 (format == pickle), and you end up being in the else, which is what I see in the screenshot. Maybe this is a good signal of not using if/else here and rather if/else if/else, and in the else show some other snippet with a comment saying it's not a supported format.

merveenoyan · 2022-12-12T17:54:58Z

@osanseviero screenshot has the one I'm using for build and it doesn't say 'model_format', it does 'fileformat', hence it gets into the if with 'pickle' (see what's logged) (the code in the HEAD is not updated, don't check that)

merveenoyan · 2022-12-13T12:52:36Z

I guess I was using a wrong version on moon-landing and everything was working fine. @osanseviero 😂

We can merge after this one is merged: https://github.com/skops-dev/skops/pull/242

Co-authored-by: Omar Sanseviero <osanseviero@gmail.com>

merveenoyan · 2022-12-16T14:09:57Z

@osanseviero updated variable name in accordance to moon-landing

merveenoyan · 2023-01-04T08:50:38Z

@osanseviero can we merge this since one in moon-landing is merged?

osanseviero

Feel free to merge :)

added extra note

e0a1222

merveenoyan requested a review from osanseviero October 17, 2022 12:53

adrinjalali reviewed Oct 17, 2022

View reviewed changes

Comment thread js/src/lib/interfaces/Libraries.ts Outdated

merveenoyan added 2 commits December 8, 2022 14:09

change snippet

bd90383

Merge branch 'main' into fix_snippet

89f5881

osanseviero reviewed Dec 8, 2022

View reviewed changes

merveenoyan added 2 commits December 9, 2022 20:52

addressed comments

56f0d2b

renamed variable according to my PR at skops

02dc01d

merveenoyan requested review from adrinjalali and osanseviero December 9, 2022 18:12

osanseviero approved these changes Dec 10, 2022

View reviewed changes

adrinjalali approved these changes Dec 12, 2022

View reviewed changes

Comment thread js/src/lib/interfaces/Libraries.ts

merveenoyan added 3 commits December 12, 2022 14:39

unindent

1a6136d

added note on skops persistence docs

2e0e765

warnings

4436cff

merveenoyan changed the title ~~Added extra note to snippet~~ Added new file format param to skops snippet script Dec 12, 2022

added param from models.ts

5ec7c72

osanseviero reviewed Dec 13, 2022

View reviewed changes

Comment thread js/src/lib/interfaces/Libraries.ts Outdated

merveenoyan and others added 2 commits December 14, 2022 11:54

Update js/src/lib/interfaces/Libraries.ts

f84874f

Co-authored-by: Omar Sanseviero <osanseviero@gmail.com>

changed var name

89781f6

fix

e3c1a8c

merveenoyan requested a review from osanseviero January 4, 2023 08:50

osanseviero approved these changes Jan 4, 2023

View reviewed changes

merveenoyan merged commit 2e4d048 into main Jan 5, 2023

merveenoyan deleted the fix_snippet branch January 5, 2023 06:07

Conversation

merveenoyan commented Oct 17, 2022

Uh oh!

Uh oh!

osanseviero commented Dec 6, 2022

Uh oh!

merveenoyan commented Dec 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

osanseviero Dec 8, 2022

Choose a reason for hiding this comment

Uh oh!

merveenoyan commented Dec 8, 2022

Uh oh!

merveenoyan commented Dec 9, 2022

Uh oh!

merveenoyan commented Dec 9, 2022

Uh oh!

osanseviero left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

osanseviero Dec 10, 2022

Choose a reason for hiding this comment

Uh oh!

merveenoyan Dec 12, 2022

Choose a reason for hiding this comment

Uh oh!

osanseviero Dec 10, 2022

Choose a reason for hiding this comment

Uh oh!

merveenoyan Dec 12, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adrinjalali commented Dec 12, 2022

Uh oh!

merveenoyan commented Dec 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

osanseviero commented Dec 12, 2022

Uh oh!

merveenoyan commented Dec 12, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

merveenoyan commented Dec 13, 2022

Uh oh!

Uh oh!

merveenoyan commented Dec 16, 2022

Uh oh!

merveenoyan commented Jan 4, 2023

Uh oh!

osanseviero left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

merveenoyan commented Dec 8, 2022 •

edited

Loading

merveenoyan commented Dec 12, 2022 •

edited

Loading

merveenoyan commented Dec 12, 2022 •

edited

Loading