Skip to content

Commit eb0f09e

Browse files
jam-jeejam-jee
authored
docs(train): add an SOP for adding/updating shallow integ tests (#6219)
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
1 parent 6f431c9 commit eb0f09e

2 files changed

Lines changed: 225 additions & 0 deletions

File tree

‎sagemaker-train/tests/integ/train/shallow/README.md‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,9 @@
11
# Shallow (submit-then-stop) integration tests
22

3+
> **Adding or changing a test? Follow [SOP.md](./SOP.md).** This README explains why
4+
> the suite is shaped the way it is; the SOP is the step-by-step procedure, including
5+
> how to run the suite locally.
6+
37
These tests add fast acceptance coverage on the PR gate. They run in their own
48
`fast-integ-tests` job, **alongside** the existing `integ-tests` CodeBuild suite,
59
which is unchanged. The deep suites still run on the scheduled CI-health workflows.
@@ -375,6 +379,9 @@ capped by default instead of by remembering to opt in.
375379

376380
## Writing a new test
377381

382+
The harness API, for reference. For the full procedure — where the test goes, markers,
383+
running it locally, the pre-submit checklist — see [SOP.md](./SOP.md).
384+
378385
Use the harness; do not call `trainer.train()` directly.
379386

380387
```python

‎sagemaker-train/tests/integ/train/shallow/SOP.md‎

Lines changed: 218 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,218 @@
1+
# SOP — adding or updating a fast (shallow) integ test
2+
3+
The step-by-step procedure for changing anything under
4+
`sagemaker-train/tests/integ/train/shallow`. [`README.md`](./README.md) explains *why*
5+
the suite is shaped the way it is; this file is what to do, in what order, and how to
6+
verify each step.
7+
8+
## 0. Does your test belong here?
9+
10+
This suite submits a real `CreateTrainingJob`, asserts the service returned an ARN,
11+
and immediately stops the job. A test belongs here if and only if what it checks is
12+
decided **synchronously, at submit time** — payload shape, validation, permissions,
13+
image/recipe resolution.
14+
15+
| You want to assert… | Put it in |
16+
|---|---|
17+
| the service accepted (or rejected) a payload | **here** |
18+
| client-side config assembly, with no service call | `tests/integ/train/test_dry_run_integration.py` |
19+
| artifacts, metrics, logs, convergence — anything needing the job to *run* | the deep suite, marked `gpu_intensive` |
20+
21+
A regression that makes training itself fail will still pass here. That is deliberate;
22+
see *What these tests deliberately do NOT cover* in the README.
23+
24+
## 1. Decide where the test goes
25+
26+
| Case | Action | Cost |
27+
|---|---|---|
28+
| New behaviour on one trainer | Add to the matching `test_<trainer>.py` | 1 job |
29+
| Behaviour shared by **all** recipe trainers | Add a case to `recipe_cases.py` | **1 job × every subclass** — currently 5 (SFT, DPO, RLVR, RLAIF, CPT) |
30+
| A brand-new trainer | New `test_<trainer>.py` subclassing `RecipeTrainerCases` | the 9 shared cases |
31+
32+
Adding to `recipe_cases.py` multiplies. Only put a case there if it is genuinely
33+
trainer-independent; otherwise it belongs in one file.
34+
35+
A new trainer file is two lines plus opt-outs:
36+
37+
```python
38+
class TestMyTrainer(RecipeTrainerCases):
39+
TRAINER = MyTrainer
40+
EXTRA_KWARGS = {...} # required constructor args, if any
41+
SUPPORTS_SERVERFUL = False # trainer takes no `compute`
42+
SUPPORTS_TRAINING_TYPE = False # no LoRA/full distinction
43+
```
44+
45+
**Verify:** `pytest tests/integ/train/shallow --collect-only` lists your test(s) and
46+
the total moved by the number you expect.
47+
48+
## 2. Write it with the harness
49+
50+
Never call `trainer.train()` directly — the harness is what forces `wait=False`,
51+
stops the job in a `finally`, and holds the concurrency slot.
52+
53+
```python
54+
from .harness import assert_submitted, assert_rejected, submitted, unique_name
55+
56+
def test_my_feature_is_accepted(sagemaker_session, train_data_uri):
57+
trainer = _trainer(sagemaker_session, unique_name("shallow-my-feature"), ...)
58+
with submitted(trainer) as job:
59+
assert_submitted(job)
60+
61+
def test_bad_input_is_rejected(sagemaker_session):
62+
assert_rejected(trainer, ("does not exist", "ValidationException"))
63+
```
64+
65+
Rules that are not negotiable, each with the failure it prevents:
66+
67+
| Rule | Why |
68+
|---|---|
69+
| Use `submitted()` / `assert_rejected()`, never bare `train()` | Skips the stop, the slot, and the terminal-wait |
70+
| Never pass `wait=` | Rejected with `TypeError`, so a copy-pasted `wait=True` cannot reintroduce a real training run |
71+
| Never set `keep_alive_period_in_seconds` | A warm pool outlives the stop and keeps instances provisioned |
72+
| Smallest instance that exercises the path (`ml.m5.large` unless a recipe needs an accelerator) | Validation is instance-type agnostic; big instances cost real money on a non-deterministic teardown |
73+
| Resolve images with `harness.cpu_image(sagemaker_session)` | The DLC registry account differs by partition; a hardcoded URI is unusable outside one |
74+
| Derive S3 paths from `default_bucket()`, never hardcode a bucket | A hardcoded bucket means the test only passes in one account — how five deep Nova tests ended up never running |
75+
| Look resources up and `skip` when absent; never create them | Creating IAM roles/Lambdas/MLflow apps is a durable side effect a PR-gate suite must not have |
76+
| Keep at least one **negative** test per feature area | Without them the suite degenerates into "any ARN is fine" and stays green on a permissive-but-wrong payload |
77+
78+
**Verify:** `grep -n "wait=\|keep_alive" <your file>` returns nothing.
79+
80+
## 3. Markers
81+
82+
| Marker | Effect on your shallow test |
83+
|---|---|
84+
| *(none)* | Runs on the PR gate in `fast-integ-tests`. **This is what you want.** |
85+
| `us_east_1` | Removed from `fast-integ-tests`; runs in the `integ-tests-us-east-1` job instead. Needs us-east-1 test-account credentials. Use only for Nova. |
86+
| `serial` | No effect here (the fast project runs one pytest command), but it *does* split the deep and us-east-1 projects. Don't add it. |
87+
| `gpu_intensive` | **Never on a shallow test.** It marks deep tests *off* the gate. |
88+
89+
Two rules that cut both ways:
90+
91+
* **Do not add `gpu_intensive` to a deep test unless a shallow test covers the same
92+
path** — the gate silently loses coverage. Update the coverage table in the README
93+
when you do.
94+
* **Register any new marker in `pyproject.toml`, not `tox.ini`.** pytest reads its
95+
config from `pyproject.toml` and prints
96+
`WARNING: ignoring pytest config in tox.ini`, so a marker declared only in
97+
`tox.ini` is unregistered at runtime. That matters because the gate selects with
98+
`-m "not gpu_intensive and not us_east_1"`: a typo'd name would put an expensive
99+
deep test back on the gate instead of erroring.
100+
101+
**Verify:** run with `-W error::pytest.PytestUnknownMarkWarning`; an unregistered
102+
marker fails instead of warning.
103+
104+
## 4. Adding a new submission path? Take slots yourself
105+
106+
The concurrency cap is enforced inside `submitted()` and `assert_rejected()`, so a
107+
normal test is capped automatically. If you add a path that submits **without** going
108+
through those two — as `_tuning()` in `test_tuner.py` does, because a tuning job is
109+
stopped via `tuner.stop_tuning_job()` — you must acquire `job_slots()` yourself and
110+
hold them until the job is **terminal**, not until `stop()` returns.
111+
112+
Getting this wrong is the bug that made an earlier version peak at ~37 concurrent jobs
113+
against a limit of 20. See *Concurrency cap* in the README.
114+
115+
## 5. Run it locally
116+
117+
**Prerequisites**
118+
119+
* AWS credentials for an SDK test account. The suite uses ambient credentials — no
120+
profile logic — and resolves the execution role through the real discovery path
121+
(`TrainDefaults.get_role(role=None, ...)`), so the account needs a discoverable
122+
SageMaker execution role.
123+
* Region defaults to **us-west-2**; an autouse fixture pins `AWS_DEFAULT_REGION`
124+
unless you set it yourself.
125+
* Recipe resolution goes through a private hub named `sdktest` (an autouse fixture
126+
sets `SAGEMAKER_HUB_NAME`), so the account needs it.
127+
* `us_east_1` tests need credentials in the us-east-1 test account; their fixture
128+
pins the region regardless of `AWS_DEFAULT_REGION`.
129+
130+
**Install, the same way CI does**
131+
132+
```bash
133+
cd sagemaker-core && pip install -e '.[test]'
134+
cd ../sagemaker-train && pip install -e '.[test]'
135+
```
136+
137+
**Run**
138+
139+
```bash
140+
# one test, no cap, no xdist — for debugging
141+
SHALLOW_MAX_CONCURRENT_JOBS=0 python -m pytest \
142+
tests/integ/train/shallow/test_sft_trainer.py -k my_feature
143+
144+
# one file, gate selection
145+
python -m pytest tests/integ/train/shallow/test_sft_trainer.py \
146+
-m "not gpu_intensive and not us_east_1"
147+
148+
# the whole suite exactly as the PR gate runs it
149+
python -m pytest tests/integ/train/shallow -v -n 8 \
150+
-m "not gpu_intensive and not us_east_1" --durations 15
151+
```
152+
153+
Expect ~7 minutes for the full suite at `-n 8`, and ~1–2 minutes for a single test —
154+
dominated by the wait for the job to reach a terminal state, not by the SDK.
155+
156+
**Optional env vars.** Tests whose inputs are not derivable read them from the
157+
environment and **skip** when absent, so they never fail for a missing resource:
158+
159+
| Variable | Gates | Default |
160+
|---|---|---|
161+
| `SHALLOW_MAX_CONCURRENT_JOBS` | Concurrency cap; `0` disables gating | 10 |
162+
| `SHALLOW_HYPERPOD_CLUSTER` | `test_cpt_trainer.py` HyperPod cases | skip |
163+
| `SHALLOW_MTRL_AGENT_ENV`, `SHALLOW_MTRL_MLFLOW_APP_ARN`, `SHALLOW_MTRL_DATASET` | `test_multi_turn_rl_trainer.py` | skip |
164+
| `SHALLOW_MTRL_MODEL` | Multi-turn RL model id | `mock-oss-test` |
165+
166+
## 6. Pre-submit checklist
167+
168+
```bash
169+
tox -e black-format && tox -e flake8 && tox -e pylint # from sagemaker-train/
170+
```
171+
172+
- [ ] Collection count moved by exactly what you expect (step 1).
173+
- [ ] At least one negative test for the feature area (step 2).
174+
- [ ] No `wait=`, no `keep_alive_period_in_seconds`, no hardcoded bucket/URI/ARN.
175+
- [ ] Your test is unmarked unless it is genuinely us-east-1-only (step 3).
176+
- [ ] If you marked a deep test `gpu_intensive`, the README coverage table accounts
177+
for it.
178+
- [ ] The suite still passes locally at `-n 8`.
179+
180+
## 7. What CI will do
181+
182+
| Job | Project | Runs |
183+
|---|---|---|
184+
| `fast-integ-tests` | `…-ci-sagemaker-train-fast-integ-tests` | `python3.10 -m pytest tests/integ/train/shallow -v -n 8 -m "not gpu_intensive and not us_east_1" --durations 15`, `SHALLOW_MAX_CONCURRENT_JOBS=10`, 30-min timeout |
185+
| `integ-tests-us-east-1` | `…-ci-integ-tests-us-east-1` | `pytest tests/integ -m "us_east_1 and not gpu_intensive …"` — where your `us_east_1` shallow tests run |
186+
| `integ-tests` | `…-ci-sagemaker-train-integ-tests` | The deep suite. Goes through tox, and `tox.ini` passes `--ignore=tests/integ/train/shallow`, so it does **not** rerun this suite |
187+
188+
Two things worth knowing:
189+
190+
* `fast-integ-tests` only fires when `sagemaker-train` is in the change set. A
191+
docs-only or CDK-only change will not exercise your test.
192+
* Neither job checks out PR code onto the GitHub runner — both start CodeBuild with
193+
`source-version-override`. Adding a *file* under `shallow/` is picked up
194+
automatically and needs no CI change.
195+
196+
## 8. Changing *how* the suite is invoked
197+
198+
The worker count, marker selection, Python version, compute size and timeout live in
199+
`createCIShallowIntegBuildSpec` in the internal `SageMakerMLFPySDKInfraCDK` package —
200+
**not in this repo**. Changing any of them is a CR against that package that deploys
201+
through a pipeline, not something that merges with your PR. Two constraints there,
202+
both learned the hard way:
203+
204+
* The `cpu-integ` image the project runs on has **only Python 3.10.13** under pyenv.
205+
* Do not add `--dist loadfile`. It pins a file's tests to one xdist worker, and since
206+
each test holds a slot until its job is terminal (~75s), it serialized the largest
207+
file to 19m45s against a 30-minute timeout.
208+
209+
## 9. Troubleshooting
210+
211+
| Symptom | Cause |
212+
|---|---|
213+
| `ResourceLimitExceeded` / utilization above the quota | A submission path that doesn't hold a slot to terminal (step 4), or `SHALLOW_MAX_CONCURRENT_JOBS` raised |
214+
| Test hangs ~15 min then warns | Slot-acquire timeout (900s). Something is holding slots — usually a leaked job |
215+
| `PytestUnknownMarkWarning` | Marker registered in `tox.ini` instead of `pyproject.toml` (step 3) |
216+
| Build passes but ran nothing | pytest exit code 5, "no tests collected" — a moved directory or mistyped marker. The fast project deliberately does **not** tolerate exit 5, so this fails the build |
217+
| `AccessDenied` on S3 from CI but not locally | A hardcoded bucket belonging to your account (step 2) |
218+
| `ImportError: cannot import name …` locally | Stale editable install; re-run the step-5 installs |

0 commit comments

Comments
 (0)