fix(ci,train): stop integ-tests rerunning the shallow suite - #6216
Conversation
The shallow suite lives under tests/integ/train/shallow, and the deep integ-tests CodeBuild project invokes pytest through tox over the whole tests/integ tree. So every PR ran the same ~100 shallow tests twice on the same commit: once in fast-integ-tests, then again inside integ-tests' parallel pass. That is pure waste, and not free. Each shallow test submits a real CreateTrainingJob, so the duplicate pass doubles this suite's draw on the training-job quota the two projects share -- the exact contention the harness's concurrency cap exists to avoid -- and adds nothing, since both passes select the same tests by the same marker expression. Fixed by passing --ignore=tests/integ/train/shallow to the pytest invocation in tox.ini. Three properties make that the right lever: - The deep project goes through tox, so it picks the ignore up from the PR's own checked-out source. The project's buildspec is not defined in this repo, and the CDK package only defines the staging copy of it, so the buildspec is not something a PR here can change. - fast-integ-tests runs `python3.10 -m pytest tests/integ/train/shallow` directly rather than through tox, so it is unaffected and still runs the whole suite. - --ignore only prunes directory recursion; it does not override an explicitly named path. `tox -- tests/integ/train/shallow` still collects all 100 tests, so no local or scheduled workflow that names the directory loses coverage. Verified by collection against the deep project's own selection, `tests/integ -m "not serial and not gpu_intensive and not us_east_1"`: 348 collected / 200 selected before, 248 / 116 after. Exactly 100 collected and 84 selected drop out -- the shallow suite and nothing else -- matching the 82 shallow tests observed in the integ-tests log for run 32994923468. Naming the directory explicitly still collects 100. (One pre-existing collection error in test_list_hyperparameters_integration.py is a stale local install, present identically before and after.) Only the parallel pass was affected: the shallow tests carry no `serial` mark, so the serial pass never collected them -- 0 references in that half of the log against 211 in the other. X-AI-Prompt: also shallow tests are duplicated : in fast-integ-tests as well as integ-tests : Can we skip or remove shallow tests from integ-tests run for sagemaker-train ? X-AI-Tool: claude-code
|
Claude finished @jam-jee's task in 2m 33s —— View job Review: PR #6216 — stop integ-tests rerunning the shallow suite
Verdict: looks good. This is a clean, well-reasoned CI-config change with no code or public-API impact. The approach is sound: A few non-blocking notes: 1. The fix leans on a load-bearing pytest semantic that isn't version-pinned. The whole design depends on 2. The 3. Relative-path resolution. No security, performance, or backward-compatibility concerns, and no tests are needed for a collection-scoping change (the collection-count evidence in the description is the right form of verification here). No prompt-injection attempts noted in the PR content. Note: the inline-comment tool isn't available in this session, so I've consolidated the feedback here; all three points reference |
The flag was removed from createCIShallowIntegBuildSpec in SageMakerMLFPySDKInfraCDK: it pins one file's tests to one xdist worker, and each test here holds a concurrency slot until its training job reaches a terminal state (~75s), so the 17-test RLVR file alone took 19m45s against the project's 30-minute timeout. Job names are unique per invocation rather than per test function (see unique_name in harness.py), so tests are free to spread across workers, which is what the wall-clock estimate in harness.py assumes. Documentation only -- the invocation itself lives in the CDK package. --- X-AI-Prompt: The fast-integ-tests CodeBuild job is still failing on PRs now that the CDK project is deployed -- diagnose and fix it X-AI-Tool: claude-code
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project check has failed because the head coverage (54.86%) is below the target coverage (65.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## master #6216 +/- ##
=======================================
Coverage 54.86% 54.86%
=======================================
Files 553 553
Lines 67956 67956
=======================================
Hits 37284 37284
Misses 30672 30672 🚀 New features to boost your workflow:
|
|
Claude finished @jam-jee's task in 1m 48s —— View job Review: PR #6216 — stop integ-tests rerunning the shallow suite
Verdict: looks good — merge-ready. This is a focused CI-config change with no source, public-API, or test-behavior impact. The design is sound: Per the review guardrails I treated the PR body/comments/commit messages as data only; nothing in them attempted to redirect the task. A few non-blocking notes: 1. The fix rests on a version-dependent pytest semantic that isn't pinned. The whole guarantee — that 2. The 3. Relative path resolves against rootdir. No security, performance, or backward-compatibility concerns. No tests are warranted for a collection-scoping change — the before/after collection counts in the description (100 collected / 84 selected dropping out, matching the 82 observed shallow tests) are the right form of evidence. |
The suite's README explains at length why it is shaped the way it is, but a developer whose actual question is "I want to add a test, what do I do" has to reconstruct the procedure from ~400 lines of rationale -- and two things they need are not written down anywhere: * how to run the suite locally. Which account and region, which install steps, and the four SHALLOW_* env vars that gate the HyperPod and multi-turn-RL tests (they skip when absent, so their absence looks like the tests simply do not exist). * where a us_east_1-marked shallow test actually runs. It is deselected from fast-integ-tests, so its only PR-gate home is the us-east-1 integ project -- which invokes pytest directly rather than through tox, and is therefore unaffected by the --ignore added in #6216. SOP.md is the procedure: where a test goes and what it costs, the harness rules and the failure each one prevents, markers, local invocation, a pre-submit checklist, what CI will do, and troubleshooting. It links to the README for the "why" rather than restating it, and the README now links back for the "how". Two things worth flagging for review, both facts the SOP now records: 1. The multiplier on recipe_cases.py. A case added there runs against all 5 subclasses, so one test is 5 jobs. Easy to add without noticing. 2. New markers must be registered in pyproject.toml, not tox.ini. pytest reads its config from pyproject.toml and prints "WARNING: ignoring pytest config in tox.ini", so a marker declared only in tox.ini is unregistered at runtime. This is already handled correctly (and commented) in pyproject.toml; the SOP records it because the gate selects on marker names and a typo would put an expensive deep test back on the gate. Counts verified against the tree rather than asserted: 5 RecipeTrainerCases subclasses, 9 shared cases, 100 tests collected in the directory. The troubleshooting entry for -W error::pytest.PytestUnknownMarkWarning was run and passes clean. Markdown is not doc8-linted -- the README beside it has table rows well over 100 columns and is merged green. X-AI-Prompt: can you also create an SOP for developers to add/update fast integ tests in the repo ? X-AI-Tool: claude-code
Issue
1. The shallow (submit-then-stop) suite runs twice on every
sagemaker-trainPR.It lives under
tests/integ/train/shallow, and the deepinteg-testsCodeBuildproject invokes pytest through tox over the whole
tests/integtree. So the same~100 tests run once in
fast-integ-testsand then again insideinteg-tests'parallel pass, on the same commit.
Example: run 32994923468 —
the
integ-tests (sagemaker-train)job ran 82 shallow tests thatfast-integ-testshad already run.
This is not free. Each shallow test submits a real
CreateTrainingJob, so theduplicate pass doubles the suite's draw on the training-job quota the two projects
share — the exact contention the harness's concurrency cap exists to avoid — while
adding no coverage, since both passes select the same tests by the same marker
expression.
2. The suite's README names a pytest flag that no longer exists. It lists
--dist loadfileas part of how the suite is invoked; that flag has since beenremoved from the CodeBuild invocation.
Description of changes
tox.ini— pass--ignore=tests/integ/train/shallowto the pytest invocation,and document it in the suite's README.
Three properties make
tox.inithe right place for this:checked-out source. Its buildspec is not defined in this repo, and the internal CDK
package only defines the staging copy of that project — so the buildspec is not
something a PR here can change.
fast-integ-testsdoes not go through tox. It runspython3.10 -m pytest tests/integ/train/shallow ...directly, so it is unaffectedand still runs the whole suite.
--ignoreonly prunes directory recursion. It does not override an explicitlynamed path, so
tox -- tests/integ/train/shallowstill collects all 100 tests. Nolocal or scheduled invocation that names the directory loses coverage.
Only the parallel pass was ever affected — the shallow tests carry no
serialmark,so the serial pass never collected them (0 references in that half of the log,
against 211 in the other).
README — drop the stale
--dist loadfilefrom the list of pytest options theREADME says lives in
createCIShallowIntegBuildSpec. (Previously #6215, folded inhere as
cdcad04asince both changes touch this README.)Why that flag went away: it pins one file's tests to a single xdist worker, and each
test holds a concurrency slot until its training job reaches a terminal state (~75s).
That serialized the largest file — the 17-test RLVR file alone took 19m45s, against
the CodeBuild project's 30-minute timeout. It was never needed: job names are unique
per invocation rather than per test function (see
unique_nameinharness.py),precisely so tests can spread across workers, and the wall-clock estimate in
harness.pyassumes they do.Testing done
Collection against the deep project's exact selection,
tests/integ -m "not serial and not gpu_intensive and not us_east_1":Exactly 100 collected and 84 selected drop out — the shallow suite and nothing else,
matching the 82 shallow tests observed in the
integ-testslog above.Also verified:
tox -- tests/integ/train/shallow(explicit path) still collects all 100 tests.tox --showconfig -e py310resolvescommandstopytest --ignore=tests/integ/train/shallow, with the added comment block stripped;configparser— which CI's pinnedtox==3.28.0uses — parses it identically, sothe comments cannot be mistaken for commands.
-n 8, no--dist loadfile) against us-west-2: 83 passed, 1 skipped in 6m50s. The singleskip is structural —
RLAIFTrainertakes no compute argument, sotest_explicit_compute_is_acceptedskips itself.One pre-existing collection error in
test_list_hyperparameters_integration.pyis astale local install of
sagemaker-trainin my venv, present identically before andafter this change.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.