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

FEAT Add skops space creator app #307

Merged
merged 29 commits into from Apr 20, 2023

Conversation

BenjaminBossan
Copy link
Collaborator

Description

This is the code for a streamlit app whose goal it is to let users easily build HF spaces for sklearn models. In particular, it offers a GUI to interface with our skops.card.Card class, giving users a wysiwyg experience.

The app is currently hosted on my personal account:

https://huggingface.co/spaces/BenjaminB/skops-model-card-creator2

We want to move it to skops and update it automatically.

Comments

I have adopted the CI to create this space using skops-ci. If the space itself is faulty, the CI job will still finish successfully, but at least one can check it and do some manual testing.

Adding automatic tests will prove quite difficult, because, AFAICT, streamlit doesn't have anything like a testing client. Automatic testing would require some heavy lifting from the likes of Selenium or Playwright.

It is possible to write tests for the parts of the code that are streamlit-agnostic, e.g. in tasks.py. However, that's the least brittle part of the code, so the additional value is low.

I had some strange issues with pre-commit and mypy. I don't want mypy to check the streamlit app -- it is a bit hacky in some places, which results in mypy errors. Therefore, I added an exclusion rule to pyproject.toml, which is indeed respected when I run mypy manually. However, pre-commit will still check the space, even though it's told to use the pyproject.toml. Thus I had to exclude the space code explicitly in the .pre-commit-config.yaml. If anyone knows what the issue is, please let me know.

TODO

Right now, there is no job that automatically builds this space and pushes it to the sklearn orga. I assume for that, we should create a workflow similar to clean-skops-user.yml that runs regularly and that uses a secret token so that random people cannot override the app.

BenjaminBossan and others added 16 commits February 27, 2023 17:29
Description

This is the code for a streamlit app whose goal it is to let users
easily build HF spaces for sklearn models. In particular, it offers a
GUI to interface with our skops.card.Card class, giving users a wysiwyg
experience.

The app is currently hosted on my personal account:

https://huggingface.co/spaces/BenjaminB/skops-model-card-creator2

We want to move it to skops and update it automatically.

Comments

I have adopted the CI to create this space using skops-ci. If the space
itself is faulty, the CI job will still finish successfully, but at
least one can check it and do some manual testing.

Adding automatic tests will prove quite difficult, because, AFAICT,
streamlit doesn't have anything like a testing client. Automatic testing
would require some heavy lifting from the likes of Selenium or
Playwright.

It is possible to write tests for the parts of the code that are
streamlit-agnostic, e.g. in tasks.py. However, that's the least brittle
part of the code, so the additional value is low.

Right now, there is no job that automatically builds this space and
pushes it to the sklearn orga. I assume for that, we should create a
workflow similar to clean-skops-user.yml that runs regularly and that
uses a secret token so that random people cannot override the app.

I had some strange issues with pre-commit and mypy. I don't want mypy to
check the streamlit app -- it is a bit hacky in some places, which
results in mypy errors. Therefore, I added an exclusion rule to
pyproject.toml, which is indeed respected when I run mypy manually.
However, pre-commit will stil check the repo, even though it's told to
use the pyproject.toml. Thus I had to exclude the space code explicitly
in the pre-commit-config.yaml. If anyone knows what the issue is, please
let me know.
Setup fails otherwise, as it thinks that spaces could also be part of
the package.
@adrinjalali
Copy link
Member

So this is now importing the token. It's odd cause the effective change is adding -e. Which suggests somehow the installation is odd when -e is missing. I tried locally with a new env, installed skops from pypi, and the test files exist and I can import the token. I'm not sure why it doesn't work on the CI.

Even though the install steps are shared, it doesn't make sense to put
the space deployment job  into the testing workflow, since the latter
runs multiple times because of the test matrix.

The secret for deploying on the HF scikit-learn orga is not set yet, so
this is WIP.
Forgot to add it with last commit ...
@BenjaminBossan
Copy link
Collaborator Author

BenjaminBossan commented Mar 3, 2023

