Added `save_format` to `config.json` and `hub_utils.init()` #242

merveenoyan · 2022-12-05T13:40:10Z

Big chunk of work is to refactor tests. TIL you could add parametrization to fixtures

This is a draft PR, feel free to skim through it but don't look for nits

adrinjalali

looks good. you should also send a commit including the text [CI inference] in the commit message to run the inference tests.

skops/hub_utils/_hf_hub.py

BenjaminBossan

Not much to add from my side on top of what Adrin wrote.

skops/hub_utils/tests/test_hf_hub.py

skops/hub_utils/_hf_hub.py

Co-authored-by: Adrin Jalali <[email protected]>

adrinjalali

Thanks @merveenoyan

We also need an entry in the changelog

adrinjalali · 2022-12-09T12:18:54Z

skops/hub_utils/tests/test_hf_hub.py

+        # a model card is needed for inference engine to work.
+        model_card = card.Card(
+            model, metadata=card.metadata_from_config(Path(destination_path))
+        )
+        model_card.save(Path(destination_path) / "README.md")


add a comment to remove when #207 is merged.

adrinjalali · 2022-12-09T12:20:02Z

skops/hub_utils/tests/test_hf_hub.py

+    if file_format != "pickle":
+        return


I think instead of silently skipping the inference test, we should parameterize and only test for pickle, and when inference API starts working with .skops, we add it here.

merveenoyan · 2022-12-09T14:50:35Z

there was a problem with fixture temp_path with python 3.7 so I swapped it with mkdtemp instead.

skops/hub_utils/tests/test_hf_hub.py

adrinjalali · 2022-12-09T14:54:09Z

docs/changes.rst

+- Add `model_format` argument to :meth:`skops.hub_utils.init` to write it
+  as a section to `config.json`. :pr:`242`by `Merve Noyan`_.


this should go to v0.4 section.

Co-authored-by: Adrin Jalali <[email protected]>

merveenoyan · 2022-12-09T17:50:19Z

@adrinjalali I don't know I could swear adding it to v0.4 🥲🥲🥲 addressed both

merveenoyan · 2022-12-12T09:26:06Z

@adrinjalali ran black, apparently previous pre-commit didn't catch something (I looked at my CLI and it was all passed 🥲)

adrinjalali

Otherwise LGTM

docs/changes.rst

Co-authored-by: Adrin Jalali <[email protected]>

adrinjalali · 2022-12-12T12:39:15Z

I'll let @BenjaminBossan have another look and merge.

BenjaminBossan

Implementation-wise, this looks good to me.

I have a small issue with the wording, please take a look if my concern is valid. Apart from that, I suggested a empty lines in the docstring for consistency.

BenjaminBossan · 2022-12-12T14:55:23Z

docs/changes.rst

+- Add `model_format` argument to :meth:`skops.hub_utils.init` to be stored in
+  `config.json` so that we know how to load a model from the repository.
+  :pr:`242` by `Merve Noyan`_.


If I didn't already know, I would be confused about what this new feature does. Maybe we should avoid the word "we" in the changes.rst and instead use more neutral language.

what do you suggest here @BenjaminBossan

It's not a blocker, so feel free to merge.

Maybe something like:

Add model_format argument to :meth:skops.hub_utils.init to be stored in
config.json, which is used to determine the format of the model file
(pickle or skops).

BenjaminBossan · 2022-12-12T15:00:54Z

skops/hub_utils/_hf_hub.py

