From 9538208b0f890e2de10792643365295c8825760b Mon Sep 17 00:00:00 2001 From: Amarjeet LNU Date: Thu, 27 Aug 2026 13:05:13 -0700 Subject: [PATCH] docs(train): add an SOP for adding/updating shallow integ tests 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 --- .../tests/integ/train/shallow/README.md | 7 + .../tests/integ/train/shallow/SOP.md | 218 ++++++++++++++++++ 2 files changed, 225 insertions(+) create mode 100644 sagemaker-train/tests/integ/train/shallow/SOP.md diff --git a/sagemaker-train/tests/integ/train/shallow/README.md b/sagemaker-train/tests/integ/train/shallow/README.md index ceeb4f53be..4a84bc0a79 100644 --- a/sagemaker-train/tests/integ/train/shallow/README.md +++ b/sagemaker-train/tests/integ/train/shallow/README.md @@ -1,5 +1,9 @@ # Shallow (submit-then-stop) integration tests +> **Adding or changing a test? Follow [SOP.md](./SOP.md).** This README explains why +> the suite is shaped the way it is; the SOP is the step-by-step procedure, including +> how to run the suite locally. + These tests add fast acceptance coverage on the PR gate. They run in their own `fast-integ-tests` job, **alongside** the existing `integ-tests` CodeBuild suite, 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. ## Writing a new test +The harness API, for reference. For the full procedure — where the test goes, markers, +running it locally, the pre-submit checklist — see [SOP.md](./SOP.md). + Use the harness; do not call `trainer.train()` directly. ```python diff --git a/sagemaker-train/tests/integ/train/shallow/SOP.md b/sagemaker-train/tests/integ/train/shallow/SOP.md new file mode 100644 index 0000000000..7b77e3b2b0 --- /dev/null +++ b/sagemaker-train/tests/integ/train/shallow/SOP.md @@ -0,0 +1,218 @@ +# SOP — adding or updating a fast (shallow) integ test + +The step-by-step procedure for changing anything under +`sagemaker-train/tests/integ/train/shallow`. [`README.md`](./README.md) explains *why* +the suite is shaped the way it is; this file is what to do, in what order, and how to +verify each step. + +## 0. Does your test belong here? + +This suite submits a real `CreateTrainingJob`, asserts the service returned an ARN, +and immediately stops the job. A test belongs here if and only if what it checks is +decided **synchronously, at submit time** — payload shape, validation, permissions, +image/recipe resolution. + +| You want to assert… | Put it in | +|---|---| +| the service accepted (or rejected) a payload | **here** | +| client-side config assembly, with no service call | `tests/integ/train/test_dry_run_integration.py` | +| artifacts, metrics, logs, convergence — anything needing the job to *run* | the deep suite, marked `gpu_intensive` | + +A regression that makes training itself fail will still pass here. That is deliberate; +see *What these tests deliberately do NOT cover* in the README. + +## 1. Decide where the test goes + +| Case | Action | Cost | +|---|---|---| +| New behaviour on one trainer | Add to the matching `test_.py` | 1 job | +| Behaviour shared by **all** recipe trainers | Add a case to `recipe_cases.py` | **1 job × every subclass** — currently 5 (SFT, DPO, RLVR, RLAIF, CPT) | +| A brand-new trainer | New `test_.py` subclassing `RecipeTrainerCases` | the 9 shared cases | + +Adding to `recipe_cases.py` multiplies. Only put a case there if it is genuinely +trainer-independent; otherwise it belongs in one file. + +A new trainer file is two lines plus opt-outs: + +```python +class TestMyTrainer(RecipeTrainerCases): + TRAINER = MyTrainer + EXTRA_KWARGS = {...} # required constructor args, if any + SUPPORTS_SERVERFUL = False # trainer takes no `compute` + SUPPORTS_TRAINING_TYPE = False # no LoRA/full distinction +``` + +**Verify:** `pytest tests/integ/train/shallow --collect-only` lists your test(s) and +the total moved by the number you expect. + +## 2. Write it with the harness + +Never call `trainer.train()` directly — the harness is what forces `wait=False`, +stops the job in a `finally`, and holds the concurrency slot. + +```python +from .harness import assert_submitted, assert_rejected, submitted, unique_name + +def test_my_feature_is_accepted(sagemaker_session, train_data_uri): + trainer = _trainer(sagemaker_session, unique_name("shallow-my-feature"), ...) + with submitted(trainer) as job: + assert_submitted(job) + +def test_bad_input_is_rejected(sagemaker_session): + assert_rejected(trainer, ("does not exist", "ValidationException")) +``` + +Rules that are not negotiable, each with the failure it prevents: + +| Rule | Why | +|---|---| +| Use `submitted()` / `assert_rejected()`, never bare `train()` | Skips the stop, the slot, and the terminal-wait | +| Never pass `wait=` | Rejected with `TypeError`, so a copy-pasted `wait=True` cannot reintroduce a real training run | +| Never set `keep_alive_period_in_seconds` | A warm pool outlives the stop and keeps instances provisioned | +| 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 | +| Resolve images with `harness.cpu_image(sagemaker_session)` | The DLC registry account differs by partition; a hardcoded URI is unusable outside one | +| 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 | +| 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 | +| 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 | + +**Verify:** `grep -n "wait=\|keep_alive" ` returns nothing. + +## 3. Markers + +| Marker | Effect on your shallow test | +|---|---| +| *(none)* | Runs on the PR gate in `fast-integ-tests`. **This is what you want.** | +| `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. | +| `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. | +| `gpu_intensive` | **Never on a shallow test.** It marks deep tests *off* the gate. | + +Two rules that cut both ways: + +* **Do not add `gpu_intensive` to a deep test unless a shallow test covers the same + path** — the gate silently loses coverage. Update the coverage table in the README + when you do. +* **Register any new marker 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. That matters because the gate selects with + `-m "not gpu_intensive and not us_east_1"`: a typo'd name would put an expensive + deep test back on the gate instead of erroring. + +**Verify:** run with `-W error::pytest.PytestUnknownMarkWarning`; an unregistered +marker fails instead of warning. + +## 4. Adding a new submission path? Take slots yourself + +The concurrency cap is enforced inside `submitted()` and `assert_rejected()`, so a +normal test is capped automatically. If you add a path that submits **without** going +through those two — as `_tuning()` in `test_tuner.py` does, because a tuning job is +stopped via `tuner.stop_tuning_job()` — you must acquire `job_slots()` yourself and +hold them until the job is **terminal**, not until `stop()` returns. + +Getting this wrong is the bug that made an earlier version peak at ~37 concurrent jobs +against a limit of 20. See *Concurrency cap* in the README. + +## 5. Run it locally + +**Prerequisites** + +* AWS credentials for an SDK test account. The suite uses ambient credentials — no + profile logic — and resolves the execution role through the real discovery path + (`TrainDefaults.get_role(role=None, ...)`), so the account needs a discoverable + SageMaker execution role. +* Region defaults to **us-west-2**; an autouse fixture pins `AWS_DEFAULT_REGION` + unless you set it yourself. +* Recipe resolution goes through a private hub named `sdktest` (an autouse fixture + sets `SAGEMAKER_HUB_NAME`), so the account needs it. +* `us_east_1` tests need credentials in the us-east-1 test account; their fixture + pins the region regardless of `AWS_DEFAULT_REGION`. + +**Install, the same way CI does** + +```bash +cd sagemaker-core && pip install -e '.[test]' +cd ../sagemaker-train && pip install -e '.[test]' +``` + +**Run** + +```bash +# one test, no cap, no xdist — for debugging +SHALLOW_MAX_CONCURRENT_JOBS=0 python -m pytest \ + tests/integ/train/shallow/test_sft_trainer.py -k my_feature + +# one file, gate selection +python -m pytest tests/integ/train/shallow/test_sft_trainer.py \ + -m "not gpu_intensive and not us_east_1" + +# the whole suite exactly as the PR gate runs it +python -m pytest tests/integ/train/shallow -v -n 8 \ + -m "not gpu_intensive and not us_east_1" --durations 15 +``` + +Expect ~7 minutes for the full suite at `-n 8`, and ~1–2 minutes for a single test — +dominated by the wait for the job to reach a terminal state, not by the SDK. + +**Optional env vars.** Tests whose inputs are not derivable read them from the +environment and **skip** when absent, so they never fail for a missing resource: + +| Variable | Gates | Default | +|---|---|---| +| `SHALLOW_MAX_CONCURRENT_JOBS` | Concurrency cap; `0` disables gating | 10 | +| `SHALLOW_HYPERPOD_CLUSTER` | `test_cpt_trainer.py` HyperPod cases | skip | +| `SHALLOW_MTRL_AGENT_ENV`, `SHALLOW_MTRL_MLFLOW_APP_ARN`, `SHALLOW_MTRL_DATASET` | `test_multi_turn_rl_trainer.py` | skip | +| `SHALLOW_MTRL_MODEL` | Multi-turn RL model id | `mock-oss-test` | + +## 6. Pre-submit checklist + +```bash +tox -e black-format && tox -e flake8 && tox -e pylint # from sagemaker-train/ +``` + +- [ ] Collection count moved by exactly what you expect (step 1). +- [ ] At least one negative test for the feature area (step 2). +- [ ] No `wait=`, no `keep_alive_period_in_seconds`, no hardcoded bucket/URI/ARN. +- [ ] Your test is unmarked unless it is genuinely us-east-1-only (step 3). +- [ ] If you marked a deep test `gpu_intensive`, the README coverage table accounts + for it. +- [ ] The suite still passes locally at `-n 8`. + +## 7. What CI will do + +| Job | Project | Runs | +|---|---|---| +| `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 | +| `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 | +| `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 | + +Two things worth knowing: + +* `fast-integ-tests` only fires when `sagemaker-train` is in the change set. A + docs-only or CDK-only change will not exercise your test. +* Neither job checks out PR code onto the GitHub runner — both start CodeBuild with + `source-version-override`. Adding a *file* under `shallow/` is picked up + automatically and needs no CI change. + +## 8. Changing *how* the suite is invoked + +The worker count, marker selection, Python version, compute size and timeout live in +`createCIShallowIntegBuildSpec` in the internal `SageMakerMLFPySDKInfraCDK` package — +**not in this repo**. Changing any of them is a CR against that package that deploys +through a pipeline, not something that merges with your PR. Two constraints there, +both learned the hard way: + +* The `cpu-integ` image the project runs on has **only Python 3.10.13** under pyenv. +* Do not add `--dist loadfile`. It pins a file's tests to one xdist worker, and since + each test holds a slot until its job is terminal (~75s), it serialized the largest + file to 19m45s against a 30-minute timeout. + +## 9. Troubleshooting + +| Symptom | Cause | +|---|---| +| `ResourceLimitExceeded` / utilization above the quota | A submission path that doesn't hold a slot to terminal (step 4), or `SHALLOW_MAX_CONCURRENT_JOBS` raised | +| Test hangs ~15 min then warns | Slot-acquire timeout (900s). Something is holding slots — usually a leaked job | +| `PytestUnknownMarkWarning` | Marker registered in `tox.ini` instead of `pyproject.toml` (step 3) | +| 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 | +| `AccessDenied` on S3 from CI but not locally | A hardcoded bucket belonging to your account (step 2) | +| `ImportError: cannot import name …` locally | Stale editable install; re-run the step-5 installs |