Merge master into feature-smtj-instance-preferences-latest (merge commit, do NOT squash) - #6248
Closed
deeppcs wants to merge 31 commits into
Closed
Conversation
…ws#6182) * fix: rever preset reward function deletion from hyperparams dict * testing: add unit test to prevent future regression of preset_reward_function --------- Co-authored-by: Syed Jafri <syedjfr@amazon.com>
The reviewer aborted on every fork PR with "Actor does not have write permissions to the repository" (e.g. run 31217496013 on aws#6166), so it only ever ran for collaborators. claude-code-action checks that the PR author has write access before doing anything. That default protects its normal @claude usage, where a read-only user's comment becomes the prompt. It does not apply here: pull_request_target always runs the base-branch copy of this workflow, so the prompt is fixed by maintainers and a fork cannot supply it. Set allowed_non_write_users so the review actually runs, and harden the prompt-injection surface it exposes (untrusted diff/PR text entering context): - deny Read on /proc, /sys, ~/.aws, the Actions _temp dir and .git/config so an injected instruction cannot use the review comment as a secret-exfiltration channel - instruct the model to treat all contributor-authored content as data, never as instructions, and to report attempted injection Fork PRs continue to require maintainer approval via the manual-approval environment, Bash/Write/Edit remain unavailable, and the assumed role is still limited to bedrock:InvokeModel on a single inference profile.
…w submit-then-stop suite (aws#6176) * change(train): gate deep integ tests behind gpu_intensive, add shallow submit-then-stop suite Replaces the CodeBuild integ suite for sagemaker-train on the PR gate with a faster selection that keeps meaningful server-side coverage. CreateTrainingJob returns a TrainingJobArn only after the request has cleared every synchronous server-side gate: public-model shape validation, SigV4, sagemaker:CreateTrainingJob authorization (including condition keys), iam:PassRole on the execution role, the training backend's synchronous request validators, its role-assuming validators (which make real S3/ECR/FSx calls as the customer), post-validator business logic (training-plan capacity, routing, recipe filtering) and the final conditional write that rejects duplicate job names. So "the ARN came back" proves the SDK-shaped payload was accepted as sent and the caller held the permissions needed to submit it -- without paying for a training run. Adds tests/integ/train/shallow (70 tests) built on that: submit, assert the ARN, stop immediately. Covers ModelTrainer (payload shaping, source-code packaging, input channels, compute, networking, checkpointing/spot), the recipe trainers (SFT/DPO/RLVR/RLAIF, serverless and serverful), recipe customization (overrides, explicit recipe files, sequence_length, DataMixingConfig), and the non-training job types (HyperParameterTuningJob, AgentRFT Job). Includes negative tests so the suite cannot pass merely because some ARN came back. Marks the 19 previously-unmarked tests that submit a job and wait for it with gpu_intensive, so they continue running on the scheduled CI-health workflows instead of the PR gate. Widens that marker's description: despite the name it gates anything consuming real training capacity, including serverless and CPU-instance jobs. The PR job now runs the whole tests/integ/train tree with -m "not gpu_intensive and not us_east_1" rather than only shallow/, which keeps the ~170 client-side tests (recipe resolution, data utils, dry-run, log streaming) on the gate -- they make no service call and were never the expensive part. Net: 191 of 251 tests on the PR gate, none of which waits for a training job. This is a deliberate scope reduction: training *behaviour* (artifacts, metrics, convergence) is no longer asserted on the PR gate. A regression that breaks training itself -- a bad entry script, a broken container command -- will pass here and be caught by the scheduled suites. * change(train): fix CPTTrainer construction and role-rejection test after first real AWS run Verified against AWS in account 729646638167 (us-west-2): * test_unassumable_role_is_rejected: ModelTrainer.__init__ validates the role via iam:SimulatePrincipalPolicy, so a bad role raises RoleValidationError at construction and never reaches CreateTrainingJob. Assert around the constructor instead of around train(). * test_cpt_trainer_is_accepted: CPTTrainer takes no training_type, and its compute is HyperPodCompute-only, so it cannot use the shared _trainer helper. WIP: 2 further real failures still to fix (RLAIF compute, tuner job-name collision). See SHALLOW_TEST_RUN_STATE.md. * change(train): fix shallow suite against real AWS; 62/62 passing Ran the suite against account 729646638167 (us-west-2) with PYTHONPATH pointed at this clone, and fixed every failure it surfaced. All were wrong assumptions in the tests, not service problems: * conftest: add a session-scoped bundled_service_model fixture setting AWS_DATA_PATH to sagemaker-core/sample. The public botocore model has no ServerlessJobConfig.SequenceLength, so sequence_length requests were rejected client-side before reaching the service. Mirrors the existing setup_aws_data_path fixture in test_recipe_override_integration.py. * harness: unique_name() now takes max_length. Tuning job names are capped at 32 characters, not the 63 allowed for training jobs, and the service enforces it: Value '...' at 'hyperParameterTuningJobName' failed to satisfy constraint: Member must have length less than or equal to 32 * tuner tests: submit under an explicit job_name via a _tuning() context manager. The tuner derives its default name from the training image plus a second-granularity timestamp and ignores base_job_name, so two tuner tests in the same second collided with ResourceInUse. * RLAIF: excluded from TestServerfulSubmission. RLAIFTrainer has no compute parameter, so it has no serverful path. Still covered by every serverless case. * CPT: marked gpu_intensive and skipped unless SHALLOW_HYPERPOD_CLUSTER is set. CPT refuses to submit without HyperPod compute, and HyperPod targets a pre-provisioned cluster rather than CreateTrainingJob. * sequence_length / training_type: narrowed to the values the recipe catalogue actually offers for this model ('4K' only; no serverless recipe for FULL). Both left parametrized so more values can be added against a model that supports them, rather than dropping the distinction. Result: 62 passed, 0 failed, 5m18s serial (~5s/test). Cost model confirmed empirically rather than assumed: across 100 jobs created by these runs, every one ended Stopped and every BillableTimeInSeconds was null. Jobs are torn down while still in Starting/Pending, before instances become billable. * change(train): one shallow file per trainer; only mark deep tests that have shallow coverage Addresses two review points. 1. Only mark deep tests that this suite actually replaces. Reverts gpu_intensive from 9 tests that had no shallow counterpart, so the PR gate no longer loses coverage with nothing replacing it: * all 8 evaluator tests (benchmark, custom scorer, inspect_ai, llm_as_judge x2, llmaj_custom_model) -- evaluate() is a different API surface returning pipeline executions, and this suite has no coverage for it * test_notifications.py -- asserts EventBridge/SNS side effects, not submission 10 marks remain, each with a named shallow equivalent documented in the suite README. The rule is written down there: do not mark a deep test unless a shallow test covers the same path. 2. One file per trainer, matching the existing deep-suite layout. test_recipe_trainers_submission.py -> test_{sft,dpo,rlvr,rlaif,cpt}_trainer.py test_recipe_customization_submission.py (recipe cases folded into rlvr/sft; Nova data mixing to its own file) test_other_job_types_submission.py -> test_tuner.py, test_multi_turn_rl_trainer.py test_model_trainer_submission.py -> test_model_trainer.py The "recipe_*" names described how the SDK groups these internally rather than what a reader looks for; the shallow counterpart of a given deep test is now obvious from the filename. recipe_cases.py holds the cases every recipe trainer shares. Each per-trainer class subclasses RecipeTrainerCases and sets TRAINER, so a new trainer is a two-line file, and per-trainer deviations are declared rather than duplicated: EXTRA_KWARGS (RLAIF's reward model), SUPPORTS_SERVERFUL=False (RLAIF takes no compute), SUPPORTS_TRAINING_TYPE=False (CPT has no LoRA/full split). Not named test_* so pytest does not collect the base class. Inheriting the shared cases also widened coverage: DPO and RLAIF now get the full set (output path, dataset override, both negative cases) rather than only the three they had as parametrized entries. 80 tests total, 69 on the PR gate. Verified against AWS (account 729646638167, us-west-2): 68 passed, 1 skipped, 0 failed in 6m59s. The skip is RLAIF's serverful case, reporting "RLAIFTrainer takes no compute argument". * change(train): add shallow coverage for every gpu_intensive test that has an equivalent Previous commits only audited the marks this PR added. This audits all 46 gpu_intensive tests in tests/integ/train -- including those already marked on master -- and adds the missing shallow counterparts. Added (were gaps): * MLflow, in RecipeTrainerCases so all four recipe trainers get it. Every *_complete_workflow deep test configures MLflow, so without this their shallow counterparts missed that half of the payload. Two forms: experiment/run names (always runs) and mlflow_resource_arn (skips if the account has no app). * RLVR reward functions, all three forms the deep suite covers: hub-content ARN, Lambda ARN (auto-creates an Evaluator), and a pre-created Evaluator object. * RLAIF reward_prompt as a hub-content ARN rather than a Builtin.* name, and continued fine-tuning from a model-package ARN. * Nova SFT and Nova RLVR, in test_nova_trainers.py. Nova needs a different recipe family, region and account, so it cannot share RecipeTrainerCases; marked us_east_1. Two real constraints the AWS run surfaced, both now recorded in comments: * The reward-function tests cannot use this suite's generic chat-format fixture. Before submitting, the SDK *invokes* the reward function over sample records and fails if they do not score ("GSM8k scoring failed"). They now use the same dataset as the deep RLVR suite, via a dedicated reward_scored_data_uri fixture. * list_mlflow_apps is not a paginatable operation, so the fixture calls it directly instead of via get_paginator. Also fixed a ScopeMismatch: the three new lookup fixtures were session-scoped but depend on the parent conftest's module-scoped sagemaker_session. All three new fixtures (mlflow_arn, reward_lambda_arn, reward_evaluator) only look resources up and skip when absent. The deep suite's equivalents create them -- IAM roles, Lambdas, MLflow apps, registry entries -- which is a durable side effect a fast PR-gate suite should not have. Still uncovered, documented in the suite README with the reason: the 11 evaluator tests (evaluate() is a different API surface returning pipeline executions) and the 3 HyperPod tests (submit to a pre-provisioned cluster, not CreateTrainingJob). Neither is newly marked by this PR, so no coverage is lost; the evaluator gap is the clearest follow-up. 97 tests total, 82 on the PR gate. Verified against AWS (729646638167, us-west-2): 81 passed, 1 skipped, 0 failed in 7m04s. The skip is RLAIF's serverful case, which reports its own reason. * change(train): add shallow coverage for recipe overrides, GRPO hyperparameters, Nova serverful Three remaining gpu_intensive tests had no shallow counterpart: * test_sft_trainer_serverful_smtj.py (override half) -> SFT test_recipe_overrides_are_accepted. Asserts both halves: the merge reached the rendered recipe (client-side, exact) and the resulting payload is still accepted (recipe filtering runs after the request validators, so a bad merge only surfaces at submission). Verified against AWS: overrides are written flat under training_config but land nested under training_args, and the recipe default for this model is 5 -- so asserting 1 proves the override applied rather than coinciding with the default. * test_rlvr_trainer_nemotron_with_kl_and_recipe -> RLVR test_kl_and_clipping_hyperparameters. These are separate recipe fields rather than one flag, so the existing max_epochs-only test did not prove they serialize. * test_sft_trainer_serverful_smtj.py (Nova half) -> Nova TestNovaServerfulSubmission. Distinct from the shared serverful case: Nova model, Nova recipe family, Nova-only instance type, us-east-1. Accepts the override under either trainer.max_epochs or training_args.max_epochs, since recipe families nest epoch control differently -- so the test fails on a lost override rather than on a recipe-layout difference. Verified against a real account (us-west-2): 83 passed, 1 skipped, 0 failed in 5m20s. The skip reports its own reason (RLAIFTrainer takes no compute argument). * fix(train): make the shallow Nova tests runnable in any account The five us_east_1 shallow tests referenced resources hardcoded to one test account and had therefore never actually executed. Verified: from 729646638167, `aws s3 ls s3://sagemaker-us-east-1-784379639078/input_data/sft-nova/` returns AccessDenied. Derive everything from the calling account instead, the way test_sft_trainer_serverful_smtj.py::training_resources already does: * nova_sft_data_uri -- uploads the Nova-shaped sample data the deep suite already ships (tests/data/train/sft_smtj_sample_data.jsonl) to the caller's own bucket. Cannot reuse nova_train_data_uri: Nova SFT records carry a schemaVersion the generic chat-format fixture lacks. * nova_rlvr_data_uri -- copies the GSM8k-shaped dataset the us-west-2 RLVR tests use into the us-east-1 bucket. A copy rather than a reference because an S3 input must be in the job's region. * nova_output_path -- default_bucket() rather than a named bucket. * nova_reward_function_arn -- resolves the hub content in the caller's own account, look-up-and-skip like the other reward fixtures. Two service-verified region constraints drove this: * the model package group must be in the job's region -- passing the us-west-2 MODEL_PACKAGE_GROUP ARN is rejected with "Model package group ARN region 'us-west-2' does not match expected region 'us-east-1'". Added NOVA_MODEL_PACKAGE_GROUP (a bare name) alongside it in recipe_cases so the two Nova files cannot drift. * likewise for S3 inputs, hence the RLVR copy above. The Nova RLVR case sets skip_reward_validation=True. The SDK invokes the reward function over sample records before submitting; the function registered under that name in this account returns a shape the verifier rejects ("Each output must include 'id', 'aggregate_reward_score'"), so the test would assert per-account hub contents rather than this payload. The verifier is already covered against a known-compatible function by the three us-west-2 reward-function cases; what is unique here is the Nova recipe family and region. Also register gpu_intensive and us_east_1 in pyproject.toml. They were declared only in tox.ini, but pytest reads its config from pyproject.toml, so both were unregistered at runtime. That matters here: the PR gate selects with -m "not gpu_intensive and not us_east_1", so a typo'd marker name would silently put an expensive deep test back on the gate instead of warning. Verified against a real account: 5 passed in 47s, all five for the first time. Every job ended Stopped with BillableTimeInSeconds null, so the cost model holds in us-east-1 as well. * docs(train): record what actually bounds the PR gate's runtime A full gate run showed the shallow suite is not what makes this job slow. Measured (us-west-2, -n 8 --dist loadfile): 201 of 204 tests finished in ~7 minutes, then three evaluator tests held the run open for another 40+ before being killed. Five evaluator tests are not marked gpu_intensive and each blocks on execution.wait(..., timeout=14400) -- a 4-hour ceiling, ~33 minutes per execution in practice: test_benchmark_evaluator.py::test_benchmark_evaluation_full_flow (no marks) test_custom_scorer_evaluator.py::test_custom_scorer_evaluation_full_flow (xdist_group) test_llm_as_judge_evaluator.py::test_llm_as_judge_evaluation_full_flow (no marks) test_llm_as_judge_base_model_fix.py::test_base_model_evaluation_uses_correct_weights (serial) test_llm_as_judge_base_model_fix.py::test_base_model_false_still_works (serial) They run on master's gate too, so this PR does not add them -- but it does not fix them either, and they now dominate the job's wall clock. Deliberately NOT marking them here: unlike every other gpu_intensive test they have no shallow counterpart, so marking would remove coverage, which is what the rule this PR establishes forbids. Correct order is to add evaluator support to the harness first, then mark. Documented in the suite README so the next person does not have to rediscover it by watching a run stall at 95%. Also flags test_local_model_trainer.py in the workflow: it runs real containers, so it needs Docker and pulls pytorch-training:2.0.0-cpu-py310 (2.3 GB compressed, verified via ECR). That is fine on GitHub-hosted Ubuntu runners, which preinstall Docker, and the ECR read is already covered by the role the shallow tests use -- but it is the slowest non-evaluator thing on the gate and the only step with a disk-space floor, so the note says what to deselect first if the job ever goes flaky on runner capacity. * change(ci): keep the sagemaker-train integ job, add shallow tests alongside it Restores integ-tests to its master definition -- sagemaker-train is back in the matrix, byte-identical to master -- and makes fast-integ-tests additive rather than a replacement. The deep tests still come off the gate, just not by removing the job. The CodeBuild project's buildspec already selects -m "not gpu_intensive and not us_east_1" (verified by reading the live project), so the marks added earlier in this PR are what deselect them. No workflow edit was needed for that. Keeping the CodeBuild job also keeps things the shallow job cannot cover: * the whole tests/integ tree, so the ~170 client-side tests (recipe resolution, data utils, dry-run, log streaming) run without this job repeating them; * test_local_model_trainer.py, which needs a Docker daemon. CodeBuild runs start-dockerd with privilegedMode, which is a better home for it than a GitHub runner pulling a 2.3 GB image -- so the reviewer caveat about that is dropped as moot; * the serial/parallel split the buildspec does for rate-limited tests. fast-integ-tests is therefore scoped to tests/integ/train/shallow only. Widening it would duplicate the client-side tests and double the training jobs this suite creates. It stays a separate job rather than folding into the buildspec because the buildspec is CDK-managed outside this repo, and because a shallow failure then reports as its own check. Corrects a claim in the previous comment: the shallow suite does carry gpu_intensive tests -- 11 of them, the CPT and MTRL classes, which need a HyperPod cluster and an agent runtime. With us_east_1 that is 16 deselected, so 84 of 100 run here. The comment now lists both groups and why. Verified against a real account: 83 passed, 1 skipped, 0 failed in 3m15s (the skip self-reports: RLAIFTrainer takes no compute argument). Faster than the 5m20s measured with the client-side tests bundled in. Every job ended Stopped with BillableTimeInSeconds null; no leaked jobs. * fix: memoize role validation and mark six pipeline-waiting evaluator tests Two problems the PR gate surfaced on its own run of this branch. 1. SimulatePrincipalPolicy throttling (4 shallow tests failed) FAILED tests/integ/train/shallow/test_model_trainer.py::TestSourceCodePackaging::test_shell_entry_script FAILED tests/integ/train/shallow/test_rlaif_trainer.py::TestRLAIFTrainerSubmission::test_mlflow_resource_arn FAILED tests/integ/train/shallow/test_rlvr_trainer.py::TestRLVRTrainerSubmission::test_with_validation_dataset FAILED tests/integ/train/shallow/test_rlvr_trainer.py::TestRLVRTrainerSubmission::test_dataset_passed_to_train_overrides_constructor botocore.exceptions.ClientError: An error occurred (Throttling) when calling the SimulatePrincipalPolicy operation (reached max retries: 9): Rate exceeded Not a test defect: all four pass locally in isolation and in a local -n 36 run. Every ModelTrainer construction calls TrainDefaults.get_role -> resolve_and_validate_role, which paginates SimulatePrincipalPolicy over ~20 action names against a low, account-wide TPS limit. The CodeBuild job runs the whole tests/integ tree under -n auto (~36 workers on a 2XLARGE), which is 188 trainer constructions -- enough to exhaust even the adaptive 10-attempt budget the existing _configure_boto_adaptive_retries fixture grants. The cause is volume, not burstiness, so more retries would not have fixed it. Fixed with a _memoize_role_validation autouse session fixture: each distinct (role, role_type, region) is validated once per xdist worker instead of once per test. Measured with an instrumented botocore _make_api_call: 3 trainers -> 3 calls unpatched, 10 trainers -> 1 call memoized. Two details worth keeping: * exceptions are cached alongside successes, so a bad role still fails -- test_unassumable_role_is_rejected still passes; * teardown restores any caller now holding the memoized function, not just the ones this fixture explicitly patched. A module imported after the source module was patched binds the memoized function at its own import time, so restoring only what was patched here would leak across the session. Verified: 83 passed, 1 skipped, 0 failed in 3m33s with memoization active. 2. Six evaluator tests wait on a full evaluation pipeline From the same build's serial pass durations: 2783.83s test_llm_as_judge_base_model_fix.py::test_base_model_evaluation_uses_correct_weights 2504.59s test_llm_as_judge_base_model_fix.py::test_base_model_false_still_works 91.44s the next-slowest test in that pass 88 minutes for two tests, against a 180-minute build timeout, and they were the entire tail. Each blocks on execution.wait(..., timeout=14400) -- a 4-hour ceiling per test. Marked gpu_intensive, along with the three *_full_flow tests that wait the same way in test_benchmark_evaluator.py, test_custom_scorer_evaluator.py and test_llm_as_judge_evaluator.py. test_llmaj_custom_model.py was a genuine mismarking: it carried @pytest.mark.slow, but the registered marker name is slow_test, so the mark silently did nothing (PytestUnknownMarkWarning). us_east_1 already kept it off the us-west-2 gate, so this changes nothing there; it now also stays off the us-east-1 job. This is a small, real coverage reduction, and the README says so rather than claiming otherwise. Three of the files are marked per-test and keep their constructor/validation tests on the gate; the two class-level ones leave nothing behind, and what the gate stops checking is that a submitted pipeline is accepted and succeeds. Already-marked siblings in the same files (test_benchmark_evaluation_base_model_only, test_custom_scorer_base_model_only) show this was already the established call for pipeline-waiting tests -- these six were unmarked by omission. Shallow evaluate() coverage is the follow-up that closes the gap. Verified: 266/342 collected on the gate's selection (76 deselected), none of the six selected, and no PytestUnknownMark warnings remain. * change(train): cap concurrent training jobs in the shallow suite The shallow suite creates a training job per test. That puts it against two different quotas in two different units: * serverless (the default recipe-trainer path, no explicit compute) is bounded by "Maximum number of concurrent model customization serverless jobs per Region" -- a count of jobs, currently 20; * serverful (an explicit Compute/TrainingJobCompute: the ModelTrainer tests, the tuner, test_explicit_compute_is_accepted) is bounded by the per-instance-type quota, e.g. "ml.m5.large for training job usage" -- a count of instances. A slot is one concurrent job; a serverful job also takes one per instance, so a single cap holds the suite inside both quotas without the harness needing to know which kind of job a given test produces. Hold the slot until the job is terminal, not until stop() returns This is the subtle part, and the first cut got it wrong. The service counts a job against the concurrency quota from CreateTrainingJob until the job reaches Completed/Failed/Stopped -- NOT until StopTrainingJob returns. Measured against the service, stop() returns in a few seconds but the job takes ~1-3 min to actually drain (the reservation is torn down without ever becoming billable). Releasing the slot at stop() therefore bounded nothing: with the cap at 10 and 8 workers, each slot recycled ~20x inside a single job's counted lifetime, the suite peaked at ~37 concurrent jobs, and it tripped ResourceLimitExceeded at a utilization of 21 against the limit of 20. _wait_until_terminal closes that gap by holding the slot across the drain, so the cap bounds what the service actually counts. With the fix, live counted concurrency stayed at 4-5 against a cap of 10 for the whole run. The cost is runtime: holding to terminal makes the suite's floor roughly (#jobs * drain) / cap. At ~83 jobs, a ~75s median drain and cap 10 that is ~8-13 min, versus ~2 min if slots released early -- but that fast run is the one that breaches the quota. This is the batches-of-10 behaviour: at most 10 jobs counted at once. Mechanism job_slots() in harness.py, held by submitted() and assert_rejected() until the job is terminal. Slots are O_EXCL-created files under a run-keyed temp directory; xdist workers are separate processes, so an in-process semaphore would bound nothing. Keyed on PYTEST_XDIST_TESTRUNUID (falling back to the parent pid) so two concurrent local runs get separate budgets rather than deadlocking, and a stale directory from a killed run is never mistaken for live slots. Details that matter: * both waits proceed with a warning rather than failing -- acquiring a slot waits up to 900s, _wait_until_terminal up to 300s -- since the cap is a courtesy to the quota, not an assertion about the SDK, and a leaked slot or stuck drain should mean a slower run rather than a red build; * status is read per job type (training_job_status / job_status / hyper_parameter_tuning_job_status), since the SDK is not consistent, and a job that exposes no status releases its slot immediately rather than hanging; * a request larger than the cap is clamped, so a single test cannot deadlock against itself; * enforced in the harness rather than per test, so a new test is capped by default instead of by remembering to opt in. Default 10, overridable via SHALLOW_MAX_CONCURRENT_JOBS; 0 disables gating for a single-worker debugging run. Set explicitly in the workflow so the ceiling is visible at the call site rather than only in a Python default. Verified * Slot mechanism holds under contention: 12 processes x 4 iterations against cap=3, observed peak exactly 3, never 4; slots released on the happy path, on exception, and with correct multi-slot accounting; cap=0 takes none; an oversized request clamps without deadlocking. * Terminal-hold bounds what the service counts: a multi-process simulation where each job stays "counted" past stop() peaked at exactly the cap (3) with 10 workers, versus the pre-fix design that would have peaked far higher. * _wait_until_terminal waits through non-terminal states, releases on terminal, honours each job type's status attribute, and returns rather than hanging on None / a read error / a timeout. * Full suite green with the fix: 83 passed, 1 skipped in 810s (13:30), zero ResourceLimitExceeded, live counted concurrency 4-5 throughout, account fully drained afterward. * docs(train): drop account IDs from the shallow suite's comments This is a public repo, so the comments should not name internal test accounts. Every reference was explanatory -- "the deep test hardcodes a bucket in account X, which other accounts cannot read" -- and the point it makes is that the bucket belongs to *one specific account*, not which account that is. Reworded to say that instead, keeping each rationale (and the verified AccessDenied finding) intact. Comments and docs only; no functional change. Test resource ARNs still name the account they actually live in, since resolving them is what the tests do, and that already matches the convention in the surrounding suite. * change(train): use a bare model package group name in both regions Review feedback: recipe_cases.py pinned MODEL_PACKAGE_GROUP to a full ARN while the Nova path used a bare NOVA_MODEL_PACKAGE_GROUP, for the same group. The bare name is the better form on both paths, so the two constants collapse into one. The SDK accepts either -- _resolve_model_package_group_arn() returns an ARN unchanged and otherwise resolves a name via ModelPackageGroup.get() against the *session's* region -- so a name is region- and account-portable where an ARN pins both. Pinning the region is what forced the split in the first place: passing the us-west-2 ARN to a us-east-1 Nova job is rejected with "Model package group ARN region 'us-west-2' does not match expected region 'us-east-1'". One name serves both regions and drops a hardcoded account ID from a public repo. Verified: the bare name resolves to the same ARN via DescribeModelPackageGroup in us-west-2, and the us-west-2 recipe path still submits -- SFT, DPO and RLVR minimal-request tests pass against the service (3 passed). Collection unchanged at 100 tests. * test: resolve the shallow suite's CPU training image per region `CPU_IMAGE` hardcoded a us-west-2 URI in the public DLC account. Replace it with `cpu_image(sagemaker_session)`, which resolves the same image in the session's own region through `image_uris.retrieve` -- the resolver the SDK's framework estimators already use, so this is the supported mapping rather than a reconstruction of it. The registry account is not constant, which is what makes the hardcoded form actually wrong rather than merely untidy: it is 763104351884 across the commercial regions but 442386744353 in GovCloud and 727897471807 in China (on .com.cn). A pinned URI is unusable outside one partition, and it fails as an ECR error from the backend's role-assuming validators, which reads like a test bug rather than a hardcoded constant. A function rather than a constant because it needs the session's region; all three call sites already had a session in scope. Verified against AWS: reproduces the previously hardcoded URI byte-for-byte in us-west-2, and returns the correct in-region host (and per-partition registry) in us-east-1, eu-west-1, ap-northeast-1, us-gov-west-1 and cn-north-1. The affected tests pass on a real account -- 10 passed in 88s, covering the ModelTrainer helper, the raw TrainingJob.create path, and the tuner. * fix(train): bring the tuner path inside the shallow concurrency cap `_tuning()` submitted via `tuner.tune()` without acquiring slots, because a tuning job is stopped through `tuner.stop_tuning_job()` rather than `stop_quietly` and so never went through `submitted()`. Meanwhile the `DEFAULT_MAX_CONCURRENT_JOBS` note, the README quota table and `_requested_slots` all described the tuner as being inside the cap. It wasn't. Wrap it in `job_slots()` and drain after stopping. Slots are sized from the tuner's `max_parallel_jobs`, not a compute block: a tuning job occupies instance quota through the child training jobs it launches, which is also why `_requested_slots` cannot size this and `_tuning()` requests its own. The drain matters for the same reason it does elsewhere -- `stop_tuning_job()` returns while the job is still `Stopping` and its children are still tearing down, so releasing there is the release-before-terminal pattern that caused the ~37-concurrent breach. `_STATUS_ATTRS` already carried `hyper_parameter_tuning_job_status`, so the waiter handled this job type already; nothing ever called it with one. Renamed `_wait_until_terminal` -> `wait_until_terminal`. A test module outside the harness now needs it, and no other test imports a private name from there. Real impact today is small and worth saying so: both tuner tests are `max_parallel_jobs=1`, so this is 1 slot each. It is wired up because the cost is one context manager and the failure mode otherwise is silent -- a future test raising `max_parallel_jobs` would consume capacity outside a cap that still claimed to bound it. Verified: 3 unit scenarios (slots held through tune -> stop -> drain and released after; a failing test body still stops and releases; a missing or None `max_parallel_jobs` yields 1 slot, never an unbounded 0), plus a real run -- 2 passed in 28s, both jobs logging "reached Stopped; releasing slot" with no drain timeout. Docs corrected in all three places that overclaimed.
…6187) * feat(train): Add inherited list_supported_models to BaseTrainer Expose a list_supported_models capability on all fine-tuning trainers (SFT, RLVR, RLAIF, DPO, CPT) mirroring MultiTurnRLTrainer. Rather than duplicating the method per trainer, add a single inherited classmethod on BaseTrainer that resolves cls._customization_technique and delegates to the existing recipe_utils._list_hub_models_by_recipe primitive. RLAIFTrainer previously used the technique inline; add its _customization_technique class attribute so the inherited method resolves. MultiTurnRLTrainer keeps its own override (CreateJob flow, non-enum technique). Add unit tests covering per-trainer technique resolution, the base delegation path, and the missing-technique guard. --- X-AI-Prompt: Add list_supported_models to SFT/RLVR/RLAIF/DPO trainers like MTRL; refactored to a shared BaseTrainer method X-AI-Tool: Kiro * test(train): Add parametrized integ test for list_supported_models Add an integration test that queries SageMakerPublicHub for each fine-tuning trainer (SFT, RLVR, RLAIF, DPO, CPT) and asserts the inherited list_supported_models returns a non-empty, sorted list of model names. This closes the contract gap unit tests cannot cover: that each trainer's _customization_technique string matches the live hub recipe keywords (@recipe:finetuning_{technique}_...). Mirrors the existing MTRL integ test. --- X-AI-Prompt: Add a parametrized integ test for list_supported_models across all fine-tuning trainers X-AI-Tool: Kiro * fix(train): Match suffix-less recipe keywords in model listing _list_hub_models_by_recipe built the search keyword as "@recipe:{type}_{technique}_" (trailing underscore) and matched by prefix, assuming every recipe keyword carries a "_{strategy}" component. Techniques with no strategy — CPT is tagged as the bare "@recipe:finetuning_cpt" — never matched, so CPTTrainer.list_supported_models() always returned an empty list despite the hub having CPT-tagged models. Match the bare base keyword OR the "{base}_{strategy}" form. The "{base}_" guard prevents a shorter technique from matching a longer one that shares its prefix (e.g. "rl" must not match "rlvr"). Rework the integ test into a hub-content-agnostic oracle: independently scan the active hub and assert list_supported_models returns exactly the tagged set per technique (0 or many), instead of a brittle non-empty assertion that broke under the private test hub which carries no CPT models. Add unit coverage for the bare-keyword match and the prefix-collision guard. Verified against SageMakerPublicHub: SFT=36, RLVR=33, RLAIF=31, DPO=32, CPT=5 models, each matching the independent oracle. --- X-AI-Prompt: Integ test surfaced CPT list returning empty; fixed suffix-less recipe keyword matching and made the integ test hub-agnostic X-AI-Tool: Kiro
…) (aws#5964) * fix(tgi): honor S3 model_path as weight source for TGI builds ModelBuilder with ModelServer.TGI silently ignored an S3 weight source (model_path="s3://..." or s3_model_data_url="s3://..."): it created a literal local "s3:/..." directory and set HF_MODEL_ID to the HF repo id, so the container always downloaded weights from huggingface.co. _build_for_tgi now detects an S3 weight source, skips the local mkdir for it, attaches the S3 prefix as an uncompressed ModelDataSource, and sets HF_MODEL_ID=/opt/ml/model and HF_HUB_OFFLINE=1 (via setdefault, preserving any user-supplied HF_MODEL_ID). Genuine local paths, HF-Hub downloads, JumpStart, and all non-TGI servers are unchanged. Also fixes two defects surfaced during real deployment: - HF_HUB_OFFLINE was reset to "0" at the end of the build; it now stays "1" for the S3-mounted path so TGI loads from /opt/ml/model. - The uncompressed ModelDataSource S3Uri could end in "//" when the input prefix already had a trailing slash; normalized to exactly one slash so S3Prefix matching finds the weight objects. Adds unit and regression tests for all of the above. * fix(tgi): distinguish S3 model source from upload destination --------- Co-authored-by: Sagar Dubey <dubeysag@amazon.com> Co-authored-by: Syed Mujtaba <42322958+mujtaba1747@users.noreply.github.com>
…ts (aws#6194) * fix: Made CPT integ tests dry run for optimize for capacity constraints * fix: add dryrun to test name for visibility in logs --------- Co-authored-by: Syed Jafri <syedjfr@amazon.com>
…ws#6195) custom_extractall_tarfile falls back to filtering members with _get_safe_members when tarfile.data_filter is unavailable (Python < 3.12, before the 3.9.17 / 3.10.12 / 3.11.4 backports). That fallback had two containment defects that combined to let an archive member be written outside the extraction directory: - _get_safe_members anchored its check to the process working directory (_get_resolved_path("")) rather than the directory the archive is extracted into. It now takes the base as an argument, and custom_extractall_tarfile passes the resolved extract_path. - _is_bad_path compared paths with str.startswith, so a sibling directory sharing a textual prefix with the base (e.g. base "/tmp/extract" and "/tmp/extract-evil/f") was treated as contained. Containment is now checked with os.path.commonpath, via a shared _is_within_base helper that _validate_extracted_paths uses as well. Absolute member paths are now rejected outright, since joinpath would otherwise silently discard the base for them. Both defects were required for the escape: relative member resolution is base-independent under normpath, so a plain "../../x" member does not escape on its own. The working-directory anchoring is what made a prefix-matching sibling ("<cwd>" vs "<cwd>evil") pass validation while extraction still wrote outside extract_path. _validate_extracted_paths only walks extract_path, so it did not catch the escape either. This is the shared utility every v3 extraction path routes through (model unpack, local/image.py, serve TGI and DJL prepare, pipeline repack). An already-correct implementation of the same logic exists in sagemaker-mlops/src/sagemaker/mlops/workflow/_repack_model.py; this brings common_utils.py in line with it. Adds regression tests covering the end-to-end escape, the sibling-prefix bypass, absolute members, and that validation is anchored to extract_path.
…e and stop telemetry from blocking SDK calls (aws#6197) * feat(mlops): allow specifying region in feature_store ingest_dataframe ingest_dataframe() had no way to say which AWS region the FeatureGroup lives in, so both the DescribeFeatureGroup call and the record writes fell back to whatever region boto3 resolved, with no caller control. Add an optional `region` argument and thread it through the whole ingestion path: - feature_utils.ingest_dataframe -> CoreFeatureGroup.get(region=...) and IngestionManagerPandas(region=...) - IngestionManagerPandas gains a `region` field, forwarded to put_record(region=...) and batch_write_record(region=...) in the single-thread, multi-thread, multi-process, and BatchWriteRecord paths `region` is appended last and defaults to None, so existing calls and their behavior are unchanged. * fix(core): stop telemetry from blocking SDK calls Telemetry could add unbounded latency to any decorated SDK call. A Feature Store ingest was reported taking ~47 minutes from a private VPC while the underlying PutRecord completed server-side in 348ms; all of the remaining time was spent in the two telemetry emissions that follow the call. Three defects, all in the emission path: 1. `_requests_helper` passed the timeout positionally. `requests.get` takes `params` as its second positional argument, so the value was appended to the query string and the request had no timeout at all. From a VPC with no route to the telemetry endpoint the GET hung until a network device dropped the flow. Now passed as `timeout=`, verified bounded at ~2s against a black-holed address. The same one-line defect existed in sagemaker-serve's telemetry_logger and is fixed there too. 2. `_get_default_sagemaker_session` hardcoded us-west-2. Module-level functions such as `ingest_dataframe` have no session of their own, so the decorator synthesizes one, which pointed both the STS `get_caller_identity` call and the telemetry GET at a region the caller may have no route to. The region is now resolved by boto3 from the caller's environment, with the default kept only as a last resort since `Session` requires a region. 3. Emission was synchronous. `_send_telemetry_request` now dispatches to a daemon thread and returns immediately, so neither the STS call nor the GET can sit in the caller's critical path. Daemon threads are killed at interpreter exit, so a pending send cannot delay shutdown either. In-flight sends are capped and excess events dropped rather than growing threads without bound, and nothing can escape the thread. The existing body moved to `_send_telemetry_request_sync`. Telemetry request failures now log at debug instead of logging a full traceback at error level; a best-effort metric should not look like an error. Note: events queued at process exit may now be lost. That is the intended trade-off for never blocking the caller. * fix(core): never drop telemetry events when sending asynchronously Removes the in-flight cap added alongside the async send. Telemetry events are data we cannot silently lose, so every event now gets its own daemon thread rather than being dropped once eight sends are outstanding. Feature Store ingestion is the special case that motivated moving the send off the caller's thread at all: it is decorated at more than one level (ingest_dataframe and IngestionManagerPandas.run), so a single user call emits several events, and sending them serially turned an ingest the service finished in under a second into a multi-minute wait.
* fix: update timeout of RLVR sequence_length test * fix: add xdist_group to Nova Bedrock deployment tests * Address rlaif trainer fix * Address rlaif trainer fix --------- Co-authored-by: Roja Reddy Sareddy <rsareddy@amazon.com>
Add root entrypoints so the major AI coding agents pick up the repo's existing AGENTS.md guidance: - CLAUDE.md — Claude Code (imports AGENTS.md via @import) - GEMINI.md — Gemini CLI (imports AGENTS.md via @import) - .github/copilot-instructions.md — GitHub Copilot (prose pointer; Copilot instructions do not support file imports) AGENTS.md remains the single source of truth. Codex, Cursor, Windsurf, Aider, Zed, etc. already read AGENTS.md natively and need no extra file. --- X-AI-Prompt: Replicate AGENTS.md for Claude, Gemini, and Copilot; raise/update PR X-AI-Tool: Kiro
…R cases a reward signal (aws#6207) * 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 aws#6181 on 2026-08-14, five days before the shallow suite merged (aws#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 aws#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 * 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
Co-authored-by: Roja Reddy Sareddy <rsareddy@amazon.com>
…ws#6149) * feat(train): add list_hyperparameters() for pre-trainer HP discovery Add a public utility function that returns available hyperparameters for a model/technique/training_type combination without requiring a fully constructed trainer object. This enables tools and scripts to discover valid hyperparameter names, defaults, and ranges before setting up training infrastructure (model package groups, datasets, roles, etc.). Motivation: COE 398545 identified that hardcoded HP names in downstream consumers break when recipe templates rename parameters. Dynamic discovery at code-generation time prevents this class of failure. Usage: from sagemaker.train import list_hyperparameters hp = list_hyperparameters('model-name', 'SFT', 'LORA') hp.get_info() # display all params hp.get_info('learning_rate') # display one param * test: use meta-textgeneration-llama-3-2-1b-instruct in integ tests Switch to the same model used by ~80% of existing integ tests to avoid deprecation risk. Llama 3.2 1B is the most battle-tested model in the repo's test infrastructure. --------- Co-authored-by: Joshua Towner <josh@tonwer.xyz>
* fix(ci,train): stop integ-tests rerunning the shallow suite 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 * docs(train): drop --dist loadfile from the shallow suite's README 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
…/RLVR) (aws#6213) When a PipelineSession is passed as sagemaker_session, the serverless training path in SFTTrainer, DPOTrainer, RLAIFTrainer, and RLVRTrainer now intercepts the CreateTrainingJob request and returns step arguments instead of immediately launching a training job. This enables V3 trainers to be used with SageMaker Pipelines TrainingStep, matching the existing behavior of ModelTrainer, Processor, Transformer, and HyperparameterTuner. The fix follows the established SDK pattern: isinstance check for PipelineSession, call _intercept_create_request with the request args, and return session.context (the captured step arguments). Fixes: aws#6163 Co-authored-by: nayan3107 <nayancho@amazon.com>
…SV path (aws#6212) DatasetBuilder defined _register_as_hub_content_dataset and the register_as_dataset flag, but _to_csv_from_feature_group never invoked the helper. Wire the call into the CSV extraction path, gated on register_as_dataset, passing the Athena QueryExecutionId. Add unit tests asserting the helper is invoked when the flag is set and skipped when it is not. Co-authored-by: Vishakha Nerkar <vnerkar@amazon.com>
…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 aws#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
…pytorch (aws#6220) * change: add image_uri_config for DLC serving frameworks Add image_uri_config entries for the AWS Deep Learning Containers serving frameworks, using the whole-tag (channel / amzn2023) pattern so image_uris.retrieve() returns the image tag verbatim. Channel configs expose each major tag and its latest minor (no patch), with a latest alias: - vllm-server (vllm repo): server-sagemaker-cuda v1, v1.4, v2, v2.4 - vllm-omni (vllm repo): omni-sagemaker-cuda v1, v1.6 - sglang-server (sglang repo): server-sagemaker-cuda v1, v1.3 - llama-cpp: server-sagemaker-cuda v1, v1.0 - llama-cpp-arm64: server-sagemaker-cpu v1, v1.0 - ray-serve (ray repo): serve-ml-sagemaker-cuda v1, v1.4 - whisperx: 3.8-cu128-amzn2023-sagemaker Adds tests/unit/image_uris/test_dlc_serving_frameworks.py. * change: add amzn2023 unified pytorch repo to training image_uri_config Add PyTorch 2.11/2.12/2.13 training entries for the amzn2023 unified `pytorch` ECR repo (distinct from pytorch-training/pytorch-inference). Their tags encode CUDA directly, e.g. 2.13-cu133-amzn2023-sagemaker, with no "gpu" token, so a new optional version-config key "processor_in_tag" (default true, backward compatible) lets retrieve() drop the cpu/gpu processor token while still using the processor to select container_version. Both resolve: 2.13 + gpu instance -> pytorch:2.13-cu133-amzn2023-sagemaker 2.13 + cpu instance -> pytorch:2.13-cpu-amzn2023-sagemaker - image_uris.py: honor "processor_in_tag": false - pytorch.json: add 2.11/2.12/2.13 training versions (repository "pytorch") - test_pytorch_al2023.py: cpu/gpu coverage + regression that existing pytorch-training tags are unchanged * change: drop vllm-server v1 from image_uri_config (keep v2, v2.4) * change: split amzn2023 pytorch into pytorch-amzn2023; add Ubuntu 2.9/2.10 Move the amzn2023 unified `pytorch` repo out of pytorch.json into its own framework `pytorch-amzn2023` (pytorch-amzn2023.json, training, 2.11/2.12/2.13, cpu+gpu via processor_in_tag). This keeps the existing `pytorch` (Ubuntu pytorch-training) no-version default on pytorch-training instead of shifting it onto the amzn2023 repo. Also add Ubuntu pytorch-training 2.9.0 (py312) and 2.10.0 (py313) to pytorch.json training. Replaces test_pytorch_al2023.py with test_pytorch_amzn2023.py (cpu/gpu coverage + a regression that the pytorch training default stays on pytorch-training). --------- Co-authored-by: Yadan Wei <weiyadan@amazon.com>
* change: add image_uri_config for vLLM Add image_uri_config/vllm.json for the AWS Deep Learning Containers vLLM GPU inference images (py312), versions 0.11.0 through 0.28.0. Image tags follow <version>-gpu-py312-cuNNN-ubuntuNN.NN-sagemaker. Add tests/unit/image_uris/test_vllm.py covering image_uris.retrieve() for all version/region combinations, exact URIs for representative regions, version-alias resolution, and rejection of unsupported versions. * change: limit vLLM image_uri_config to version 0.28.0 Keep only version 0.28.0 in image_uri_config/vllm.json and its 0.28 alias (drop the 0.11.0-0.27.1 backfill). Update test_vllm.py accordingly: with a single version the SDK defaults to it rather than raising, so the unsupported-version assertion no longer applies and is removed. * change: drop ADC (aws-iso) regions from vLLM image_uri_config Remove the isolated/dedicated-cloud (ADC) registries from image_uri_config/vllm.json: us-iso-east-1, us-isob-east-1, us-isof-east-1, us-isof-south-1, and eu-isoe-west-1. GovCloud, China, EUSC and commercial regions are retained. * change: drop EUSC region from vLLM image_uri_config Remove eusc-de-east-1 from image_uri_config/vllm.json so the region set matches the established pytorch footprint (38 regions: commercial, GovCloud, and China). * change: add image_uri_config for SGLang Add image_uri_config/sglang.json for the AWS Deep Learning Containers SGLang GPU inference image (version 0.5.18, Python 3.12). The image tag follows <version>-gpu-py312-cuNNN-ubuntuNN.NN-sagemaker. The region set matches the vLLM/pytorch footprint (38 regions). Add tests/unit/image_uris/test_sglang.py covering image_uris.retrieve() across all regions, the exact URI for representative regions, and version-alias resolution. --------- Co-authored-by: Yadan Wei <weiyadan@amazon.com>
* feat(sagemaker-core): Add botocore-sync GitHub workflows Enable the daily botocore-sync automation for the sagemaker-core module in the monorepo: - Add scheduled sync (sagemaker-core-botocore-sync.yml), auto-approve, and auto-merge workflows. They trigger the CodeBuild projects via the repo's existing CI_AWS_ROLE_ARN OIDC role; auto-merge is gated to sagemaker-bot + botocore-sync* branch + "Daily Sync with Botocore" title, scoped to sagemaker-core/sample/**/*.json. - Remove a stray hardcoded absolute SERVICE_JSON_FILE_PATH override in data_extractor.py so codegen resolves service models via the package-relative constant in constants.py. * chore(sagemaker-core): Defer auto-approve/merge sync workflows Remove the auto-approve and auto-merge workflows for now so the daily botocore sync can be validated in isolation: the sync workflow opens a PR that is left for manual review. The approve/merge automation will be re-added in a follow-up once PR generation is verified. Both files remain in history (commit 99ac5f7) and can be restored when needed. --------- Co-authored-by: Roja Reddy Sareddy <rsareddy@amazon.com>
…ws#6231) Docker Compose installed via Homebrew reports its version without a leading 'v' (e.g. "Docker Compose version 2.22.0"). The detection regex `v(\d+)` required a literal 'v' before the digits, so `_get_compose_cmd_prefix` failed to recognize a valid Compose v2+ plugin in that case. Change the regex to `version\s+v?(\d+)`, making the 'v' optional and anchoring to the "version" keyword so a stray number elsewhere in the output can't cause a false positive. v1 is still correctly rejected. Applied to all three v3 copies (core local image, core modules local container, train local container) with regression tests for the no-'v'-prefix format. Fixes aws#4137 Co-authored-by: Mohamed Zeidan <zeidmo@amazon.com>
…ng 2.21 (aws#6230) * change: add image_uri_config for TensorFlow inference 2.20 and training 2.21 Register the newly released SageMaker TensorFlow DLCs in tensorflow.json: * inference 2.20.0 -> tensorflow-inference:2.20.0-{cpu,gpu}-py312, plus the 2.20 minor alias * training 2.21.0 -> tensorflow-training:2.21.0-{cpu,gpu}-py312, plus the 2.21 minor alias Both are Python 3.12 / AL2023 builds, GPU variants on CUDA 12.9.1. The registry maps match the 38 regions already used by 2.19.0. Unlike earlier inference entries, 2.20.0 sets py_versions to ["py312"] because the published tags carry the Python suffix; the bare 2.20.0-cpu form used by 2.19 and earlier is not published for this release. Add tests/unit/image_uris/test_tensorflow.py, the first image-URI tests for the TensorFlow training and inference scopes, following the config-driven pattern used by test_vllm.py and test_sglang.py. * change: strengthen TensorFlow image_uri_config tests Address review feedback on the new TensorFlow image-URI tests: * Assert that the version covered by this file is the newest one registered in tensorflow.json, so adding a newer version fails with a message naming it. The previous membership check let a new version go silently uncovered. * Assert the new version's registry map against the previous release's rather than against its own entry. Reading the expected account out of the config under test meant a wrong account or a dropped region could never fail. * Cover the Python-version behaviour of the new inference entry: py312 is the only option for 2.20, and 2.19 still resolves to a tag with no py suffix. * Drop the incidental trailing newline added to tensorflow.json. * change: clarify TensorFlow image_uri_config test comments Correct the FULL_URI_REGIONS rationale: the registry map contains no ISO partition regions, and cn-north-1 is the only listed region with a non-default ECR domain. Narrow the REGISTRY_REFERENCE_VERSION comment to what the assertion actually guarantees. It catches a one-sided mistake in a new version's registry map, not a change applied to both versions, and it does not verify that the images are published in those regions. Also note that bumping it to the newly added version makes the account assertions tautological. Comments only; no assertions changed. --------- Co-authored-by: Bhanu Teja Goshikonda <bhanugk@amazon.com>
…to device-selectable configs (aws#6229) The DLC serving-framework image_uri_configs added in aws#6218/aws#6220 exposed only GPU (cuda) images. DLC also publishes CPU images for ray-serve and llama-cpp; expose them, and prepare the remaining GPU-only frameworks so a CPU variant can be added later without changing how GPU callers resolve. All serving configs now use the image_uris processor schema (processors + processor_in_tag:false + a per-processor container_version tail) instead of a verbatim whole-tag: - ray-serve, llama-cpp: processors=[cpu, gpu]; instance_type selects the device. GPU tags unchanged; adds ray:serve-ml-sagemaker-cpu-v* and llama-cpp:server-sagemaker-cpu-v*. - vllm-server, vllm-omni, sglang-server, whisperx: processors=[gpu] only. Resolution is byte-identical to before (locked by literal-tag tests) and instance_type stays optional. Adding a CPU image later is a data-only change. Behavior change: for ray-serve and llama-cpp, instance_type is now required (previously defaulted to the GPU tag). For the GPU-only configs, a non-GPU instance type now raises instead of silently returning the GPU image. Both are safe: these configs shipped only in aws#6218/aws#6220. llama-cpp-arm64 (arm64 CPU, separate repo) is unchanged; select it by framework name. Tests restructured into whole-tag / gpu-only / multi-processor tiers with cpu+gpu coverage, required/optional instance_type checks, and literal repo:tag pins. Co-authored-by: Yadan Wei <weiyadan@amazon.com>
…s#6227) * feat(train): validate raw base model name exists in SageMaker Hub When a user passes a raw base model name to a V3 trainer (SFT/DPO/RLVR/ RLAIF/CPT/MTRL), model resolution now confirms the model actually exists in the SageMaker Hub before the job proceeds. A bogus or misspelled name fails fast with a clear error that points at list_supported_models(), instead of a later, more opaque failure during recipe resolution. The check runs in _resolve_model_and_name, the shared resolve path the trainer interfaces already use, so it also covers the base_model_name supplied with an S3 checkpoint. It issues a single DescribeHubContent against the active hub. Only a definitive not-found raises; transient or permission errors are logged and skipped so a Hub hiccup never blocks an otherwise-valid training job. Adds unit tests for the classifier and the resolve integration, plus an autouse conftest that no-ops the Hub check for trainer construction tests (which use placeholder model names against mock sessions). --- X-AI-Prompt: Add Hub-availability validation when a raw base model name is passed to trainer resolve_model path X-AI-Tool: Kiro * test(train): add integ test for base model Hub availability check Validates against the live SageMaker Hub (prod us-west-2) what the mocked unit tests cannot: that a real DescribeHubContent miss surfaces as an error the not-found classifier recognizes, so the check fail-closes with a clear error instead of fail-opening on an unexpected error shape. Two cases: a real FineTuning-tagged model (picked via an independent hub scan, skipped if none) passes validation and resolves; a bogus name raises the "not available in SageMaker Hub" ValueError, both directly and through the shared resolve path. --- X-AI-Prompt: Add an integration test validating the Hub availability check against the live SageMaker Hub X-AI-Tool: Kiro * test(serve): use p5 instance type for Nova customization integ tests The Nova Lite fine-tuning recipe (nova_lite_2_0_p5_gpu_lora_sft) only accepts ml.p5.48xlarge; the hardcoded ml.g6.48xlarge now fails build-time instance-type validation with "Instance type 'ml.g6.48xlarge' not supported ... Supported: ['ml.p5.48xlarge']", breaking the trainer-build, instance-type-autodetect, and deploy cases in the shallow integ suite. Update the shared constant to the supported type. --- X-AI-Prompt: Fix drifted Nova instance type (g6->p5) in the Nova customization deployment integ tests X-AI-Tool: Kiro
) Training (and feature_store) role resolution only inferred a role from the caller identity, so a caller authenticating as an IAM user or the account root - whose identity has no backing role - hit "No IAM role could be resolved from your caller identity" and was forced to pass role= on every call, even when a default execution role was configured in the SageMaker intelligent-defaults config. resolve_and_validate_role now consults the config default (SageMaker.TrainingJob.RoleArn / FeatureGroup.RoleArn) before falling back to caller-identity inference, matching the pattern already used by processing and model monitor. Resolution order is now: explicit role -> config default -> caller identity -> raise. Behavior for a bare IAM user with no configured default is unchanged (still raises the same error). --- X-AI-Prompt: Deep-dive RLVR trainer default-role resolution failing for IAM users; add a sagemaker-config default-role fallback X-AI-Tool: Kiro
aws#6217) * evaluator_model was checked against a hardcoded _ALLOWED_EVALUATOR_MODELS dict (model -> regions) in sagemaker/train/constants.py. That list is triplicated across clients and goes stale: when a judge model reaches end of life it still passes client-side validation, so the eval job spins up and only fails deep inside the in-container Bedrock CreateEvaluationJob call, wasting compute and surfacing a poor error. Replace it with two-step validation against authoritative sources: - Construction: fetch the service-maintained supported-judge-models list at s3://jumpstart-cache-prod-<region>/fmhMetadata/supported-llmaj-judge-models.json and fail fast if evaluator_model is not a supported judge model. - evaluate(): call bedrock:GetFoundationModel and fail fast if the model is unavailable in the region or past its endOfLifeTime. The lookup is gated on the caller's IAM permission via a new non-raising caller_can_perform() helper that mirrors the existing SimulatePrincipalPolicy caller-check pattern (verify_evaluation_caller_permissions). Both steps degrade gracefully instead of blocking: if a source can't be read (missing bedrock:GetFoundationModel permission, unreadable list, or a transient error) the SDK logs an actionable warning with a link to the supported models and continues. - Remove _ALLOWED_EVALUATOR_MODELS from sagemaker/train/constants.py - Add caller_can_perform() to sagemaker/core/helper/iam_role_resolver.py - Add unit tests for both validation steps and caller_can_perform * additions * added integ tests * test(train): make LLM-as-Judge lifecycle integ tests permission-aware and self-provisioning The retired-model lifecycle assertion required the runner to hold bedrock:GetFoundationModel; CI identities that lack it correctly degrade (warn, don't block), so the hard-raise assertion failed there. - Rename test_retired_model_fails_lifecycle_check -> test_retired_model_lifecycle_enforced_or_degrades and make it tolerate both outcomes under the ambient identity (raise if permitted, warn if not). - Refactor the restricted-role fixture into a policy-parameterized _assumed_role_session() context manager; the positive bedrock-permission test now provisions a role that GRANTS bedrock:GetFoundationModel (deterministic enforce) and the negative provisions one that lacks it (deterministic degrade). Both skip cleanly without iam:CreateRole / sts:AssumeRole. --------- Co-authored-by: Mohamed Zeidan <zeidmo@amazon.com>
* change: emit a JumpStart flag in ModelBuilder telemetry Record whether ModelBuilder build and deploy calls use a JumpStart model ID. This flag lets downstream analytics identify JumpStart usage. --- X-AI-Prompt: Can you add a JumpStart identity flag to ModelBuilder telemetry and raise a pull request? X-AI-Tool: claude-code * change: emit the JumpStart model ID in ModelBuilder telemetry The isJumpstartModelId flag separates JumpStart traffic from other traffic, but it does not name the model. Analytics cannot rank JumpStart models by build count or deployment count from a boolean. Add a jumpstartModelId param to the build and deploy telemetry param lists. The param emits the model ID string when the model source is a JumpStart model ID, and emits nothing for another model source. The ATTR_CALL branch of the param extractor drops a None return, so a param that has no value stays out of the beacon. --- X-AI-Prompt: Can we track the JumpStart model ID itself in telemetry instead of only a boolean flag? X-AI-Tool: claude-code * change: remove the JumpStart boolean telemetry param The model ID identifies JumpStart traffic and the exact model. The boolean param duplicates this information and adds a second field for consumers. Remove the boolean param from ModelBuilder telemetry and keep only jumpstartModelId. --- X-AI-Prompt: Can you remove the boolean flag and keep only the JumpStart model ID telemetry param? X-AI-Tool: claude-code * fix(serve): obey telemetry test lint rules Add the necessary future import and the blank line for the lint gate. --- X-AI-Prompt: Can you fix the lint errors in my pull request and push the fix? X-AI-Tool: claude-code
Brings the branch 30 commits forward to master tip 8e7485a. No conflicts; the branch's delta against master is unchanged (the 22 instance-preferences files). Restores sagemaker-train/tests/integ/train/shallow, which the fast-integ-tests check runs and which the branch predated.
deeppcs
requested a deployment
to
manual-approval
September 10, 2026 03:10 — with 
GitHub Actions
Waiting
GitHub Actions
Waiting
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
17 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Squashing flattens this to a single-parent commit, master stops being an ancestor, and the branch reads as 30 behind again.
Description
Brings
feature-smtj-instance-preferences-latestup to master tip8e7485a1(30 commits) ahead of #6246.Merge of master into the branch; no conflicts. The branch's delta against master is unchanged: the 22 instance-preferences files, +1,923/−18.
This also fixes the
fast-integ-testsfailure on #6246: that check runssagemaker-train/tests/integ/train/shallow, which was added to master on 2026-08-24 and which the branch predated, so pytest collected nothing and exited 5. The directory is present after this merge (14 files, 100 tests collect).Testing
MaxPendingTimeInSecondsmin: 1800, both docs pages, the Spark processor plumbing,validate_instance_preferences, and the integ tests.sagemaker-coreunit tests (processing, compute configs, codec) and 64model_trainerunit tests pass on the merged tree.Context on the other failing checks on #6246
None are caused by this branch: the four
codestyle-doc-testsjobs fail identically on every recent PR to master (black wants to reformat ~2,000 files; pylint scores below the 9.9 gate on pre-existing code);integ-tests-us-east-1fails on a Nova recipe/instance mismatch and a BedrockCreateCustomModelquota;integ-tests (sagemaker-serve)hitsModelBuilder._cached_compute_requirementsbeing unset on thereuse_resources=Truepath (model_builder.py:6371, only assigned at:1347), plus a 3-hour credential expiry. This branch changes no files undersagemaker-serve.