Repository navigation
fix: resolve HuggingFace version alias in image URI tag - #5722
mohamedzeidan2021 merged 4 commits into
Conversation
When constructing the image URI tag for HuggingFace framework containers, the code used `original_version` (the raw user input, e.g. "4.49") instead of resolving it through the version alias mapping (e.g. "4.49" → "4.49.0"). This caused image URIs to contain truncated transformer versions like `transformers4.49` instead of the correct `transformers4.49.0`, resulting in references to non-existent container images. Use `_version_for_config(version, config)` to properly resolve the version alias before embedding it in the image tag. Made-with: Cursor
- Fix existing test_retrieve_huggingface assertions to expect resolved versions (4.2.1) instead of raw aliases (4.2) in image tags - Add parametrized test covering all 25 version aliases across training and inference scopes to verify aliases resolve correctly in tags - Add parametrized test covering all 26 non-aliased (full) versions to confirm no regression when passing complete version strings directly Made-with: Cursor
GitHub Actions
Failure
GitHub Actions
Failure
|
Hi, are you still looking to get this merged? |
|
@mohamedzeidan2021 Yes, can you help me do the review? |
|
Claude finished @giangle-AI's task in 1m 15s —— View job Code Review: Resolve HuggingFace version alias in image URI tag
Verdict: ✅ Correct, well-targeted fix — looks good to mergeThe one-line change is the right fix and is internally consistent with the rest of Why the fix is correct The version used to select the config and the version used to build the tag must match. The config is selected at version_config = config["versions"][_version_for_config(version, config)]...which resolves top-level No regression in the override branches
Test changes correctly encode the fixed behavior
Minor (non-blocking) observations
No security, performance, or backward-compatibility concerns — this restores the intended tag for aliased inputs rather than changing a correct behavior. Nice catch on a real bug. 🚀 |
|
Test failures are from pre-existing issues, not introduced by this PR. only codestyle failure which I will address now. |
The new parametrized HuggingFace version-alias tests were not formatted with the repo's pinned black (26.3.1), failing the codestyle-doc-tests (sagemaker-core) check. Formatting only - no behavior change.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 AI code review
This PR fixes a real bug in image_uris.retrieve for HuggingFace framework containers: the image tag used the raw original_version (e.g. 4.49) instead of resolving it through the config's version_aliases (e.g. 4.49.0), producing tags that pointed to non-existent images. The one-line fix (_version = _version_for_config(version, config)) is correct and matches the established guidance to resolve version aliases through the config mapping. The accompanying data-driven unit tests exercise every alias and full version in the real huggingface.json, which is a solid regression guard. I traced the surrounding logic and found no issues; the change is correct and well-targeted.
Reviewed commit cba26d9. Automated review; verify before acting.
| assert ( | ||
| "564829616587.dkr.ecr.us-east-1.amazonaws.com/huggingface-pytorch-training:" | ||
| "1.6.0-transformers4.3.1-gpu-py37-cu110-ubuntu18.04" == pt_new_version | ||
| "1.6.0-transformers4.2.1-gpu-py37-cu110-ubuntu18.04" == pt_new_version |
There was a problem hiding this comment.
🟡 Low: This updated assertion is worth a sanity check. The call passes version="4.3.1", which is not present in this test's single-version config (only 4.2.1 exists). Because _validate_version_and_set_if_needed takes the single-version shortcut (len(available_versions) == 1 and 4.3.1 is not an alias), version is silently coerced to 4.2.1, so after this fix the tag correctly becomes transformers4.2.1. The expectation is right, but the test name/value pt_new_version with input 4.3.1 is now misleading since the input version is effectively ignored. Consider using a value that is actually a defined alias (or adding 4.3.1 to the config) so the test clearly demonstrates alias resolution rather than the single-version fallback.
test_passing_different_pipeline_variables_to_function chains a ScriptProcessor ProcessingStep with two @step function steps (3 jobs). The 600s wait budget was too low once instance provisioning is included, so func_2 was still "Executing" at the deadline and the test failed fleet-wide (same failure on main and on unrelated PRs aws#6154/aws#6261/aws#6400/ aws#5722), not just here. Raise it to 1500s, consistent with the other multi-step tests in this file (800-1000s). Formatting-only otherwise; no product code change.
…processors (#6364) * fix(processing): Fix V3 shape regressions in processors PySparkProcessor still built ProcessingInput/ProcessingOutput with the V2 source/destination keyword signature, so spark_event_logs_s3_uri and the configuration channel failed on V3. Build them with the V3 shapes (ProcessingS3Input/ProcessingS3Output) instead. Validate that submit_py_files, submit_jars and submit_files are lists and raise a clear ValueError otherwise. FrameworkProcessor.run(requirements=...) now threads the file name into the generated runproc.sh instead of hardcoding requirements.txt. Processor.run defaults kms_key to the configured output_kms_key so uploaded code is encrypted with the same key as job outputs. The requirements file is also honored on the custom entry_point (S3 source_dir) path, and a relative path such as reqs/cpu.txt is kept as-is because the source bundle preserves the source_dir layout. Integration tests (real jobs, submit-then-stop where the fix is in the request path) cover the code-upload KMS fallback, the FrameworkProcessor requirements file, and a V3 PySparkProcessor job ported from V2 that asserts the event-log output and the conf/py-files input shape; V3 had no Spark integ coverage before. Fixes #3809 Fixes #6253 Fixes #6252 Fixes #5805 Fixes #4874 --- X-AI-Prompt: Fix S-effort PySDK V3 bugs, processing-spark theme X-AI-Tool: Kiro * fix(processing): encrypt sourcedir.tar.gz with resolved kms_key FrameworkProcessor._pack_and_upload_code computed the output_kms_key fallback (evaluated_kms_key) only after calling _package_code, and passed the raw kms_key to _package_code. FrameworkProcessor.run does not apply the fallback before delegating here, so with output_kms_key set and no explicit kms_key, sourcedir.tar.gz -- the largest, most sensitive artifact -- was uploaded unencrypted, while install_requirements.py and runproc.sh were encrypted. This left #4874 unfixed for FrameworkProcessor. Resolve the key once, before packaging, and pass it into _package_code so all three artifacts share the same key. An explicitly passed kms_key still wins; behaviour when both are unset is unchanged. Add unit tests asserting _package_code and the helper-script uploads all receive the resolved key on the fallback path (regression for #4874) and that an explicit key overrides output_kms_key. Relates-to #4874 * fix(processing): reject pipeline variable for requirements argument FrameworkProcessor.run bakes the requirements path into the generated runproc.sh script, so it cannot be a pipeline variable. Passing a ParameterString previously surfaced as an opaque "TypeError: expected str, bytes or os.PathLike object, not ParameterString" from os.path.isabs deep in script generation. Add an explicit is_pipeline_variable guard at the top of FrameworkProcessor.run, raising a clear ValueError, matching the existing code and submit_app guards. Add a unit test. Relates-to #5805 * test(workflow): raise wait_duration for 3-job step-decorator test test_passing_different_pipeline_variables_to_function chains a ScriptProcessor ProcessingStep with two @step function steps (3 jobs). The 600s wait budget was too low once instance provisioning is included, so func_2 was still "Executing" at the deadline and the test failed fleet-wide (same failure on main and on unrelated PRs #6154/#6261/#6400/ #5722), not just here. Raise it to 1500s, consistent with the other multi-step tests in this file (800-1000s). Formatting-only otherwise; no product code change. * chore(codestyle): black-format test_processing.py Apply black to tests/unit/test_processing.py (added two blank lines before TestProcessorLocalModeRole), fixing the failing codestyle-doc-tests (sagemaker-core) black-check and flake8 E302. Formatting-only. --------- Co-authored-by: Amarjeet LNU <jamjee@amazon.com> Co-authored-by: Roja Reddy Sareddy <rsareddy@amazon.com>
Summary
original_version(raw user input like"4.49") instead of resolving it through the version alias mapping ("4.49"→"4.49.0"). This produced incorrect image tags such astransformers4.49instead of the correcttransformers4.49.0, pointing to non-existent container images._version = original_versionwith_version = _version_for_config(version, config)on line 247 ofimage_uris.pyso the version alias is properly resolved before being embedded in the image tag.Before (broken)
After (fixed)
Test plan
transformers4.49in the tag)transformers4.49.0in the tag)image_uris.retrievewith HuggingFace framework4.37,4.51) also resolve correctlyBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.