-
Notifications
You must be signed in to change notification settings - Fork 9.6k
CI does not test examples with latest pytorch #1329
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
Comments
Hi @dvrogozh I think this sounds very reasonable, we could potentially just create a new venv per example, so insisting on a requirements.txt per folder sounds great, we could explore either regular venv or uv or really anything you think is most convenient I'm open to designs and PR ideas here, probably won't have time to author myself but happy to shepherd PRs |
@msaroufim : thank you for support. I will be glad to suggest some PRs. Would be nice to agree on direction we want to take before I will go ahead with implementation. Couple things to decide upon:
I think per-example makes more sense since likely usage is git-clone/cd-to-example/run-example which is per-example approach. This however gives some complexity to implement CI side since we need to care for each example.
On technical level this implies other question: do we need run_python_examples.sh and other such scripts? From one side they are convenient to run all the examples locally. From other side they complicate managing clean environment for each sample. Potentially we can:
I personally like 1st option (drop |
I agree that per example requirements.txt makes sense so there's no implicit downgrade or upgrade I do think separate venv still makes the most sense to be honest and with uv its easier because you can put the venv in the same folder as code As far as run_python_examples.sh goes, that file is not my favorite the key requirements though are
So for example if the local instructions are
Then the CI job can for loop over all the directories in the github workflow |
@msaroufim : ok, I've implemented something which we can review and discuss: |
After merging #1330, the remainder task is:
For few examples this might be as simple as unpin versions and revalidating with latest packages:
For few examples, however, this means updating example scripts as they rely on some features from
|
With: 8393ceb
This issue is a follow up on #1327 (comment). We've tried to update
fast_neural_style
example which required bumping up pytorch version and spotted few issues. Findings are around the fact that CI scripts run bulk install of dependencies for all examples at once. See:examples/utils.sh
Lines 26 to 31 in 8393ceb
This causes downgrade of nightly torch installed by the CI:
examples/.github/workflows/main_python.yml
Line 28 in 8393ceb
torchvision
version. For example here:examples/dcgan/requirements.txt
Line 2 in 8393ceb
I suggest to consider the following improvements for pytorch examples:
requirements.txt
(some miss it asfast_neural_style
does), Respect each example requirements and use uv #1330requirements.txt
, Respect each example requirements and use uv #1330torch
,torchvision
, etc.) unless as a workaround to specific issues (which must be explicitly noted) or due to example deprecation with the future dropAlternatively, we can consider that (UPDATE: we've dismissed this after discussion):
CC: @malfet, @atalman, @msaroufim
The text was updated successfully, but these errors were encountered: