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 fastai upstream and downstream capacities for fastai>=2.4 and fastcore>=1.3.27 versions #678

Merged
merged 87 commits into from Apr 26, 2022
Merged

Conversation

omarespejel
Copy link
Contributor

@omarespejel omarespejel commented Feb 10, 2022

Sorry, I created a new PR (previous one: PR huggingface/huggingface_hub#416) I had a couple of mistakes with git, and seemed easier to create a new one. Thanks @muellerzr and @osanseviero for your comments!

What

  • Add fastai upstream and downstream capabilities for versions fastai>=2.4 (link) and fastcore>=1.3.27 (link).
  • Inform users that lower versions of fastai are not supported yet (TODO -- Examine whether it is worth implementing previous versions).

Why

  • Supporting version fastai1 might not be worth it. Loading and pushing Learners involves several changes and the fastai1 library has not been updated for over a year.
  • Supporting versions 2.0.6>= fastai <2.4 involve complex changes. Fastai was updated due to what appear to be modifications to pytorch/serialization.py. For the sake of agility in our first release, I suggest releasing this version without supporting these previous versions.
  • fastai>=2.4 versions work with fastcore>=1.3.27 version which is installed automatically when fastai>=2.4 is installed.

How?

  • Following huggingface_hub practices, file_download.py checks for fastcore and fastai availability and versions.
  • The fastai and fastcore versions used to train the Learner are automatically stored in a config.json when pushed to hub.
  • A README.md is automatically generated, if none, with the fastai tag.

Testing?

  • Tested that all fastai>=2.4 versions work with the present code and with the fastcore==1.3.27 version.

Next?

  • Examine whether it is worth implementing fastai <2.4 versions. I will ask on the fastai forums to assess how many users would require this support.
  • Update description on how to load fastai models in Libraries.ts.

@omarespejel omarespejel added the enhancement New feature or request label Feb 10, 2022
@omarespejel omarespejel self-assigned this Feb 10, 2022
@omarespejel omarespejel changed the title Add fastai mixin Add fastai upstream and downstream capacities for fastai>=2.4 and fastcore>=1.3.27 versions Feb 10, 2022
Copy link
Contributor

@muellerzr muellerzr left a comment

Choose a reason for hiding this comment

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

Very nice! I just left a few nits :)

src/huggingface_hub/fastai_mixin.py Outdated Show resolved Hide resolved
src/huggingface_hub/fastai_mixin.py Outdated Show resolved Hide resolved
src/huggingface_hub/fastai_mixin.py Outdated Show resolved Hide resolved
src/huggingface_hub/fastai_mixin.py Outdated Show resolved Hide resolved
Copy link
Member

@osanseviero osanseviero left a comment

Choose a reason for hiding this comment

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

Thank you very much for the PR! There are some important things to address, but overall is going in right direction 🚀

src/huggingface_hub/fastai_mixin.py Outdated Show resolved Hide resolved
src/huggingface_hub/fastai_mixin.py Outdated Show resolved Hide resolved
src/huggingface_hub/fastai_mixin.py Outdated Show resolved Hide resolved
src/huggingface_hub/fastai_mixin.py Outdated Show resolved Hide resolved
src/huggingface_hub/fastai_mixin.py Outdated Show resolved Hide resolved
src/huggingface_hub/fastai_mixin.py Outdated Show resolved Hide resolved
src/huggingface_hub/fastai_mixin.py Outdated Show resolved Hide resolved
src/huggingface_hub/fastai_mixin.py Outdated Show resolved Hide resolved
src/huggingface_hub/fastai_mixin.py Outdated Show resolved Hide resolved
src/huggingface_hub/fastai_mixin.py Outdated Show resolved Hide resolved
@omarespejel
Copy link
Contributor Author

Hi, @osanseviero @muellerzr thank you for your kind reviews guys! All feedback was applied and tested.

Main changes:

  • Changed name. From fastai_mixin.py to fastai_utils.py.
  • No longer a mixin. Eliminated FastaiModelHubMixin class and only leaved three main public functions: from_pretrained_fastai, push_to_hub_fastai, and save_fastai_learner. This helps with simplicity.
  • Imported fastai_utils.py public functions in huggingface_hub/__init__.py.
  • Failing test. Build (3.6) and Build (3.9) actions failing only due to the test_inference_with_audio test. Will review the reason.
  • Created check_fastai_fastcore_versions function to not force fastai imports from the beggining.

Will be here :D

@osanseviero
Copy link
Member

The failing tests are unrelated. They are flaky and likely not related to this PR, so don't worry about it

Copy link
Member

@osanseviero osanseviero left a comment

Choose a reason for hiding this comment

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

Looking good! I think it's in a good state to start adding some tests 😄

src/huggingface_hub/fastai_utils.py Outdated Show resolved Hide resolved
src/huggingface_hub/fastai_utils.py Outdated Show resolved Hide resolved
src/huggingface_hub/fastai_utils.py Outdated Show resolved Hide resolved
src/huggingface_hub/fastai_utils.py Outdated Show resolved Hide resolved
src/huggingface_hub/fastai_utils.py Outdated Show resolved Hide resolved
src/huggingface_hub/fastai_utils.py Show resolved Hide resolved
* fastai was made compatible with Python 3.10 just in the most recent version 3.5.6 released on April 1st. I would prefer to wait for the next release of fastai.
* Additionally, isort test_fastai_integration.py
* Change the name from save_fastai_learner to _save_pretrained_fastai
* isort __init__.py
* functions from_pretrained_fastai and push_to_hub_fastai were added to mixins.mdx
Copy link
Member

@osanseviero osanseviero left a comment

Choose a reason for hiding this comment

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

Cool 🚀 Thanks for iterating on this

src/huggingface_hub/fastai_utils.py Outdated Show resolved Hide resolved
src/huggingface_hub/fastai_utils.py Outdated Show resolved Hide resolved
src/huggingface_hub/fastai_utils.py Outdated Show resolved Hide resolved
src/huggingface_hub/fastai_utils.py Show resolved Hide resolved
src/huggingface_hub/fastai_utils.py Outdated Show resolved Hide resolved
src/huggingface_hub/fastai_utils.py Outdated Show resolved Hide resolved
src/huggingface_hub/fastai_utils.py Outdated Show resolved Hide resolved
src/huggingface_hub/fastai_utils.py Outdated Show resolved Hide resolved
omarespejel and others added 14 commits April 21, 2022 11:42
…rectory

* By default learner.export saves learner to learner.path
* Before we where saving the model in learner.path and them moving it to save_directory
* Fix by changing learner.path to being equal to save_directory
* Additionally, makes optional that the pyproject.toml contains the fastai and fastcore versions
* Will continue to throw an  error if the toml library is not available
* Will continue to thrown an error if the pyproject.toml specifies fastai or fastcore versions that are not supported by from_pretrained_fastai
* This will allow to load fastai models from the Hub that were not necessarily uploaded using push_to_hub_fastai.
* This allows to eliminate the returns when checking for the versions of fastai and fastcore in the pyproject.toml
* This allows to eliminate the returns when checking for the versions of fastai and fastcore in the pyproject.toml
@osanseviero osanseviero merged commit cab4152 into huggingface:main Apr 26, 2022
@osanseviero
Copy link
Member

Congrats @omarespejel! 🚀 🚀 🚀

@omarespejel omarespejel deleted the add-fastai-mixin branch April 26, 2022 18:33
@omarespejel
Copy link
Contributor Author

Thank you @osanseviero @adrinjalali @muellerzr @nateraw! For your reviews and feedback 🤗🥳

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants