Repository navigation
fix(processing): Fix V3 shape regressions + KMS/requirements gaps in processors - #6364
rsareddy0329 wants to merge 8 commits into
Conversation
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 aws#3809 Fixes aws#6253 Fixes aws#6252 Fixes aws#5805 Fixes aws#4874 --- X-AI-Prompt: Fix S-effort PySDK V3 bugs, processing-spark theme X-AI-Tool: Kiro
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 aws#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 aws#4874) and that an explicit key overrides output_kms_key. Relates-to aws#4874
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 aws#5805
|
Claude finished @rsareddy0329's task in 2m 37s —— View job Review: PR #6364 — Fix V3 shape regressions + KMS/requirements gaps in processors
Overall this is a solid, well-scoped set of fixes. The KMS-key resolution is now done once before packaging, so A few things worth addressing: 1. Correctness gap —
|
![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 several V3 processor regressions: it migrates PySparkProcessor event-log output and conf/dep input channels to the V3 ProcessingS3Input/ProcessingS3Output shapes, threads a parameterized requirements file name into the generated runproc.sh, adds a ValueError guard for non-list submit_deps and pipeline-variable requirements, and resolves the KMS key before _package_code so sourcedir.tar.gz is encrypted with the same key. The source changes look correct and are well covered by new unit and integration tests. Two issues worth addressing: the moved KMS fallback uses truthiness rather than an is None check, and the requirements path is interpolated unquoted into the generated shell script.
Reviewed commit 8e82eb5. Automated review; verify before acting.
| # is not set we fall back to the configured output_kms_key. This must happen | ||
| # before _package_code so sourcedir.tar.gz (the largest, most sensitive artifact) | ||
| # is not left unencrypted (issue #4874). | ||
| evaluated_kms_key = kms_key if kms_key else self.output_kms_key |
There was a problem hiding this comment.
🟠 Medium: evaluated_kms_key = kms_key if kms_key else self.output_kms_key uses a truthiness check, which is inconsistent with the other two fallbacks in this PR (if kms_key is None in Processor.run and kms_key if kms_key is not None else self.output_kms_key in ScriptProcessor.run). An explicitly passed empty string "" is a falsy-but-set value: here it would silently fall back to output_kms_key, whereas the other paths preserve it. Prior review guidance on this change set was explicit: "Use is None checks for kms_key fallback logic, not truthiness, to avoid silent fallback on empty strings." Recommend evaluated_kms_key = kms_key if kms_key is not None else self.output_kms_key. Source: #6318
| fi | ||
|
|
||
| {entry_point_command} {entry_point} "$@" | ||
| """).format( |
There was a problem hiding this comment.
🟠 Medium: The requirements path is interpolated unquoted as the argument to install_requirements.py: python3 {install_requirements_dir}/install_requirements.py {requirements_file}. A relative path containing a space (e.g. my reqs/cpu.txt) would be split into two arguments, and other shell metacharacters would be interpreted. The -f '{requirements_file}' test is single-quoted, so the two uses are inconsistent. Prior guidance on this work: "Quote all shell interpolations of user-provided paths in generated scripts to prevent injection and handle spaces." Quote the argument (e.g. '{requirements_file}'). Source: #6318
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.
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.
There was a problem hiding this comment.
🤖 AI code review
This PR fixes several V3 processor regressions: it migrates PySparkProcessor event-log output and conf/py-files channels to the V3 ProcessingS3Input/ProcessingS3Output shapes, threads a requirements filename into the generated runproc.sh, adds ValueError guards for string submit_deps and pipeline-variable requirements, and unifies the kms_key → output_kms_key fallback (now resolved before _package_code so sourcedir.tar.gz is encrypted too). The core changes look correct and are well covered by unit tests. My main concern is an inconsistency in the KMS fallback logic that contradicts the team's own stated convention; a couple of lower-severity items (shell quoting, an acknowledged gap on the local-source custom-entrypoint path) are noted below.
Findings not attached to a changed line:
sagemaker-core/src/sagemaker/core/processing.py:1697🟡 Low:requirementsis now threaded through_generate_custom_framework_script, but only theself._is_s3_uri(source_dir)branch uses it. The local-source_dircustom-entry_pointbranch (which embeds the entry point content and never invokesinstall_requirements.py) ignoresrequirementsentirely, soFrameworkProcessor.run(entry_point=..., source_dir=<local dir>, requirements=...)silently skips dependency installation. The PR body acknowledges this is a follow-up; worth tracking so the feature isn't partially broken on that path.
Reviewed commit 29064c5. Automated review; verify before acting.
| # encrypted with the same key. An explicitly passed kms_key always wins; when it | ||
| # is not set we fall back to the configured output_kms_key. This must happen | ||
| # before _package_code so sourcedir.tar.gz (the largest, most sensitive artifact) | ||
| # is not left unencrypted (issue #4874). |
There was a problem hiding this comment.
🟠 Medium: The KMS fallback here uses truthiness (kms_key if kms_key else self.output_kms_key), but the sibling fallbacks added in this same PR (Processor.run: if kms_key is None: and ScriptProcessor.run: kms_key if kms_key is not None else self.output_kms_key) use is None. This is an inconsistency: an explicitly-passed empty string kms_key="" is preserved by the other two paths but silently replaced by output_kms_key here. Recommend using an is None check for consistency.
This also matches the retrieved guidance: "Use is None checks for kms_key fallback logic to preserve explicitly-passed empty strings or falsy values differently from unset." Source: #6364
|
|
||
| if [[ -f 'requirements.txt' ]]; then | ||
| if [[ -f '{requirements_file}' ]]; then | ||
| # Some py3 containers has typing, which may breaks pip install |
There was a problem hiding this comment.
🟡 Low: python3 {install_requirements_dir}/install_requirements.py {requirements_file} interpolates the user-provided requirements path unquoted (the [[ -f '{requirements_file}' ]] test is single-quoted, but this invocation is not). A requirements path containing spaces or shell metacharacters would be mis-parsed or injected. Quoting the argument (e.g. install_requirements.py '{requirements_file}') would make it robust, consistent with the retrieved guidance: "Quote all shell interpolations of user-provided paths in generated scripts to prevent injection and handle spaces." Source: #6318 (Same applies to the S3-source branch around line 1731.)
There was a problem hiding this comment.
🤖 AI code review
This PR fixes several V3 processor regressions: migrates PySparkProcessor event-log output and config/dep channels to the V3 ProcessingS3Input/ProcessingS3Output shapes, threads a custom requirements path into the generated runproc.sh, adds a submit_deps type guard, rejects pipeline-variable requirements, and resolves the KMS key before _package_code so sourcedir.tar.gz is encrypted. The functional changes look correct and are well covered by new unit and integ tests. Two items are worth addressing: the KMS fallback in _pack_and_upload_code uses truthiness rather than the is None check the team precedent calls for, and the requirements path is interpolated unquoted into the generated shell command.
Reviewed commit caf7f8f. Automated review; verify before acting.
| # produces -- the source bundle, install_requirements.py and runproc.sh -- is | ||
| # encrypted with the same key. An explicitly passed kms_key always wins; when it | ||
| # is not set we fall back to the configured output_kms_key. This must happen | ||
| # before _package_code so sourcedir.tar.gz (the largest, most sensitive artifact) |
There was a problem hiding this comment.
🟡 Low: This fallback uses truthiness (kms_key if kms_key else ...), which differs from the is None check used in Processor.run/ScriptProcessor.run in this same PR. The team precedent for this work is explicit: "Use is None checks for kms_key fallback logic to preserve explicitly-passed empty strings or falsy values differently from unset." With truthiness, an explicitly passed empty-string kms_key silently falls back to output_kms_key. Consider kms_key if kms_key is not None else self.output_kms_key for consistency. Source: #6364
| fi | ||
|
|
||
| {entry_point_command} {entry_point} "$@" | ||
| """).format( |
There was a problem hiding this comment.
🟡 Low: {requirements_file} is interpolated unquoted into the install_requirements.py {requirements_file} command (both here and in the S3 custom-entrypoint branch). The precedent for this change asks to "Quote all shell interpolations of user-provided paths in generated scripts to prevent injection and handle spaces." A relative subdirectory path containing a space (e.g. my reqs/cpu.txt) would word-split. The [[ -f '{requirements_file}' ]] test is quoted; the python invocation should be too (e.g. install_requirements.py '{requirements_file}'). Source: #6318
This branch was successfully deployed
2 participants
Summary
Takes over and supersedes #6318 (by @jam-jee), which fixes several V3 processor
regressions but had open review feedback. This PR keeps @jam-jee's original
commits intact and adds fixes for the review findings so the work can land.
Original fixes (from #6318, @jam-jee)
PySparkProcessorV3 shapes —spark_event_logs_s3_uriand the Sparkconfiguration channel now build
ProcessingS3Input/ProcessingS3Output(V3) instead of the removed V2
source=/destination=signature, so eventlogs and
_stage_submit_depswork again.submit_py_files/submit_jars/submit_files— a plain string nowraises a clear
ValueErrorinstead of iterating per-character.FrameworkProcessor.run(requirements=...)— the requirements path isthreaded into the generated
runproc.sh(default and S3-source customentry_pointpaths) instead of a hardcodedrequirements.txt.Processor.run/ScriptProcessor.runkms_key— falls back to theconfigured
output_kms_key, with an explicit key always winning.Additional fixes addressing review feedback
kms_keyfallback now coverssourcedir.tar.gz._pack_and_upload_coderesolved
evaluated_kms_keyafter calling_package_code, so the sourcebundle (the largest, most sensitive artifact) was uploaded unencrypted while
the helper scripts were encrypted — leaving
kms_keyinsagemaker.processing.Processorshould default tooutput_kms_key#4874 unfixed forFrameworkProcessor. The key is now resolved once, before packaging, andpassed into
_package_code.requirementsrejects pipeline variables with a clearValueError(matching the existing
code/submit_appguards) instead of an opaqueTypeErrorfromos.path.isabs, since the value is baked intorunproc.sh.Issues fixed
Fixes #3809
Fixes #6253
Fixes #6252
Fixes #5805
Fixes #4874
Testing
All unit tests in
sagemaker-core/tests/unit/test_processing.pyandtests/unit/spark/test_processing.pypass. New regression tests for the twoadditional fixes (
test_pack_and_upload_code_falls_back_to_output_kms_key,test_pack_and_upload_code_explicit_kms_key_wins,test_run_rejects_pipeline_variable_requirements) each fail without theirsource change and pass with it.
flake8clean on changed files.Still being addressed (tracked from review)
The following review items are not yet in this PR and will be addressed in
follow-up commits here:
isinstance(submit_deps, (list, tuple))inputnarrowing (accept non-
striterables),requirementson the local-source_direntry_pointpath, makingtest_framework_processor_requirements.pyuse a non-default filename, test-claim corrections in this description, and a
release note for the
output_kms_keydefault behavior change.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.