Added `save_format` to `config.json` and `hub_utils.init()` by merveenoyan · Pull Request #242 · skops-dev/skops

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.

BenjaminBossan

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

Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>

adrinjalali

Thanks @merveenoyan

We also need an entry in the changelog

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

+        # 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

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

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

+- 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 <adrin.jalali@gmail.com>

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

Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>

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

+- 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

        ``"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.

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

@adrinjalali I accidentally asked for review 🥲

Co-authored-by: Benjamin Bossan <BenjaminBossan@users.noreply.github.com>

BenjaminBossan

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

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

+- 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

Comment thread skops/hub_utils/_hf_hub.py Outdated

Comment thread skops/hub_utils/_hf_hub.py Outdated

Comment thread skops/hub_utils/_hf_hub.py Outdated

BenjaminBossan reviewed Dec 8, 2022

View reviewed changes

Comment thread skops/hub_utils/tests/test_hf_hub.py Outdated

Comment thread skops/hub_utils/_hf_hub.py Outdated

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

Update skops/hub_utils/_hf_hub.py

ece88dd

Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>

Update skops/hub_utils/_hf_hub.py

ea6c5ae

Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>

Update skops/hub_utils/_hf_hub.py

a6ffc3d

Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>

addressed comments

59c45a4

merveenoyan requested review from BenjaminBossan and adrinjalali 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 <adrin.jalali@gmail.com>

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

Comment thread docs/changes.rst Outdated

Update docs/changes.rst

ed70096

Co-authored-by: Adrin Jalali <adrin.jalali@gmail.com>

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 <BenjaminBossan@users.noreply.github.com>

Update skops/hub_utils/_hf_hub.py

9dda4e6

Co-authored-by: Benjamin Bossan <BenjaminBossan@users.noreply.github.com>

Update skops/hub_utils/_hf_hub.py

a0a5b45

Co-authored-by: Benjamin Bossan <BenjaminBossan@users.noreply.github.com>

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

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

Conversation

merveenoyan commented Dec 5, 2022

Uh oh!

adrinjalali left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BenjaminBossan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

adrinjalali left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

merveenoyan commented Dec 9, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

merveenoyan commented Dec 9, 2022

Uh oh!

merveenoyan commented Dec 12, 2022

Uh oh!

adrinjalali left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

adrinjalali commented Dec 12, 2022

Uh oh!

BenjaminBossan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

merveenoyan commented Dec 12, 2022

Uh oh!

BenjaminBossan left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants