fix: set full s3_input fields on QualityCheckStep baseline_dataset pipeline-variable input

[mohamedzeidan2021]

Issue

Fixes #6206

QualityCheckStep._generate_baseline_job_inputs() builds a ProcessingInput from an s3_input dict when baseline_dataset is a pipeline variable (Join, ParameterString, ...). That dict omitted s3_data_type, which ProcessingS3Input requires, so constructing the step raised:

ValidationError: 1 validation error for ProcessingInput
s3_input.s3_data_type
  Field required

Fix

Set the same fields on the pipeline-variable branch that the non-pipeline-variable branch already ships via _upload_and_convert_to_processing_input (model_monitoring.py): s3_data_type="S3Prefix", s3_input_mode="File", s3_data_distribution_type="FullyReplicated".

s3_data_type alone makes the model constructible, but is not sufficient: leaving s3_input_mode unset keeps it as the Unassigned() sentinel, and the arguments serializer emits that sentinel (its getattr(..., "File") fallback never applies because the attribute is set to the sentinel), so pipeline.definition() would then fail with TypeError: Object of type Unassigned is not JSON serializable. Setting all three fully unblocks the pipeline-variable path and keeps it consistent with both the else-branch and the sibling clarify_check_step.

Testing

sagemaker-mlops/tests/unit/workflow/test_quality_check_step.py::test_generate_baseline_job_inputs_with_pipeline_variable_baseline_dataset: builds the step with a Join baseline dataset, asserts the resulting s3_input has s3_data_type="S3Prefix" and that s3_input_mode/s3_data_distribution_type are concrete (not Unassigned), then json.dumps of the serialized fields to guard the definition-serialization path. Fails on master, passes with the fix.

Verified end to end: with the fix, the interpolated + json.dumps'd S3Input produces a valid pipeline definition (S3Uri → the Join expression {"Std:Join": {...}}, S3DataType": "S3Prefix", S3InputMode": "File"); on master it raises the reported ValidationError. test_quality_check_step.py + test_quality_check_step_kms.py = 11 passed; black (100) / flake8 (120) clean.

Backwards compatibility

The non-pipeline-variable branch is untouched. File / FullyReplicated match the values the else-branch already sends, so the two branches now produce consistent inputs.

[github-actions]

Claude finished @mohamedzeidan2021's task in 1m 6s —— View job

---

Code Review

I reviewed this PR against origin/master. The change is small, correct, and well-targeted. Summary: looks good to merge, with one minor test-quality note.

Task list

• Read the PR diff (/tmp/pr.diff)
• Verify the fix against the mirrored _upload_and_convert_to_processing_input code path
• Check consistency with the sibling clarify_check_step
• Assess the added test
• Post review

---

Correctness ✅

The fix is exactly right. quality_check_step.py:371-373 now sets s3_data_type="S3Prefix", s3_input_mode="File", s3_data_distribution_type="FullyReplicated" on the pipeline-variable branch. These match, value-for-value, what the non-pipeline-variable branch already ships via _upload_and_convert_to_processing_input (model_monitoring.py:1681-1687):

s3_input=ProcessingS3Input(
    s3_uri=source,
    local_path=destination,
    s3_data_type="S3Prefix",
    s3_input_mode="File",
    s3_data_distribution_type="FullyReplicated",
),

So the two branches now produce consistent inputs — the backward-compatibility claim holds. It's also consistent with the sibling clarify_check_step.py (which sets s3_data_type/s3_input_mode on its dict-based s3_inputs).

The PR body's reasoning about the Unassigned() sentinel is accurate: the serializer at clarify_check_step.py:280-281 uses getattr(inp.s3_input, "s3_input_mode", "File"), and that fallback only fires when the attribute is absent — an attribute set to the Unassigned() sentinel still returns the sentinel, which then fails json.dumps. Setting all three fields is the correct, complete fix rather than just s3_data_type.

Tests

The added regression test fails on master and passes with the fix, and the core assertions are the right ones:

• s3_input.s3_data_type == "S3Prefix" (would raise ValidationError on master)
• s3_input_mode / s3_data_distribution_type are not Unassigned

Minor nit (non-blocking): the serialized/json.dumps block at the end of the test doesn't add much value — it hardcodes "S3Uri": "s3://resolved/at/runtime" rather than exercising the real serialization path, so the json.dumps call is really only checking hand-built literal strings. The two assert not isinstance(..., Unassigned) checks above it already guard the sentinel-serialization concern directly. Consider dropping the stubbed dict, or (stronger) building the actual step and calling pipeline.definition() to exercise the real path end-to-end. Not required for merge.

Style-only observations (imports inside the test function, import json mid-body) are cosmetic and I'll leave those to the linters.

Security / Performance

No concerns — no credential handling, no new input surface, no perf implications. The values are static constants matching an existing code path.

---

Nice, well-documented fix. The inline code comment explaining why all three fields are needed (not just s3_data_type) is a good touch for future maintainers.
· fix/issue-6206-qualitycheck-baseline-pipeline-var