ci: run smoke tests across a python x torch version matrix#32
ci: run smoke tests across a python x torch version matrix#32guru-desh wants to merge 8 commits into
Conversation
9182e1a to
147cf24
Compare
Please update the branch protection rule for the 
|
There was a problem hiding this comment.
If we update to torch 2.12 in the future for the version upper bound, will we need to manually edit every place where torch_2_11 is used? I think using highest_torch as an alias would be a better choice for maintainability.
There was a problem hiding this comment.
If we update to torch 2.12 in the future for the version upper bound, will we need to manually edit every place where torch_2_11 is used?
No we would just edit @TORCH_GROUP=torch_2_11 in env-highest-torch
The TORCH_GROUP ?= torch_2_11 doesn't need to be changed. It just represents what the default make env's torch version becomes. This doesn't necessarily correspond always to highest torch version
There was a problem hiding this comment.
I think using highest_torch as an alias would be a better choice for maintainability.
We are using TORCH_GROUP as the single source of truth for the torch version and then using the make commands to edit TORCH_GROUP as needed. Are you saying we should have different environment variables?
There was a problem hiding this comment.
When specifying TORCH_GROUP, we should use an alias instead of a specific version like torch_2_11. This way, we avoid setting torch_2_11 everywhere in the code. For example, instead of using torch_2_11 directly, we can use highest_torch.
There was a problem hiding this comment.
The TORCH_GROUP ?= torch_2_11 doesn't need to be changed. It just represents what the default make env's torch version becomes.
My understanding is that we default to the highest supported version. In this case, we're already using the latest version we support, correct?
There was a problem hiding this comment.
For example, I think all places where we call:
# torch_lowest is set to torch_2_8
# torch_highest is set to torch_2_11
TORCH_GROUP=torch_2_8 -> TORCH_GROUP=torch_lowest
TORCH_GROUP=torch_2_11 -> TORCH_GROUP=torch_highest
torch_2_x only remains as implementation details.
There was a problem hiding this comment.
When specifying TORCH_GROUP, we should use an alias instead of a specific version like torch_2_11. This way, we avoid setting torch_2_11 everywhere in the code.
I see what you are saying, but we only set TORCH_GROUP=torch_2_11 in one place which is during venv creation. Are there plans to add code such that we would need to find the highest supported torch version and would then need a torch_lowest and torch_highest variable?
My understanding is that we default to the highest supported version. In this case, we're already using the latest version we support, correct?
We currently default to the highest supported version due to uv dependency resolution not due to something we set.
torch_2_x only remains as implementation details.
To do this:
# torch_lowest is set to torch_2_8
# torch_highest is set to torch_2_11
TORCH_GROUP=torch_2_8 -> TORCH_GROUP=torch_lowest
TORCH_GROUP=torch_2_11 -> TORCH_GROUP=torch_highest
We would need to set environment variables to define torch_lowest and torch_highest. This means this would turn into the following:
torch_lowest=torch_2_8
torch_highest=torch_2_11
TORCH_GROUP=torch_highest
env-lowest-torch:
@TORCH_GROUP=torch_lowest $(call use_env,VENV_LOWEST_TORCH) && \
...
dengqiaoyu
left a comment
dengqiaoyu
left a comment
There was a problem hiding this comment.
Please remember to update the status check branch protection rules when you are ready to merge the PR.
Thanks for cleaning up.
Smoke tests previously ran on one implicit torch version (whatever pyproject.toml's unpinned bound resolved to) across all supported python versions. Add torch_2_8/2_9/2_10/2_11 dependency groups (build-matched torchao/torchvision per pytorch/ao issue 2919) and a matrix.torch_group axis on the test-smoke CI job so each of the 4 torch minor versions is smoke tested independently, still covering all 3 python versions per job via the existing nox session. lowest_tested_torch/highest_tested_torch are renamed to torch_2_8/ torch_2_11 for a consistent naming scheme; the linux-tests/macos-tests full-suite jobs and their Makefile targets are unchanged.
setup_env.sh's group discovery used grep -E '^[a-z_-]+ = \[', which excludes digits and made torch_2_8/2_9/2_10/2_11 invisible to validate_groups — breaking test-highest-pytorch/test-lowest-pytorch with 'Unknown dependency group', caught by the fork CI run.
setup_env.sh now reads TORCH_GROUP directly and folds it into every sync it runs, so make env/test/test-fast/docs/etc. all pin torch instead of floating on whatever pyproject.toml's plain bound resolves to. test-highest-pytorch/test-lowest-pytorch/env-highest-torch lock their group via an inline `export TORCH_GROUP=... &&` at the top of the recipe (rather than a hardcoded --with-torch_2_X flag), so their name stays a promise regardless of a command-line override — verified that GNU Make's own `override` directive achieves the same lock but trips up the mbake Makefile formatter's duplicate-target detector, which doesn't recognize that syntax. --with-torch_2_X stays as a guarded alternate path for direct, non-Make invocations of setup_env.sh; a guard now errors clearly if it disagrees with an explicitly-set TORCH_GROUP instead of surfacing a raw uv conflicting-groups resolver error.
…ORCH_GROUP Now that the Makefile exports TORCH_GROUP unconditionally, the smoke-test-specific SMOKE_TEST_TORCH_GROUP env var was redundant indirection. ci/nox/noxfile.py reads TORCH_GROUP directly, and `make test-smoke` no longer needs to translate the Makefile variable into a differently-named env var for the nox subprocess.
The custom bash check that detected TORCH_GROUP disagreeing with an explicit --with-torch_2_X flag duplicated logic uv already provides: `uv sync --group torch_2_11 --group torch_2_8` fails on its own with a clear "Groups ... are incompatible with the conflicts: ..." error and non-zero exit. Simplifies the --all-groups exclusion loop and the plain --group append accordingly, removing the GROUP_REQUESTED/ TORCH_GROUP_ALREADY_SYNCED bookkeeping that only existed to feed it.
The Makefile always exports TORCH_GROUP before invoking nox, so the Python-side default was dead code for every make test-smoke run; kept only for a bare, non-Make nox invocation which isn't a supported entrypoint.
The expanded rationale duplicated what's now self-evident from reading the three lock-target recipes directly.
`export TORCH_GROUP=torch_2_X && ...` set the variable for the whole recipe's shell, which happened to be harmless since nothing else in these three recipes reads TORCH_GROUP, but the intent was always to scope the override to just the one setup_env.sh invocation that needs it. Using `TORCH_GROUP=torch_2_X <command>` as an inline prefix does that precisely: it applies only to the first command in the `&&` chain, reverting to the Makefile's own default/command-line value for everything after.
147cf24 to
833f0c5
Compare
3 participants
Description
Smoke tests previously ran on one implicit torch version (whatever pyproject.toml's unpinned bound resolved to) across all supported python versions. Add
torch_2_8/2_9/2_10/2_11dependency groups (build-matched torchao/torchvision per pytorch/ao issue 2919) and amatrix.torch_groupaxis on the test-smoke CI job so each of the 4 torch minor versions is smoke tested independently, still covering all 3 python versions per job via the existing nox session.Adding these new smoke tests doesn't change the total CI time. Because provisioning is fast and we have more Linux machines, each
test-smokerun takes 6 minutes.make test-tutorialtakes 11 minutes, so stage 1 CI will bemax(test-smoke-torch-2.8, test-smoke-2.9, ..., test-tutorials). Sincetest-tutorialsis 11 minutes, stage 1 CI takes 11 minutes, and adding moretest-smokewouldn't affect total CI time.