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

dependencies: keras-nlp<0.14 pin #31684

Merged
merged 3 commits into from
Jul 1, 2024
Merged

Conversation

gante
Copy link
Member

@gante gante commented Jun 28, 2024

What does this PR do?

On the latest keras-nlp release -- "KerasNLP no longer supports Keras 2.". This means we don't support this version, as we only support Keras 2

@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

@amyeroberts
Copy link
Collaborator

Fine with pinning to keep things working for now, but would be good to have a plan for future keras updates. @Rocketknight1 could we remove keras-nlp?

also cc @ydshieh for reference

@gante gante closed this Jun 28, 2024
@gante gante reopened this Jun 28, 2024
@gante
Copy link
Member Author

gante commented Jun 28, 2024

@amyeroberts @ydshieh I followed our docs for version updates, but the resulting PR doesn't look right.

What's the expected procedure to update breaking versions? 🤔

I've confirmed that:

  • having keras-nlp==0.14.0 causes these test errors
  • calling pip install "keras-nlp<0.14" -U fixes it

However, since the docker images are not updated in this PR, CI will continue to be red

@amyeroberts
Copy link
Collaborator

@gante Tbh, I'm not sure. @ArthurZucker is the best person to ping here, as he wrote the docs

@ArthurZucker
Copy link
Collaborator

Let's go on slack @gante !

@gante
Copy link
Member Author

gante commented Jun 28, 2024

@amyeroberts the dev-ci commit ran against the :dev docker images, which stem from the latest commit here. The failed test is unrelated and reported on slack, the TF issues are fixed as it can be seen :)

If you approve these changes, after the hub failures are resolved:

  1. I will add a commit [push-ci-image] to the other PR, which will push the updated docker images. After the CI on this commit runs, all models will run their CI with that docker image, fixing the issues on CI.
  2. merge this PR, which fixes the issue on local installs and ensures future docker image updates include this change.
  3. Update the CI docker docs with the learnings in this process, to make them more clear to everyone :)

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!

Copy link
Member

@Rocketknight1 Rocketknight1 left a comment

Choose a reason for hiding this comment

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

Hey @amyeroberts, we currently don't have a transition plan to move Transformers to Keras 3 because of how disruptive it would be to backwards compatibility. A lot of our TF/Keras usage is users who use it for production workflows with older software versions, so we think it'll be quite a while before we're ready to deprecate all of that and migrate to Keras 3.

For the forseeable future, we're going to keep Transformers on Keras 2, and support Keras 3 by having libraries like KerasNLP and KerasCV pick up the slack and save/load safetensors checkpoints that Transformers can read. This has already started thanks to @ariG23498's work, and more architectures will be supported very soon.

Therefore, it's totally okay to pin Keras and Keras-associated libraries on the last version that supported Keras 2, and that pin will probably be maintained indefinitely.

@julien-c
Copy link
Member

(btw cc @Wauplin for viz)

@amyeroberts
Copy link
Collaborator

Thanks for the detailed explanation @Rocketknight1 !

@Wauplin
Copy link
Contributor

Wauplin commented Jun 28, 2024

Thanks for the ping, good to keep in mind. And thanks @Rocketknight1 for the full context!

Copy link
Contributor

@fxmarty fxmarty left a comment

Choose a reason for hiding this comment

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

LGTM

@gante
Copy link
Member Author

gante commented Jul 1, 2024

Docker image pushed in #31687 -- merging 🤞

@gante gante merged commit 3345ae7 into huggingface:main Jul 1, 2024
22 checks passed
@gante gante deleted the keras_nlp_pin branch July 1, 2024 16:39
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

8 participants