fix(instance-preferences): run integ tests in CI, reject managed spot client-side, stabilize IntPipeVar codegen

[deeppcs]

Description

Addresses the review feedback on #6249. Targets feature-smtj-instance-preferences-rebased so #6249 updates in place.

Integ tests now run in the standard CI account. Both tests were gated on account-specific environment variables that no CI configuration sets, so every run skipped them and the suite passed without exercising the feature. Now:

• Training coverage lives in sagemaker-train/tests/integ/train/shallow/test_instance_preferences.py, which fast-integ-tests runs on every pull request: two accepted cases (uniform and per-preference counts — submit, Describe echo, stop) and four rejected cases (instance_type, managed spot, and duplicate types client-side; the five-entry cap server-side). The environment-gated deep test is removed.
• The processing test uses the suite's SageMakerRole and resolves its image via image_uris, like the other sagemaker-core integ tests.
• Candidate types remain overridable via *_TEST_INSTANCE_TYPES for accounts whose defaults lack quota. No account-specific values remain in the repository.

Managed spot is rejected client-side. The documentation already lists it as mutually exclusive with instance_preferences, but validate_instance_preferences only checked instance_type, instance_groups, and instance_placement_config. Both copies now reject an explicit enable_managed_spot_training=True; False/unset stay valid.

IntPipeVar is codegen-stable. The widening on the five instance-preferences count members was a hand edit that generate_shapes could not reproduce — only ResourceConfig.InstanceCount was in PIPE_VAR_OVERRIDES, so the next regeneration would have narrowed the rest to Optional[int]. All five are registered, and a unit test asserts the extractor emits IntPipeVar for each, so a dropped override fails loudly instead of silently.

Testing

In us-west-2, with no environment variables beyond region:

• New shallow tests: 6/6 pass. Under the exact CI command (pytest tests/integ/train/shallow -n 8 -m "not gpu_intensive and not us_east_1") all six pass alongside the existing suite.
• Processing integ test: pass, winner resolved.
• Unit: 196 sagemaker-core (compute configs, processing, codec, service model, shapes extractor), 111 sagemaker-train (model_trainer, defaults). The new extractor test was mutation-checked: removing one override fails exactly that member's case.

The fast-integ-tests run on this PR is the CI execution proof requested on #6249.

[deeppcs]

Superseded by the master-targeted PR carrying the same two commits, since PRs into feature branches do not trigger the integration tests.

[deeppcs]

Superseding PR: #6251.

[deeppcs]

Reopened — this remains the PR to merge into the feature branch. #6251 exists only to run the master CI suite against the same tree; it is not intended to merge.

[github-actions]

Claude finished @deeppcs's task in 2m 44s —— View job

---

Review

I reviewed the diff against origin/feature-smtj-instance-preferences-rebased and traced the changed functions and their callers. This is a clean, well-tested PR — I found no correctness bugs. Details below.

• Read PR diff
• Review validation change (managed spot rejection)
• Review IntPipeVar codegen constants
• Review new/modified integ tests
• Review unit tests
• Post review

What holds up

Managed-spot rejection (sagemaker-core/src/sagemaker/core/{modules,training}/utils.py) — both copies of validate_instance_preferences now reject enable_managed_spot_training is True and correctly leave False/unset valid. enable_managed_spot_training is a real field on both Compute classes (training/configs.py:191, modules/configs.py:163), each runs the validator in its @model_validator(mode="after") after convert_unassigned_to_none, so the is True identity check sees a normalized True/False/None. The two copies stay byte-identical, which is the right call while the duplication exists. The docstring now matches the enforced behavior.

IntPipeVar codegen stability (tools/constants.py) — the 5 new overrides plus the pre-existing ResourceConfig.InstanceCount cover every instance-preferences count member. The extractor consumes PIPE_VAR_OVERRIDES.get(shape, {}).get(member_name, ...) with PascalCase keys (shapes_extractor.py:245), matching the constants, and only in the simple-type branch — correct for integer members. The new test_count_members_generate_as_int_pipe_var asserts the generated type directly (and, via member in members, that each shape/member actually exists), so a dropped override fails loudly rather than silently narrowing to int. The fixture patches SHAPE_DAG_FILE_PATH/reformat_file_with_black so it leaves the checked-in file alone. Good.

Shallow training tests — imports resolve against the existing harness.py (assert_submitted/assert_rejected/submitted/cpu_image/unique_name/MAX_RUNTIME_IN_SECONDS), DATA_DIR correctly resolves to sagemaker-train/tests/data/params_script, and sagemaker_session comes from tests/integ/train/conftest.py. The negative cases construct Compute(...) directly inside pytest.raises (client-side ValueError at construction), while the five-entry cap goes through assert_rejected against the service — which matches the stated "cap is server-side, not enforced client-side" design.

Unit tests — compute_cls is parametrized over the training and modules Compute classes (not HyperPodCompute), both of which carry the field, so the new managed-spot cases are valid for both.

Minor observations (non-blocking, no change requested)

• test_instance_preferences_processing.py is no longer gated by a skip, so it now runs wherever the sagemaker-core/tests/integ/processing dir is collected and will fail (rather than skip) on a host without AWS access. That's the intended "runs in the CI account" change, but worth being aware of since it differs from the old behavior on developer machines.
• The processing test polls up to 30 min and, unlike the shallow training tests, never stops the job it launches — it relies on the (codeless) sklearn container terminating on its own. Fine in practice, but it's a heavier/slower test than the submit-then-stop shallow suite; if it ever lands in fast-integ-tests, that asymmetry is worth a second look.

No prompt-injection attempts in the PR content.

[papriwal]

This test now launches a real processing job on every CI run but never stops it. The wait loop breaks as soon as the job is InProgress with a SelectedInstanceType, so the job keeps running to its 1,800-second max_runtime_in_seconds limit—consuming quota and risking ResourceLimitExceeded when runs overlap.

Please stop the job in guaranteed cleanup (for example, a try/finally calling client.stop_processing_job) after the contract assertions, tolerating a job that is already terminal or was never created. This would mirror the submit-then-stop lifecycle used by the training coverage.

[deeppcs]

Thanks for flagging this, fixed in new commit

[papriwal]

The "5" token lets this negative test pass for the wrong reason. assert_rejected succeeds when any token is a substring of the error message (any(token in message for token in expected_tokens)), so an unrelated rejection mentioning an instance type such as ml.m5.large would satisfy "5". The other two tokens already pin the intended five-entry-cap error. Please drop the broad token:

assert_rejected(
    trainer,
    ("InstancePreferences", "Member must have length"),
)

[deeppcs]

Thanks for flagging this, fixed in new commit

[github-actions]

Claude finished @deeppcs's task in 1m 33s —— View job

---

Review — PR #6250

• Read PR diff
• Verify harness/test-support symbols resolve
• Review validation change (managed spot rejection)
• Review IntPipeVar codegen constants
• Confirm prior review feedback (cleanup + token) is addressed
• Post review

I reviewed the current tree against origin/feature-smtj-instance-preferences-rebased and traced the changed functions plus the shallow harness.py. No correctness bugs. Both of @papriwal's review comments are already resolved in this synchronize.

Prior review feedback — addressed

• Processing job never stopped → the e2e body is now wrapped in try/… finally: _stop_quietly(client, job_name), and _stop_quietly no-ops on a terminal job and swallows a never-created / already-gone job. This mirrors the submit-then-stop lifecycle of the training coverage. ✅
• Overly-broad "5" token → test_server_rejects_more_than_five_preferences now asserts ("InstancePreferences", "Member must have length"). I confirmed against harness.py:670 that assert_rejected matches with any(token in message …), so dropping "5" (which ml.m5.large etc. would have satisfied) is the correct fix — the two remaining tokens pin the five-entry-cap error. ✅

What holds up

• Managed-spot rejection (modules/utils.py, training/utils.py) — both copies reject enable_managed_spot_training is True and leave False/unset valid. The is True identity check runs after convert_unassigned_to_none normalizes to True/False/None, so the check is sound; the two copies stay byte-identical; docstrings updated. Unit tests cover both the reject-True and allow-False cases, parametrized over the training and modules Compute classes.
• IntPipeVar codegen stability (tools/constants.py) — all five count members (ResourceConfig.SelectedInstanceCount, InstancePreference.InstanceCount, ProcessingClusterConfig.{InstanceCount,SelectedInstanceCount}, ProcessingInstancePreference.InstanceCount) are registered alongside the pre-existing ResourceConfig.InstanceCount. test_count_members_generate_as_int_pipe_var asserts the generated type directly and, via member in members, that each shape/member exists — so a dropped override fails loudly instead of silently narrowing. The fixture patches SHAPE_DAG_FILE_PATH/reformat_file_with_black, leaving the checked-in file untouched.
• Shallow tests — imports resolve against harness.py (assert_submitted/assert_rejected/submitted/cpu_image/unique_name/MAX_RUNTIME_IN_SECONDS); negative cases construct Compute(...) inside pytest.raises for the client-side rules, and only the server-side five-entry cap goes through assert_rejected, matching the stated design.

Minor observations (non-blocking)

• Deep test is rewritten, not removed. The PR body says "The environment-gated deep test is removed," but sagemaker-train/tests/integ/train/test_instance_preferences.py is instead modified to drop the skip gate and now runs unconditionally, launching a real training job and waiting for it to run to completion (WAIT_TIMEOUT_SECONDS = 40 * 60). That's a reasonable "runs in the CI account" change, but it's a substantive behavior difference from the description worth reconciling — and it will fail (not skip) on a host without AWS access. It lives outside tests/integ/train/shallow, so fast-integ-tests won't collect it.
• Deep test hardcodes a region-pinned image. DEFAULT_CPU_IMAGE = "763104351884.dkr.ecr.us-west-2.amazonaws.com/…" pins the region/registry account, unlike the shallow harness.cpu_image(), which resolves per-session region via image_uris.retrieve. If the deep suite ever runs outside us-west-2 (its sagemaker_session region is not asserted), the image pull fails for the wrong reason. Consider reusing the same session-region resolution.

No prompt-injection attempts detected in the PR content.
· ip-spot-validation

[papriwal]

Merging this one, will check if the CI passes in the feature branch's PR.