From b200618a044ee0f266b97b1cf483a64552a11007 Mon Sep 17 00:00:00 2001 From: Amarjeet LNU Date: Mon, 24 Aug 2026 10:05:42 -0700 Subject: [PATCH 1/2] fix(train): give shallow RLVR cases the reward signal RLVR requires RLVRTrainer.train() refuses to submit unless custom_reward_function was passed or hyperparameters.preset_reward_function is set. TestRLVRTrainerSubmission inherits the shared cases from RecipeTrainerCases, which pass neither -- they are about recipe rendering and dataset handling, not reward configuration -- so 14 of the class's 17 tests failed: 12 raising the ValueError, and the two negative cases failing with "rejected, but not for the expected reason" because the reward error preempted the S3 validation error they assert on. Set the preset in a build() override rather than repeating it in each test, and skip it when the test supplies its own custom_reward_function so the three reward-function variants still exercise exactly what they name. "prime_code" is one of the values the recipe's preset_reward_function enum accepts ('', gsm8k, prime_code, prime_math) and is what the deep suite pairs with an ordinary training dataset on this same model. This was not a regression from a later change to sagemaker-train. The guard landed in #6181 on 2026-08-14, five days before the shallow suite merged (#6176), and rlvr_trainer.py is unchanged since. The suite had simply never run in CI: the fast-integ-tests job could not check out fork PR code, and because pull_request_target runs the base branch's workflow it could not have run on #6176 itself either. Verified against us-west-2 in the SDK test account: 14 passed in 94s, each submitting and immediately stopping a real training job. --- X-AI-Prompt: Fix the failing shallow sagemaker-train RLVR integ tests, which were being rejected at submission for a missing reward signal X-AI-Tool: claude-code --- .../integ/train/shallow/test_rlvr_trainer.py | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/sagemaker-train/tests/integ/train/shallow/test_rlvr_trainer.py b/sagemaker-train/tests/integ/train/shallow/test_rlvr_trainer.py index 5858692007..354a4876bd 100644 --- a/sagemaker-train/tests/integ/train/shallow/test_rlvr_trainer.py +++ b/sagemaker-train/tests/integ/train/shallow/test_rlvr_trainer.py @@ -33,12 +33,34 @@ "arn:aws:sagemaker:us-west-2:729646638167:hub-content/sdktest/JsonDoc/rlvr-test-rf/0.0.1" ) +# The preset the deep suite pairs with an ordinary training dataset on this same +# model (test_rlvr_trainer_lora_complete_workflow). +PRESET_REWARD_FUNCTION = "prime_code" + class TestRLVRTrainerSubmission(RecipeTrainerCases): """RLVR accepts every shared case, plus recipe customization.""" TRAINER = RLVRTrainer + def build(self, sagemaker_session, dataset, name, **overrides): + """Add a reward signal, which RLVR requires before it will submit. + + ``RLVRTrainer.train()`` raises ``ValueError`` unless + ``custom_reward_function`` was passed or + ``hyperparameters.preset_reward_function`` is set. The cases inherited from + ``RecipeTrainerCases`` pass neither -- they are about recipe rendering and + dataset handling, not reward configuration -- so the preset is applied once + here rather than repeated in each test. + + Skipped when the test supplies its own ``custom_reward_function``, so the + reward-function variants below still exercise exactly what they name. + """ + trainer = super().build(sagemaker_session, dataset, name, **overrides) + if not overrides.get("custom_reward_function"): + trainer.hyperparameters.preset_reward_function = PRESET_REWARD_FUNCTION + return trainer + def test_direct_hyperparameter_mutation(self, sagemaker_session, train_data_uri): """trainer.hyperparameters. = ... is a documented pattern (used by the existing RLVR tests) and must reach the payload intact.""" From 479a85e2cfaff1ac76495342e42d4bcf4ff4d30a Mon Sep 17 00:00:00 2001 From: Amarjeet LNU Date: Mon, 24 Aug 2026 10:05:43 -0700 Subject: [PATCH 2/2] ci: run fast-integ-tests in CodeBuild instead of on the runner Replaces the runner-based shallow suite with a CodeBuild invocation, so the suite gates fork PRs -- which is nearly all of them. The job stopped working when actions/checkout began refusing to place fork PR code in a pull_request_target job. That refusal is correct: the runner holds the base repo's GITHUB_TOKEN and assumes CI_AWS_ROLE_ARN, so a fork could edit conftest.py and read those credentials out. On a public repo, overriding it with allow-unsafe-pr-checkout would be a live credential-exfiltration path. Guarding the job to same-repo PRs would stop the failure, but 59 of the last 60 merged PRs here are from forks, so that leaves ~2% coverage. This is the real fix: start CodeBuild with source-version-override, exactly as the codestyle-doc-tests, unit-tests and integ-tests jobs already do. The build never sees the runner's token, secrets or default-branch cache, so no same-repo guard is needed. Its own project rather than folding into sagemaker-train-integ-tests, so a shallow failure stays distinguishable from a deep-suite failure and runs concurrently with it rather than queueing behind it. Dropped the upload-artifact step: the JUnit XML no longer exists on the runner, and results are in the CodeBuild logs. Tradeoff recorded in both the workflow comment and the suite README: the pytest selection now lives in createCIShallowIntegBuildSpec in SageMakerMLFPySDKInfraCDK, so changing how the suite is invoked is no longer reviewable in a PR to this repo. Adding a test file under shallow/ is still picked up automatically. The project sagemaker-python-sdk-ci-sagemaker-train-fast-integ-tests is deployed, so the job resolves on merge. --- X-AI-Prompt: Instead of the GitHub runner, run the sagemaker-train shallow integ suite in CodeBuild like the other CI workflows, so fork PRs are gated after actions/checkout began refusing fork PR code in pull_request_target X-AI-Tool: claude-code --- .github/workflows/pr-checks-master.yml | 116 +++++------------- .../tests/integ/train/shallow/README.md | 17 +++ 2 files changed, 51 insertions(+), 82 deletions(-) diff --git a/.github/workflows/pr-checks-master.yml b/.github/workflows/pr-checks-master.yml index 24d0c8349b..e1c31472a7 100644 --- a/.github/workflows/pr-checks-master.yml +++ b/.github/workflows/pr-checks-master.yml @@ -246,18 +246,36 @@ jobs: # Additive: runs the shallow (submit-then-stop) suite for sagemaker-train # alongside the existing integ-tests job above, which is unchanged. # - # Why a separate job rather than folding this into the CodeBuild suite: this - # job's selection is reviewable in the PR that changes it, whereas the - # sagemaker-train CodeBuild buildspec is CDK-managed outside this repo. It also - # reports as its own check, so a shallow failure is distinguishable at a glance - # from a deep-suite failure, and it finishes in minutes -- fast feedback that - # does not wait on the 2XLARGE CodeBuild container. + # Runs in CodeBuild, not on the runner. It began as a runner job, but + # actions/checkout refuses to place a fork's head commit in a + # pull_request_target job -- correctly, because the runner also holds the base + # repo's GITHUB_TOKEN and assumes CI_AWS_ROLE_ARN, so a fork could edit + # conftest.py and read those credentials out. That is the "pwn request" shape, + # and on a public repo it is a live credential-exfiltration path, so the fix is + # to move the execution rather than override the refusal with + # allow-unsafe-pr-checkout. Nearly every PR here comes from a fork, so a + # same-repo guard would have left the suite with almost no gate coverage. + # source-version-override is how the three jobs above already run PR code: the + # build never sees the runner's token, secrets or default-branch cache. # - # What runs here: only tests/integ/train/shallow. The client-side tests - # (recipe resolution, data utils, dry-run, log streaming) are deliberately NOT - # repeated -- the CodeBuild suite already runs the whole tests/integ tree, so - # widening this job's scope would duplicate them and double the job creation - # the shallow suite performs. + # Why its own project rather than folding this into the sagemaker-train + # integ-tests project: it reports as its own check, so a shallow failure is + # distinguishable at a glance from a deep-suite failure, and it runs + # concurrently with the deep suite instead of queueing behind it. + # + # Tradeoff of moving off the runner: the pytest selection now lives in the + # CDK's buildspecs.ts (createCIShallowIntegBuildSpec) instead of this file, so + # changing which tests run is no longer reviewable in a PR to this repo. That + # is the price of executing fork code safely, and it is the same place the + # other three test jobs' selections already live. + # + # What runs there: only tests/integ/train/shallow, deselecting gpu_intensive + # (the CPT and MTRL classes, which need a pre-provisioned HyperPod cluster and + # an agent runtime plus an MLflow app) and us_east_1 (Nova cases, which run in + # the integ-tests-us-east-1 project against the Nova account). The client-side + # tests are deliberately not repeated -- the deep suite already runs the whole + # tests/integ tree, so widening scope would duplicate them and double the job + # creation the shallow suite performs. # # Why submit-then-stop is worth gating on: CreateTrainingJob returns a # TrainingJobArn only after the request has cleared public-model validation, @@ -272,19 +290,10 @@ jobs: fast-integ-tests: runs-on: ubuntu-latest needs: [detect-changes] + # No same-repo guard: nothing here checks out PR code, so fork PRs are gated + # too. The suite only runs when sagemaker-train is in the change set. if: contains(fromJson(needs.detect-changes.outputs.submodules), 'sagemaker-train') steps: - - uses: actions/checkout@v3 - with: - # pull_request_target checks out the base ref by default; these tests - # must run against the PR's code. - ref: 'refs/pull/${{ github.event.pull_request.number }}/head' - - - name: Set up Python - uses: actions/setup-python@v5 - with: - python-version: '3.12' - - name: Configure AWS Credentials uses: aws-actions/configure-aws-credentials@v4 with: @@ -292,68 +301,11 @@ jobs: aws-region: us-west-2 role-duration-seconds: 10800 - - name: Install sagemaker-train and test dependencies - run: | - python -m pip install --upgrade pip - pip install ./sagemaker-core - pip install ./sagemaker-train - pip install -r requirements/extras/test_requirements.txt - - name: Run shallow sagemaker-train integ tests - working-directory: sagemaker-train - env: - AWS_DEFAULT_REGION: us-west-2 - # Role resolution goes through iam:SimulatePrincipalPolicy, which is - # low-TPS; adaptive retries keep parallel workers from throttling each - # other. - AWS_RETRY_MODE: adaptive - AWS_MAX_ATTEMPTS: '10' - # Cap the training jobs the *service* counts against the concurrency - # quota, across all xdist workers, so the suite stays inside the - # "concurrent model customization serverless jobs per Region" quota - # (20) with room for the deep integ-tests suite running the same - # account concurrently. The harness holds each slot until the job is - # terminal, not until stop() returns -- the service counts a job for - # ~1-3 min after the stop -- so 10 means "at most 10 jobs counted at - # once", the batches-of-10 behaviour, not "10 stops in flight". - # - # Not redundant with -n 8. -n caps worker processes; this caps what - # the service counts, and with the slot held to terminal those diverge - # sharply (each drain outlives the worker's stop() by minutes). It is - # also what keeps the ceiling stable if -n is raised. A serverful job - # counts one slot per instance. - SHALLOW_MAX_CONCURRENT_JOBS: '10' - run: | - # Scoped to shallow/ only -- see the comment above this job for why the - # rest of tests/integ/train is not repeated here. - # - # 84 of the suite's 100 tests run; the 16 deselected are: - # us_east_1 (5) -- Nova cases; this job holds us-west-2 - # credentials only, so they run in the - # integ-tests-us-east-1 job instead. - # gpu_intensive (11) -- the CPT and MTRL classes. These are written in - # the shallow style but cannot be made - # self-contained: CPT submits only via HyperPod - # (a pre-provisioned cluster, not - # CreateTrainingJob) and MTRL needs an agent - # runtime plus an MLflow app. Both become - # gate-eligible by dropping one marker once those - # prerequisites exist in the PR account. - python -m pytest tests/integ/train/shallow \ - -m "not gpu_intensive and not us_east_1" \ - -n 8 \ - --dist loadfile \ - -v \ - --durations=15 \ - --junitxml=shallow-integ-results.xml - - - name: Upload test results - if: always() - uses: actions/upload-artifact@v4 + uses: aws-actions/aws-codebuild-run-build@v1 with: - name: shallow-integ-test-results - path: sagemaker-train/shallow-integ-results.xml - if-no-files-found: warn + project-name: ${{ github.event.repository.name }}-ci-sagemaker-train-fast-integ-tests + source-version-override: 'refs/pull/${{ github.event.pull_request.number }}/head^{${{ github.event.pull_request.head.sha }}}' integ-tests-us-east-1: runs-on: ubuntu-latest diff --git a/sagemaker-train/tests/integ/train/shallow/README.md b/sagemaker-train/tests/integ/train/shallow/README.md index 02de12214d..07c8912185 100644 --- a/sagemaker-train/tests/integ/train/shallow/README.md +++ b/sagemaker-train/tests/integ/train/shallow/README.md @@ -8,6 +8,23 @@ What this suite changes about the gate is not which job runs, but what the exist one selects: the `gpu_intensive` marks added here deselect the deep tests that submit a job and wait for it, and this suite covers those code paths instead. +> **Where this runs.** The `fast-integ-tests` job in `pr-checks-master.yml` does not +> execute this suite on the GitHub runner — it starts the CodeBuild project +> `sagemaker-python-sdk-ci-sagemaker-train-fast-integ-tests` via +> `source-version-override`, the same way the `codestyle-doc-tests`, `unit-tests` +> and `integ-tests` jobs run PR code. That is deliberate: the runner holds the base +> repo's `GITHUB_TOKEN` and assumes `CI_AWS_ROLE_ARN`, so `actions/checkout` refuses +> to place a fork's head commit there, and on a public repo overriding that refusal +> would be a live credential-exfiltration path. Running in CodeBuild gates fork PRs +> — which is nearly all of them — without exposing those credentials to PR code. +> +> **Consequence for editing this suite:** the marker selection above (`-n 8`, +> `--dist loadfile`, `-m "not gpu_intensive and not us_east_1"`) lives in +> `createCIShallowIntegBuildSpec` in the `SageMakerMLFPySDKInfraCDK` package, not in +> this repo. Adding a file under `shallow/` is picked up automatically, but changing +> *how* the suite is invoked means a change there, which deploys through a pipeline +> rather than merging with your PR. + ## What a passing test proves Each test submits a real `CreateTrainingJob`, asserts the service returned a