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

Install the tensorflow example requirements in docker #31428

Merged

Conversation

amyeroberts
Copy link
Collaborator

What does this PR do?

Unlike the pytorch examples here the docker file used to run the tensorflow examples doesn't install the requirements from the examples requirements file.

Recently, we had to pin the datasets version used for the examples #31417, but this wasn't propogated for tensorflow because of this omisison.

This means added requirements won't be included, and is currently causing failing tests on main: https://app.circleci.com/pipelines/github/huggingface/transformers/95698/workflows/52a112da-0d84-4569-8f69-ca180f4c7b2a/jobs/1260731

@amyeroberts amyeroberts requested a review from ydshieh June 14, 2024 17:31
@gante
Copy link
Member

gante commented Jun 14, 2024

@amyeroberts There are PT example failures too, possibly related (e.g see here, from this PR)

@amyeroberts
Copy link
Collaborator Author

amyeroberts commented Jun 14, 2024

@gante Thanks for flagging!

I think what's happening is that datasets is already installed with the pip install . command and so not updated to reflect the constraints here. I'll play about and see if I can figure out what's happening

Scratch all that. If we're installing from the library's setup.py then the datasets version should still be limited, and my understanding is that if we're installing with pip install . datasets shouldn't be installed at all. Which brings into question how the TF examples were running in the first place....

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

amyeroberts commented Jun 14, 2024

@gante Are you sure the linked PR is from the latest main? If I look at recent runs for examples_torch under the installation step -- uv venv && uv pip install . && uv pip install -r examples/pytorch/_tests_requirements.txt -- then you see datasets downgraded to 2.19.2, whereas I don't see that for that PR.

@ydshieh
Copy link
Collaborator

ydshieh commented Jun 14, 2024

It makes examples_tf_job equivalent examples_torch_job regarding + the logic makes sense to me.

Thank you!

@amyeroberts amyeroberts merged commit 3d0bd86 into huggingface:main Jun 14, 2024
22 checks passed
@amyeroberts amyeroberts deleted the fix-tensorflow-examples-docker branch June 14, 2024 18:35
Copy link
Collaborator

@ArthurZucker ArthurZucker left a comment

Choose a reason for hiding this comment

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

I'll open a fix for both next week, but we should not allow such quick fixes anymore given that the process to update the docker image is super straigforward now

@@ -326,7 +326,7 @@ def job_name(self):
"examples_tensorflow",
cache_name="tensorflow_examples",
docker_image=[{"image":"huggingface/transformers-examples-tf"}],
install_steps=["uv venv && uv pip install ."],
install_steps=["uv venv && uv pip install . && uv pip install -r examples/tensorflow/_tests_requirements.txt"],
Copy link
Collaborator

Choose a reason for hiding this comment

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

Mmmm again, that is not what we should do. The CI is gonna get slower because we just need the docker that is built to run this.

@amyeroberts
Copy link
Collaborator Author

should not allow such quick fixes anymore given that the process to update the docker image is super straigforward now

@ArthurZucker I think "not allow" is very strong here for fixing something which would essentially be required anyway: regardless of upstream changes to the docker images (and how easy that is to implement) the commands for setting up our examples runs were not consistent. So, either, we need to have inconsistent docker images for the different frameworks (harder to maintain) or the pytorch command would need to be updated to remove the requirements install

@gante
Copy link
Member

gante commented Jun 17, 2024

@gante Are you sure the linked PR is from the latest main?

@amyeroberts possibly not, can't confirm 👼 In any case, rebasing to the latest main has sorted it, thanks 🙏

@ArthurZucker
Copy link
Collaborator

We already have different docker images for different frameworks, and docker image exist for example_tensorflow and example_torch. So I think we do need to update this but the idea is to make sure we don't install anything in the CIs

@ydshieh
Copy link
Collaborator

ydshieh commented Jun 18, 2024

Make sense. Sorry I didn't think of this aspect. We can definitely take the requirements in examples into account in the docker image build time.

@amyeroberts
Copy link
Collaborator Author

We already have different docker images for different frameworks, and docker image exist for example_tensorflow and example_torch

Yes, I'd expect the docker images to be different. After all, we need one with a tensorflow environment and one for torch. Their overall setup should be consistent though, and the errors which were being experienced on the CI highlighted that they weren't (the pytorch one was reliant on the requirements installs, whereas TF wasn't).

Once the docker images have been updated, we can remove the installs here. And, I'm assuming the _test_requirements.txt files?

@ydshieh
Copy link
Collaborator

ydshieh commented Jun 18, 2024

Once the docker images have been updated, we can remove the installs here.
yeah!

And, I'm assuming the _test_requirements.txt files?

I guess those are better to be kept even if we update the docker file, although I don't feel strong (as it means there are duplication and we have to keep them synced.).

Or better, if we can use _test_requirements.txt directly in the docker file (not to repeat the versions and only keep one source of information)

@amyeroberts
Copy link
Collaborator Author

Or better, if we can use _test_requirements.txt directly in the docker file (not to repeat the versions and only keep one source of information)

Yes please :)

@ArthurZucker
Copy link
Collaborator

transformers/setup.py

Lines 466 to 479 in 682f221

extras["tests_torch"] = deps_list()
extras["tests_tf"] = deps_list()
extras["tests_flax"] = deps_list()
extras["tests_torch_and_tf"] = deps_list()
extras["tests_torch_and_flax"] = deps_list()
extras["tests_hub"] = deps_list()
extras["tests_pipelines_torch"] = deps_list()
extras["tests_pipelines_tf"] = deps_list()
extras["tests_onnx"] = deps_list()
extras["tests_examples_torch"] = deps_list()
extras["tests_examples_tf"] = deps_list()
extras["tests_custom_tokenizers"] = deps_list()
extras["tests_exotic_models"] = deps_list()
extras["consistency"] = deps_list()

should be what we are looking for! Either this or the requirement but yes everything should be in the docker build

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.

5 participants