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

CLI: Command to update a skops file #333

Closed
BenjaminBossan opened this issue Mar 28, 2023 · 5 comments · Fixed by #343
Closed

CLI: Command to update a skops file #333

BenjaminBossan opened this issue Mar 28, 2023 · 5 comments · Fixed by #343
Labels
enhancement New feature or request help wanted Extra attention is needed persistence Secure persistence feature

Comments

@BenjaminBossan
Copy link
Collaborator

The skops persistence format is updated over time, which may lead to bugs being fixed, performance being improved, reduced file size, etc. Therefore, as a user, I may want to upgrade my skops files periodically to ensure I can take advantage of the changes. Right now, I would need to do that manually but it would be nice for the skops CLI to offer this possibility.

Some optional features that come to mind:

  • allow updating multiple files at once
  • report if files where upgraded or nothing was changed
  • some kind of check that upgraded file produces same object
  • probably out of scope: upgrade to specified protcol version (or even downgrade)
@BenjaminBossan BenjaminBossan added enhancement New feature or request help wanted Extra attention is needed persistence Secure persistence feature labels Mar 28, 2023
@EdAbati
Copy link
Contributor

EdAbati commented Mar 31, 2023

Hi @BenjaminBossan , this sounds like a fun issue to work on. I can take it, if it's ok. 🙂

@adrinjalali
Copy link
Member

@EdAbati sure, go for it :)

@EdAbati
Copy link
Contributor

EdAbati commented Apr 10, 2023

Hi both,
I finally had some time to have a look at this one (see #343) and I have a couple of questions regarding the optional points above:

report if files where upgraded or nothing was changed

I thought we could compare the schemas (ignoring the __id__ values since they are not deterministic). Is there anything else that could be updated/changed?

some kind of check that upgraded file produces same object

I'm a bit unsure how we could do this. Maybe by allowing the user to provide some test data and checking that the predictions are the same? 🤔

@adrinjalali
Copy link
Member

I'm a bit unsure how we could do this. Maybe by allowing the user to provide some test data and checking that the predictions are the same? 🤔

We should probably use assert_params_equal which we use in our tests already. WDYT @BenjaminBossan ?

I thought we could compare the schemas (ignoring the id values since they are not deterministic). Is there anything else that could be updated/changed?

Sounds about right.

@BenjaminBossan
Copy link
Collaborator Author

We should probably use assert_params_equal which we use in our tests already.

That would be a possibility. Since it creates overhead, maybe it should be optional (could be added in a later PR too).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed persistence Secure persistence feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants