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 flax installation in daily doctest workflow #25860

Merged
merged 1 commit into from
Aug 30, 2023

Conversation

ydshieh
Copy link
Collaborator

@ydshieh ydshieh commented Aug 30, 2023

What does this PR do?

#25763 enables the doctesting against a few more files, including some flax related files. As the docker image doesn't have flax/jax installed, the pytest failed to collect the test to run, see this doctest run:

src/transformers/generation/flax_utils.py - ModuleNotFoundError: No module named 'flax'

Let's install jax/flax in the doctest workflow, and see what this gives us.

Note we don't want to install jax/flax in the docker image, as the same image is used for daily (non-doctest) CI, and we don't want to have them to (potentially) interfere the CI.

@ydshieh ydshieh requested review from amyeroberts and gante August 30, 2023 09:28
@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented Aug 30, 2023

The documentation is not available anymore as the PR was closed or merged.

@amyeroberts
Copy link
Collaborator

As Flax and Jax break our environments frequently because of (large) breaking changes in their releases and it's a pain to managed, could we pin the install to the highest version currently allowed in setup.py?

@ydshieh
Copy link
Collaborator Author

ydshieh commented Aug 30, 2023

Hi @amyeroberts , with

python3 -m pip install -e .[flax]

it should already respect the versions we specify in setup.py. Correct me otherwise 🙏

Copy link
Member

@gante gante left a comment

Choose a reason for hiding this comment

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

👍

@gante
Copy link
Member

gante commented Aug 30, 2023

@amyeroberts jax, jaxlib, and flax are pinned in the setup, so it should be okay :D (e.g. here)

@amyeroberts
Copy link
Collaborator

@gante @ydshieh 🤦‍♀️ d'oh, my bad, sorry I just skimmed the pip command

Copy link
Collaborator

@amyeroberts amyeroberts left a comment

Choose a reason for hiding this comment

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

Thanks for adding!

@ydshieh ydshieh merged commit 62399d6 into main Aug 30, 2023
3 checks passed
@ydshieh ydshieh deleted the add_flax_to_daily_ci_doctest branch August 30, 2023 13:13
parambharat pushed a commit to parambharat/transformers that referenced this pull request Sep 26, 2023
fix

Co-authored-by: ydshieh <ydshieh@users.noreply.github.com>
blbadger pushed a commit to blbadger/transformers that referenced this pull request Nov 8, 2023
fix

Co-authored-by: ydshieh <ydshieh@users.noreply.github.com>
EduardoPach pushed a commit to EduardoPach/transformers that referenced this pull request Nov 18, 2023
fix

Co-authored-by: ydshieh <ydshieh@users.noreply.github.com>
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