@@ -291,6 +318,9 @@ def init(
        :class:`numpy.ndarray`. If ``task`` is ``"text-classification"`` or
        ``"text-regression"``, the data needs to be a ``list`` of strings.

+    model_format: str
+        The format used to persist the model. Can be ``"auto"``, ``"skops"``


Maybe it's just me, but when I read this docstring, it could also mean "the model format that is being used [by this function] to persist the model", when what it really means is "the model format that was used to persist the model [by the user]". The former interpretation could lead the user to believe that init will create a new model file using that format. E.g. when I have a pickle file and I use model_format="skops", then init will create a skops file from the pickle file for me.

Something like "The format used by the persisted model" could help to disambiguate the two meanings. WDYT?

The same ambiguity can be found in the docstring of _create_config but there it's less important, as it's internal.

skops/hub_utils/_hf_hub.py

merveenoyan · 2022-12-12T15:09:17Z

@adrinjalali I accidentally asked for review 🥲

Co-authored-by: Benjamin Bossan <[email protected]>

BenjaminBossan

LGTM, thanks. Last point is no blocker for merging.

BenjaminBossan · 2022-12-13T13:00:13Z

docs/changes.rst

+- Add `model_format` argument to :meth:`skops.hub_utils.init` to be stored in
+  `config.json` so that we know how to load a model from the repository.
+  :pr:`242` by `Merve Noyan`_.


It's not a blocker, so feel free to merge.

Maybe something like:

Add model_format argument to :meth:skops.hub_utils.init to be stored in
config.json, which is used to determine the format of the model file
(pickle or skops).

added format in config

70993e0

adrinjalali mentioned this pull request Dec 6, 2022

Generate minimal README.md file in repository initialization (issue #113) #207

Closed

added tests

36223e2

merveenoyan requested a review from adrinjalali December 7, 2022 13:43

merveenoyan marked this pull request as ready for review December 7, 2022 13:43

merveenoyan requested a review from BenjaminBossan December 8, 2022 10:54

merveenoyan mentioned this pull request Dec 8, 2022

Added new file format param to skops snippet script huggingface/hub-docs#417

Merged

adrinjalali reviewed Dec 8, 2022

View reviewed changes

skops/hub_utils/_hf_hub.py Outdated Show resolved Hide resolved

skops/hub_utils/_hf_hub.py Outdated Show resolved Hide resolved

skops/hub_utils/_hf_hub.py Outdated Show resolved Hide resolved

BenjaminBossan reviewed Dec 8, 2022

View reviewed changes

skops/hub_utils/tests/test_hf_hub.py Outdated Show resolved Hide resolved

skops/hub_utils/_hf_hub.py Outdated Show resolved Hide resolved

merveenoyan and others added 4 commits December 9, 2022 13:10

Update skops/hub_utils/_hf_hub.py

ece88dd

Co-authored-by: Adrin Jalali <[email protected]>

Update skops/hub_utils/_hf_hub.py

ea6c5ae

Co-authored-by: Adrin Jalali <[email protected]>

Update skops/hub_utils/_hf_hub.py

a6ffc3d

Co-authored-by: Adrin Jalali <[email protected]>

addressed comments

59c45a4

merveenoyan requested review from adrinjalali and BenjaminBossan December 9, 2022 10:59

adrinjalali reviewed Dec 9, 2022

View reviewed changes

merveenoyan added 2 commits December 9, 2022 17:39

[CI Inference]

1c2eaf1

swapped fixture with mkdtemp

9eb24bd

merveenoyan requested a review from adrinjalali December 9, 2022 14:51

adrinjalali reviewed Dec 9, 2022

View reviewed changes

merveenoyan and others added 2 commits December 9, 2022 20:33

Update skops/hub_utils/tests/test_hf_hub.py

d3e11e6

Co-authored-by: Adrin Jalali <[email protected]>

changed changelog

589de5c

merveenoyan requested a review from adrinjalali December 9, 2022 17:49

merveenoyan added 2 commits December 12, 2022 12:12

black

d4c1b89

black tests

2ea068f

adrinjalali approved these changes Dec 12, 2022

View reviewed changes

docs/changes.rst Outdated Show resolved Hide resolved

Update docs/changes.rst

ed70096

Co-authored-by: Adrin Jalali <[email protected]>

Merge branch 'main' into format_in_config

2937c73

BenjaminBossan reviewed Dec 12, 2022

View reviewed changes

merveenoyan requested a review from adrinjalali December 12, 2022 15:08

merveenoyan and others added 3 commits December 12, 2022 18:09

Update skops/hub_utils/_hf_hub.py

23a6c07

Co-authored-by: Benjamin Bossan <[email protected]>

Update skops/hub_utils/_hf_hub.py

9dda4e6

Co-authored-by: Benjamin Bossan <[email protected]>

Update skops/hub_utils/_hf_hub.py

a0a5b45

Co-authored-by: Benjamin Bossan <[email protected]>

merveenoyan requested a review from BenjaminBossan December 13, 2022 12:07

changed wording

67a6de2

BenjaminBossan approved these changes Dec 13, 2022

View reviewed changes

BenjaminBossan merged commit 7ebf893 into skops-dev:main Dec 13, 2022

BenjaminBossan mentioned this pull request Dec 14, 2022

Add serialization format to config.json #228

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added `save_format` to `config.json` and `hub_utils.init()` #242

Added `save_format` to `config.json` and `hub_utils.init()` #242

merveenoyan commented Dec 5, 2022

adrinjalali left a comment

BenjaminBossan left a comment

adrinjalali left a comment

adrinjalali Dec 9, 2022

adrinjalali Dec 9, 2022

merveenoyan commented Dec 9, 2022

adrinjalali Dec 9, 2022

merveenoyan commented Dec 9, 2022

merveenoyan commented Dec 12, 2022

adrinjalali left a comment

adrinjalali commented Dec 12, 2022

BenjaminBossan left a comment

BenjaminBossan Dec 12, 2022

adrinjalali Dec 13, 2022

BenjaminBossan Dec 13, 2022

BenjaminBossan Dec 12, 2022

merveenoyan commented Dec 12, 2022

BenjaminBossan left a comment

BenjaminBossan Dec 13, 2022

		- Add `model_format` argument to :meth:`skops.hub_utils.init` to write it
		as a section to `config.json`. :pr:`242`by `Merve Noyan`_.

Added save_format to config.json and hub_utils.init() #242

Added save_format to config.json and hub_utils.init() #242

Conversation

merveenoyan commented Dec 5, 2022

adrinjalali left a comment

Choose a reason for hiding this comment

BenjaminBossan left a comment

Choose a reason for hiding this comment

adrinjalali left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

merveenoyan commented Dec 9, 2022

Choose a reason for hiding this comment

merveenoyan commented Dec 9, 2022

merveenoyan commented Dec 12, 2022

adrinjalali left a comment

Choose a reason for hiding this comment

adrinjalali commented Dec 12, 2022

BenjaminBossan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

merveenoyan commented Dec 12, 2022

BenjaminBossan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Added `save_format` to `config.json` and `hub_utils.init()` #242

Added `save_format` to `config.json` and `hub_utils.init()` #242