@skops-dev/maintainers I made all changes as discussed here and with Adrin on Slack. Whether the "main" deployment works, we will only see once we merge to main :) The test space is deployed here:

https://huggingface.co/spaces/skops-ci/skops-space-creator-b5559d5e-667a-4498-a0b2-a8df12cf5ee5

Before going further, some points:

  1. some changes in MNT: refactor model card to render sections lazily #310 may affect the space (I think: facilitate some stuff). It's probably easier to wait for it to land first
  2. I'm not happy with the name "skops space creator", any better suggestions are welcome
  3. do we already want to document this space (+ the converter space) in the docs or not yet?

@adrinjalali
Copy link
Member

There's a warning on the bottom of the space you link here:

image

I'm not happy with the name "skops space creator", any better suggestions are welcome

To me it's more of a scikit-learn model card creator/editor, or skops model card manager or something. It's not creating a space, it's creating a model card.

do we already want to document this space (+ the converter space) in the docs or not yet?

We can keep that for another PR.

@BenjaminBossan
Copy link
Collaborator Author

There's a warning on the bottom of the space you link here:

That's what I mentioned about flakiness of streamlit, refreshing should remove the error, and everything works despite of it. I add the button with that key only once, I really don't have an idea how the key is duplicated (would the button not appear twice then??).

To me it's more of a scikit-learn model card creator/editor, or skops model card manager or something. It's not creating a space, it's creating a model card.

Although I agree that this the main purpose, it's also not quite all of it. First of all, you can also create a model repo, not just the model card, second, it technically can also be used to create/edit any kind of model card. Also, I think having "skops" appear prominently would be good.

@adrinjalali
Copy link
Member

@BenjaminBossan should we move this forward?

@BenjaminBossan
Copy link
Collaborator Author

should we move this forward?

Yes, what is the next step?

@adrinjalali
Copy link
Member

You have some items here: #307 (comment)

  • I think we can document this later
  • maybe call it skops README generator?
  • do the modifications based on the now-merged PR

Most notably, the new card class stores the specific section types like
PlotSection in its data. This allows us to more directly modify those
sections, e.g. when a user updates a plot file.

Also use the new get_toc method.

Another change was to add a specific task for updating the title of a
plot, instead of having a big task both for title and content with many
if...else. This is cleaner.
@BenjaminBossan
Copy link
Collaborator Author

Hmm, I get AttributeError: 'Card' object has no attribute 'get_toc' on the deployed space, not sure how that's possible...

@adrinjalali
Copy link
Member

Hmm, I get AttributeError: 'Card' object has no attribute 'get_toc' on the deployed space, not sure how that's possible...

Would a factory reset on the space help? but this is odd since they're unique spaces anyway. Hmm...

@BenjaminBossan
Copy link
Collaborator Author

Would a factory reset on the space help? but this is odd since they're unique spaces anyway. Hmm...

Exactly. I reran the whole workflow, the new space has the same issue.

@adrinjalali
Copy link
Member

Something in the manifest/setup must be flaky, we're seeing too many of these issues.

@BenjaminBossan
Copy link
Collaborator Author

BenjaminBossan commented Apr 6, 2023

Something in the manifest/setup must be flaky, we're seeing too many of these issues.

Possibly, but when I was debugging that stuff, I couldn't identify anything (e.g. I tried removing MANIFEST completely and starting from an almost empty setup.py). Maybe pip cache somehow messes stuff up even with --no-cache-dir and even on GH/HF, no idea, but I'm grasping at straws :D

@adrinjalali
Copy link
Member

Should we merge this, have it deployed, and then try to fix it?

@BenjaminBossan
Copy link
Collaborator Author

Should we merge this, have it deployed, and then try to fix it?

We could try that and just don't advertise this feature until it's fixed.

Copy link
Member

@adrinjalali adrinjalali left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's merge and iterate then :)

@adrinjalali adrinjalali merged commit 96f8d35 into skops-dev:main Apr 20, 2023
16 checks passed
@BenjaminBossan BenjaminBossan deleted the skops-space-creator branch April 20, 2023 14:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants