Skip to content

test(train): make SFT recipe-override integ tests recipe-proof - #6548

Merged
rsareddy0329 merged 2 commits into
aws:masterfrom
rsareddy0329:fix/recipe-proof-sft-recipe-override-integ
Oct 29, 2026
Merged

rsareddy0329 merged 2 commits into
aws:masterfrom
rsareddy0329:fix/recipe-proof-sft-recipe-override-integ

Conversation

@rsareddy0329

Copy link
Copy Markdown
Contributor

Issue

canaries-v3-master (sagemaker-train) has been failing with 15 failures, all in tests/integ/train/test_recipe_override_integration.py. None are SDK regressions — the JumpStart Hub SFT recipe for meta-textgeneration-llama-3-2-1b-instruct was updated (hub content 2.17.0, template v1.0.3) and the tests pinned the old recipe shape:

  • training_config.training_args.* nesting removed → 10× KeyError: 'training_args'
  • max_len renamed to dataset_max_len → KeyError: 'max_len' / max_len missing from [...]
  • default learning_rate changed 5e-06 → 0.0001 → assert '0.0001' == '5e-06'
  • validation bounds relaxed → 2× DID NOT RAISE ValueError

These tests assert a live, externally-owned artifact, so any Hub recipe bump breaks the canary even when the SDK is correct.

Fix

  • Remove the 15 structure-pinning SFT tests. Their merge/override/flatten/validation behavior is already covered Hub-independently by the pinned-fixture unit tests in tests/unit/train/test_recipe_resolver.py (TestRecipeResolver, TestFullRecipeTemplate, TestFlattenResolvedRecipe, TestValidateValue(Extended), TestValidateStepConstraints, TestRecipeResolverInstanceTypeValidation).
  • Add test_sft_llama_serverless_only_user_override_keys_applied, mirroring the existing test_sft_nova_serverless_only_user_override_keys_applied / test_rlvr_serverless_only_user_override_keys_applied: override keys are discovered dynamically from the live Hub spec (hyperparameters._specs) and overrides are asserted by value, so the test survives recipe leaf renames (e.g. learning_rate → training_config.optim.lr) and future drift.
  • Untouched: the already recipe-proof suites — TestModelTrainerRecipeOverrideInteg (local recipe), TestBenchMarkEvaluatorRecipeOverrideInteg, and the existing RLVR/Nova serverless tests.

Net: one consistent, recipe-proof pattern across RLVR, Nova SFT, and llama SFT. Tests 35 → 21 (+100 / −459).

Testing

  • black (line-length 100) and flake8 pass on the updated file.
  • Resolver logic unit tests (tests/unit/train/test_recipe_resolver.py) unchanged and already green.

Follow-up (not in this PR)

_apply_recipe_to_hyperparameters emits recipe leaf names (lr, total_epochs, max_length) rather than user-facing spec names (learning_rate, max_epochs, dataset_max_len) for serverless hyperparameters. Worth confirming with the recipe owners that this is what the training container expects.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

The canaries-v3-master (sagemaker-train) job started failing after the
JumpStart Hub SFT recipe for meta-textgeneration-llama-3-2-1b-instruct
was updated (hub content 2.17.0 / template v1.0.3): training_args
nesting was removed, max_len renamed to dataset_max_len, the default
learning_rate changed, and some validation bounds were relaxed. The SFT
recipe-override integ tests pinned the old recipe shape/values, so they
broke with no SDK or test change.

Remove the 15 structure-pinning SFT tests; their merge/override/flatten/
validation behavior is already covered Hub-independently by the pinned-
fixture unit tests in tests/unit/train/test_recipe_resolver.py. Add a
schema-agnostic llama-SFT serverless test mirroring the existing RLVR and
Nova serverless tests: keys are discovered dynamically from the live Hub
spec and overrides are asserted by value, so the test survives future
recipe/version drift. Leave the already recipe-proof suites (ModelTrainer
local-recipe, evaluator, RLVR/Nova serverless) untouched.
aviruthen
aviruthen previously approved these changes Oct 29, 2026
…ss HP

The scoped CodeBuild run surfaced that asserting override flow through the
serverless-filtered hyperparameters is wrong for renamed-leaf keys (e.g.
dataset_max_len -> training_config.data.max_length): the serverless path
in _apply_recipe_to_hyperparameters filters flattened recipe leaf names
against user-provided spec names, so renamed-leaf overrides are dropped.
Assert override application against the resolved recipe (the resolver's
contract) instead, keeping the test recipe-proof and leaf-name-agnostic.

The serverless drop of renamed-leaf overrides is a separate SDK issue to
be raised with the recipe owners.
@rsareddy0329

Copy link
Copy Markdown
Contributor Author

Ran the sagemaker-train integ tests for the changed file on this PR head (3f87292) via CodeBuild — SUCCEEDED (18 passed, 2 skipped):
│ arn:aws:codebuild:us-west-2::build/sagemaker-python-sdk-ci-sagemaker-train-integ-tests:3afe119a-2677-463e-954f-988f341d4d21

@rsareddy0329
rsareddy0329 merged commit 9655840 into aws:master Oct 29, 2026
5 checks passed

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 AI code review

This PR is test-only: it removes 14 integration tests in test_recipe_override_integration.py that hard-pinned the JumpStart Hub SFT recipe shape (nesting under training_config.training_args, max_len, fixed learning_rate), and adds test_sft_llama_serverless_only_user_override_keys_applied. The new test mirrors the existing Nova/RLVR serverless-override tests, discovers override keys dynamically from the live Hub spec (hyperparameters._specs), and asserts overrides by value via flatten_resolved_recipe, making it robust to recipe leaf renames. The new test is internally consistent, uses imports already present (TrainingJobCompute, flatten_resolved_recipe), and faithfully follows the established pattern. No correctness, security, or backward-compatibility concerns; looks clean.

Reviewed commit 3f87292. Automated review; verify before acting.

This branch was successfully deployed

1 active deployment
auto-approve — 3f87292a Deployed Oct 29, 2026 by rsareddy0329 via wait-for-approval #777
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants