-
Notifications
You must be signed in to change notification settings - Fork 54
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
Add tabular regression example #254
Add tabular regression example #254
Conversation
05aa485
to
3cde33b
Compare
I'm okay with adding this, but it'd need to add a few things for the example to be more useful:
Thanks for the work @lazarust |
Hi @adrinjalali, thanks for taking a look at my PR! I've addressed your first point and replaced using pickle with |
bd88712
to
8dc669e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks,
This should also add the generating file the same way as it's done here:
Lines 85 to 87 in 495abe6
if "__file__" in locals(): # __file__ not defined during docs built | |
# Add this script itself to the files to be uploaded for reproducibility | |
hub_utils.add_files(__file__, dst=local_repo) |
examples/plot_tabular_regression.py
Outdated
# Inference | ||
# ========= | ||
# Let's see if the model works. | ||
prediction_data = [[100, 200, 300, 400, 500, 600, 700, 800, 900, 1000]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should use a few rows from X_test
instead.
examples/plot_tabular_regression.py
Outdated
# to push your models please refer to | ||
# :ref:`this example <sphx_glr_auto_examples_plot_hf_hub.py>`. | ||
|
||
model_card.save(Path(local_repo) / "README.md") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing new line at the end of the file.
8dc669e
to
86af97e
Compare
@adrinjalali I've made those requested changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks good, it's only nits, thank you!!!
examples/plot_tabular_regression.py
Outdated
" This model is pretty limited and should just be used as an example of how to user `skops` and Hugging Face Hub." | ||
) | ||
model_card_authors = "skops_user, lazarust" | ||
get_started_code = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is generated when use skops template (which is default), see here and here) should we keep it like this for this tutorial? @adrinjalali
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no we should remove them now.
3476e20
to
441c2a5
Compare
@merveenoyan I think I've addressed all of your comments. |
876ada2
to
7c34274
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! Thanks a lot!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noticed tests aren't passing :/
examples/plot_tabular_regression.py
Outdated
model_card_authors = "skops_user, lazarust" | ||
citation_bibtex = "bibtex\n@inproceedings{...,year={2022}}" | ||
model_card.add(**{ | ||
"How to Get Started with the Model": get_started_code, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"How to Get Started with the Model": get_started_code, |
You can remove it as discussed 🙂
7c34274
to
2fc3fd5
Compare
@merveenoyan Tests should be fixed now. I didn't realize that there was a check for formatting using Black. |
@lazarust yesterday I saw the workflow failing for docs tests, as in, your script was failing, that's why. All seems good now! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
@BenjaminBossan could you skim through it and see if it's ok to merge? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, this looks very good to me, thanks for the work. I have some minor comments, please take a look.
There is also a bigger issue: If I'm not mistaken, this example isn't linked anywhere in the docs. That means it would be really hard for readers to find it, they would have to manually change the URL to https://skops.readthedocs.io/en/stable/auto_examples/ to get there. Probably it would be best if we linked to the auto examples from some place, not necessarily to each auto example individually.
docs/changes.rst
Outdated
@@ -83,4 +83,4 @@ Contributors | |||
|
|||
:user:`Adrin Jalali <adrinjalali>`, :user:`Merve Noyan <merveenoyan>`, | |||
:user:`Benjamin Bossan <BenjaminBossan>`, :user:`Ayyuce Demirbas | |||
<ayyucedemirbas>`, :user:`Prajjwal Mishra <p-mishra1>` | |||
<ayyucedemirbas>`, :user:`Prajjwal Mishra <p-mishra1>`, :user:`Thomas Lazarus <lazarust>` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there not also be an entry (in a new section "v0.5") about what was added? Otherwise, it's hard to see the changes at a glance.
examples/plot_tabular_regression.py
Outdated
prediction_data = X_test[:5] | ||
prediction = model.predict(prediction_data) | ||
print(prediction) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prediction_data = X_test[:5] | |
prediction = model.predict(prediction_data) | |
print(prediction) | |
y_pred = model.predict(X_test[:5]) | |
print(y_pred) |
This is not a must, but I would prefer we use the canonical y_pred
for the output of model.predict
(further below, we also use y_pred
). I know that in plot_text_classification.py
, we also don't use y_pred
, but I'd rather change it there as well, than keeping it here.
We can have a page where we manually list all examples in well defined categories for better discoverability. |
Isn't that https://skops.readthedocs.io/en/stable/auto_examples/? We just need to link to it somewhere. |
That file is auto generated and not really nice to read @BenjaminBossan , would be nicer for us to have a file which we maintain instead of linking to this autogenerated one. |
35dccf1
to
1ab80e4
Compare
@BenjaminBossan I added the page to the documentation, but I don't super love the title and description of the examples. Let me know if you want me to change them! |
docs/examples.rst
Outdated
@@ -0,0 +1,9 @@ | |||
.. _examples: | |||
|
|||
Examples of using skops |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the "of using skops" part is a bit redundant inside of the skops documentation ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Examples of using skops | |
Examples of interactions with the Hugging Face Hub |
docs/examples.rst
Outdated
- Tabular Regression: | ||
`Here <https://github.com/skops-dev/skops/blob/main/examples/plot_tabular_classification.py>`_ is an example of using skops to serialize a tabular regression model and create a model card and a Hugging Face Hub repository. | ||
- Text Classification: | ||
`Here <https://github.com/skops-dev/skops/blob/main/examples/plot_text_classification.py>`_ is an example of using skops to serialize a text classification model and create a model card and a Hugging Face Hub repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would expect all auto-examples to be listed here:
https://skops--254.org.readthedocs.build/en/254/auto_examples/
At the same time, the paragraph on the start page of the skops page becomes a bit redundant with this new page:
The following examples are good starting points: ...
(link)
Should we remove it in favor of just linking to this page? @adrinjalali @merveenoyan WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that paragraph can stay since we can only link to a few examples where people can start, while also linking to this page with all examples listed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@adrinjalali I'm a little confused, are we wanting to link from the start page to this examples page?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes we do @lazarust
docs/examples.rst
Outdated
@@ -0,0 +1,9 @@ | |||
.. _examples: | |||
|
|||
Examples of using skops |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Examples of using skops | |
Examples of interactions with the Hugging Face Hub |
docs/examples.rst
Outdated
- Tabular Regression: | ||
`Here <https://github.com/skops-dev/skops/blob/main/examples/plot_tabular_classification.py>`_ is an example of using skops to serialize a tabular regression model and create a model card and a Hugging Face Hub repository. | ||
- Text Classification: | ||
`Here <https://github.com/skops-dev/skops/blob/main/examples/plot_text_classification.py>`_ is an example of using skops to serialize a text classification model and create a model card and a Hugging Face Hub repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please keep the lines at max 79 chars
docs/examples.rst
Outdated
- Tabular Regression: | ||
`Here <https://github.com/skops-dev/skops/blob/main/examples/plot_tabular_classification.py>`_ is an example of using skops to serialize a tabular regression model and create a model card and a Hugging Face Hub repository. | ||
- Text Classification: | ||
`Here <https://github.com/skops-dev/skops/blob/main/examples/plot_text_classification.py>`_ is an example of using skops to serialize a text classification model and create a model card and a Hugging Face Hub repository. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that paragraph can stay since we can only link to a few examples where people can start, while also linking to this page with all examples listed.
d5b8b67
to
bff9f26
Compare
@adrinjalali @BenjaminBossan I've addressed your comments other than linking from the start page (if we want to do that). |
docs/examples.rst
Outdated
regression model and create a model card and a Hugging Face Hub repository. | ||
- Text Classification: | ||
`Here <https://github.com/skops-dev/skops/blob/main/examples/plot_text_cl | ||
assification.py>`_ is an example of using skops to serialize a text classi |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You accidentally split the word "classification" into 2.
docs/examples.rst
Outdated
_classification.py>`_ is an example of using skops to serialize a tabular | ||
regression model and create a model card and a Hugging Face Hub repository. | ||
- Text Classification: | ||
`Here <https://github.com/skops-dev/skops/blob/main/examples/plot_text_cl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the links here are to the .py files on GH. However, it is better to point them to the rendered documentation. E.g. for text classification, that would be: https://skops.readthedocs.io/en/stable/auto_examples/plot_text_classification.html
190c2f3
to
2ab5941
Compare
@BenjaminBossan I've made the updates you requested, but it looks like there's an error happening in |
@lazarust I have no idea, it seems unlikely that any change you made could cause that. I tried to trigger a rebuild of the docs in case the problem was just temporary, but didn't see an option to do that. Could you please push an empty commit to trigger it? |
I triggered a rebuild on the readthedocs side and it passes |
@lazarust please avoid force pushing to your branch. We squash and merge at the end, so it doesn't matter how many commits with what commit messages you have in between. |
@adrinjalali Should I update the branch via rebase or merge commit? I've also added the link to the index page. |
Resolves skops-dev#248 This example showcases how to use custom templates with the skops model card class. The new document is not yet explicitly linked in the documentation yet. Ideally, this can be done once skops-dev#254 has been finalized.
@BenjaminBossan This is ready for you to review again |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just this typo, then we should be good :)
Co-authored-by: Benjamin Bossan <BenjaminBossan@users.noreply.github.com>
@BenjaminBossan Fixed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thanks a lot for working on this!
* [skip ci] Add example of using a custom template Resolves #248 This example showcases how to use custom templates with the skops model card class. The new document is not yet explicitly linked in the documentation yet. Ideally, this can be done once #254 has been finalized. * [skip ci] Add entry to changes.rst
No description provided.