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

Add tabular regression example #254

Merged

Conversation

lazarust
Copy link
Contributor

No description provided.

@lazarust lazarust force-pushed the add-tabular-regression-example branch 2 times, most recently from 05aa485 to 3cde33b Compare December 15, 2022 01:09
@adrinjalali
Copy link
Member

I'm okay with adding this, but it'd need to add a few things for the example to be more useful:

  • use skops format skops.io instead of pickle
  • Have a story behind the example. It should be something where we could also explain limitations of the model, and disparities if they exist, to highlight how the model card should be used and written.
  • Add a more detailed model card, with all sections filled and add a few different inspection and interpretation methods to better explain the model

Thanks for the work @lazarust

@lazarust
Copy link
Contributor Author

Hi @adrinjalali, thanks for taking a look at my PR! I've addressed your first point and replaced using pickle with skops.io. I did have some questions about your other two issues though. I tweaked the model text and comments but was unsure what sections I was missing. I based this example off of examples/plot_text_classification.py. If you could let me know what else you want to be added I can go ahead and add it and update the text classification example too. Thanks!

@lazarust lazarust force-pushed the add-tabular-regression-example branch 4 times, most recently from bd88712 to 8dc669e Compare December 20, 2022 15:04
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.

Thanks,

This should also add the generating file the same way as it's done here:

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)

# Inference
# =========
# Let's see if the model works.
prediction_data = [[100, 200, 300, 400, 500, 600, 700, 800, 900, 1000]]
Copy link
Member

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.

# 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")
Copy link
Member

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.

@lazarust lazarust force-pushed the add-tabular-regression-example branch from 8dc669e to 86af97e Compare January 4, 2023 00:13
@lazarust
Copy link
Contributor Author

lazarust commented Jan 4, 2023

@adrinjalali I've made those requested changes

Copy link
Collaborator

@merveenoyan merveenoyan left a 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 Show resolved Hide resolved
" 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 = (
Copy link
Collaborator

@merveenoyan merveenoyan Jan 5, 2023

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

Copy link
Member

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.

examples/plot_tabular_regression.py Outdated Show resolved Hide resolved
@lazarust lazarust force-pushed the add-tabular-regression-example branch from 3476e20 to 441c2a5 Compare January 10, 2023 02:27
@lazarust
Copy link
Contributor Author

@merveenoyan I think I've addressed all of your comments.

@lazarust lazarust force-pushed the add-tabular-regression-example branch from 876ada2 to 7c34274 Compare January 10, 2023 15:32
Copy link
Collaborator

@merveenoyan merveenoyan left a 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!

Copy link
Collaborator

@merveenoyan merveenoyan left a 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 :/

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,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"How to Get Started with the Model": get_started_code,

You can remove it as discussed 🙂

@lazarust lazarust force-pushed the add-tabular-regression-example branch from 7c34274 to 2fc3fd5 Compare January 16, 2023 20:56
@lazarust
Copy link
Contributor Author

@merveenoyan Tests should be fixed now. I didn't realize that there was a check for formatting using Black.

@merveenoyan
Copy link
Collaborator

merveenoyan commented Jan 17, 2023

@lazarust yesterday I saw the workflow failing for docs tests, as in, your script was failing, that's why. All seems good now!

Copy link
Collaborator

@merveenoyan merveenoyan left a comment

Choose a reason for hiding this comment

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

LGTM!

examples/plot_tabular_regression.py Show resolved Hide resolved
@merveenoyan
Copy link
Collaborator

@BenjaminBossan could you skim through it and see if it's ok to merge?

Copy link
Collaborator

@BenjaminBossan BenjaminBossan left a 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>`
Copy link
Collaborator

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.

Comment on lines 59 to 61
prediction_data = X_test[:5]
prediction = model.predict(prediction_data)
print(prediction)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
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.

@adrinjalali
Copy link
Member

We can have a page where we manually list all examples in well defined categories for better discoverability.

@BenjaminBossan
Copy link
Collaborator

BenjaminBossan commented Jan 19, 2023

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.

@adrinjalali
Copy link
Member

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.

@lazarust lazarust force-pushed the add-tabular-regression-example branch from 35dccf1 to 1ab80e4 Compare January 26, 2023 01:51
@lazarust
Copy link
Contributor Author

@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!

@@ -0,0 +1,9 @@
.. _examples:

Examples of using skops
Copy link
Collaborator

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 ;)

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Examples of using skops
Examples of interactions with the Hugging Face Hub

- 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.
Copy link
Collaborator

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?

Copy link
Member

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.

Copy link
Contributor Author

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?

Copy link
Member

Choose a reason for hiding this comment

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

Yes we do @lazarust

@@ -0,0 +1,9 @@
.. _examples:

Examples of using skops
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Examples of using skops
Examples of interactions with the Hugging Face Hub

Comment on lines 6 to 9
- 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.
Copy link
Member

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

- 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.
Copy link
Member

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.

@lazarust lazarust force-pushed the add-tabular-regression-example branch from d5b8b67 to bff9f26 Compare January 27, 2023 03:31
@lazarust
Copy link
Contributor Author

@adrinjalali @BenjaminBossan I've addressed your comments other than linking from the start page (if we want to do that).

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
Copy link
Collaborator

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.

_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
Copy link
Collaborator

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

@lazarust lazarust force-pushed the add-tabular-regression-example branch from 190c2f3 to 2ab5941 Compare January 28, 2023 16:45
@lazarust
Copy link
Contributor Author

@BenjaminBossan I've made the updates you requested, but it looks like there's an error happening in plot_hf_hub.py when it uploads the model to the Hub. Would you be able to help me figure out why this is happening?

@BenjaminBossan
Copy link
Collaborator

@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?

@adrinjalali
Copy link
Member

I triggered a rebuild on the readthedocs side and it passes

@adrinjalali
Copy link
Member

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

@lazarust
Copy link
Contributor Author

lazarust commented Jan 31, 2023

@adrinjalali Should I update the branch via rebase or merge commit? I've also added the link to the index page.

BenjaminBossan added a commit to BenjaminBossan/skops that referenced this pull request Feb 3, 2023
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.
@lazarust
Copy link
Contributor Author

lazarust commented Feb 8, 2023

@BenjaminBossan This is ready for you to review again

Copy link
Collaborator

@BenjaminBossan BenjaminBossan left a 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 :)

docs/examples.rst Outdated Show resolved Hide resolved
Co-authored-by: Benjamin Bossan <BenjaminBossan@users.noreply.github.com>
@lazarust
Copy link
Contributor Author

lazarust commented Feb 8, 2023

@BenjaminBossan Fixed!

Copy link
Collaborator

@BenjaminBossan BenjaminBossan left a 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!

@BenjaminBossan BenjaminBossan merged commit b324b8d into skops-dev:main Feb 8, 2023
adrinjalali pushed a commit that referenced this pull request Feb 27, 2023
* [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
@lazarust lazarust deleted the add-tabular-regression-example branch April 7, 2023 01:55
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.

4 participants