Repository navigation
test(train): make SFT recipe-override integ tests recipe-proof - #6548
rsareddy0329 merged 2 commits into
Conversation
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.
…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.
|
Ran the sagemaker-train integ tests for the changed file on this PR head (3f87292) via CodeBuild — SUCCEEDED (18 passed, 2 skipped): |
There was a problem hiding this comment.
🤖 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.
Issue
canaries-v3-master (sagemaker-train)has been failing with 15 failures, all intests/integ/train/test_recipe_override_integration.py. None are SDK regressions — the JumpStart Hub SFT recipe formeta-textgeneration-llama-3-2-1b-instructwas 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_lenrenamed todataset_max_len→KeyError: 'max_len'/max_len missing from [...]learning_ratechanged5e-06→0.0001→assert '0.0001' == '5e-06'DID NOT RAISE ValueErrorThese tests assert a live, externally-owned artifact, so any Hub recipe bump breaks the canary even when the SDK is correct.
Fix
tests/unit/train/test_recipe_resolver.py(TestRecipeResolver,TestFullRecipeTemplate,TestFlattenResolvedRecipe,TestValidateValue(Extended),TestValidateStepConstraints,TestRecipeResolverInstanceTypeValidation).test_sft_llama_serverless_only_user_override_keys_applied, mirroring the existingtest_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.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) andflake8pass on the updated file.tests/unit/train/test_recipe_resolver.py) unchanged and already green.Follow-up (not in this PR)
_apply_recipe_to_hyperparametersemits 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.