Skip to content

feat: Add application/x-parquet support to clarify.ModelConfig (Issue #3450) - #6068

Open
wasim-builds wants to merge 7 commits into
aws:masterfrom
wasim-builds:feature/issue-3450-parquet-support
Open

wasim-builds wants to merge 7 commits into
aws:masterfrom
wasim-builds:feature/issue-3450-parquet-support

Conversation

@wasim-builds

@wasim-builds wasim-builds commented Jul 21, 2026 •

Copy link
Copy Markdown
Contributor

Closes #3450

Summary

Add application/x-parquet support to ModelConfig in the Clarify module.

Changes

  • sagemaker-core/src/sagemaker/core/clarify/__init__.py: accept application/x-parquet as a valid content_type and accept_type.

Verification

clarify.ModelConfig now accepts parquet content types without validation errors.

@wasim-builds

Copy link
Copy Markdown
Contributor Author

Hi maintainers,

Submitting this update regarding feat: Add application/x-parquet support to clarify.ModelConfig (Issue #3450) for aws/sagemaker-python-sdk. Working on Amazon SageMaker Python SDK machine learning and model deployment pipelines has been a great experience, and I am very keen to continue contributing to aws/sagemaker-python-sdk.

I am also open to collaborating on a contract/freelance basis, discussing full-time opportunities, or joining the organization/team.

You can view my background and other open-source contributions at https://github.com/wasim-builds.

Copilot AI balanced review requested due to automatic review settings October 9, 2026 02:01

Copilot AI 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.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@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

The PR adds application/x-parquet as a valid content_type and accept_type for clarify.ModelConfig, both in the schema validation and the ModelConfig.__init__ runtime checks, plus docstring updates. The change is small, correct, and consistent with the existing DatasetType.PARQUET enum and DataConfig parquet support (Source: #3450). Two minor points: no unit test was added for the new accepted value, and the content_type error message was made less informative.

Reviewed commit 168202c. Automated review; verify before acting.

"image/jpg",
"image/png",
"application/x-npy",
"application/x-parquet",

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 Medium: The validation now accepts application/x-parquet for both accept_type and content_type, but no unit test exercises this new positive path. The existing tests only cover the invalid cases (test_init_with_invalid_accept_type_raises_error, test_init_with_invalid_content_type_raises_error in tests/unit/test_clarify.py). Please add a test constructing ModelConfig(..., content_type="application/x-parquet", accept_type="application/x-parquet") and asserting it is stored in predictor_config, so the newly supported behaviour is guarded against regressions.

raise ValueError(
f"Invalid content_type {content_type}."
f" Please choose text/csv or application/jsonlines."
f" Please choose a valid content_type."

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Low: While the accept_type error message was improved to list all valid values, this content_type message was changed to the vague "Please choose a valid content_type." The previous (also incomplete) text at least named options. Consider listing the valid content types here as you did for accept_type (e.g. text/csv, application/jsonlines, application/json, image/jpeg, image/png, application/x-npy, application/x-parquet) so users get actionable feedback.

@wasim-builds
wasim-builds deployed to manual-approval October 9, 2026 15:50 — with GitHub Actions Active
@wasim-builds
wasim-builds deployed to manual-approval October 9, 2026 15:50 — with GitHub Actions Active
@wasim-builds
wasim-builds deployed to manual-approval October 9, 2026 15:50 — with GitHub Actions Active

@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

The PR adds application/x-parquet as an accepted content_type and accept_type in clarify.ModelConfig, updating both the schema validation and the runtime checks in __init__, plus the docstrings. The change is correct and consistent with the existing DatasetType.PARQUET support. Two issues: the content_type error message was regressed to a generic string, and no unit test was added for the newly accepted value.

Findings not attached to a changed line:

  • sagemaker-core/src/sagemaker/core/clarify/__init__.py:1186 🟡 Low: This changes the content_type validation error to a generic "Please choose a valid content_type.", which no longer lists the valid options. The retrieved guidance for this change states: "Error messages for validation failures should list all valid options, not generic text." (Source: #6068). The previous message was also wrong (it only mentioned csv/jsonlines), so prefer enumerating all accepted values here, as you did for accept_type on line 1166-1167.

Reviewed commit 55f3e64. Automated review; verify before acting.

f"Invalid accept_type {accept_type}."
f" Please choose text/csv or application/jsonlines."
f" Please choose text/csv, application/jsonlines, application/json, or application/x-parquet."
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟠 Medium: No unit test was added for the newly accepted application/x-parquet value. The existing tests in tests/unit/test_clarify.py only assert that invalid types raise, so this new acceptance path is untested. The guidance for this change notes: "Add unit tests when introducing new accepted values to ModelConfig content_type and accept_type validation to prevent regressions." (Source: #6068). Please add positive tests that construct a ModelConfig with content_type="application/x-parquet" and accept_type="application/x-parquet".

This branch is waiting to be deployed

1 waiting deployment
manual-approval — d057cfa6 Waiting Oct 16, 2026 by wasim-builds via wait-for-approval #683
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.

Allow sagemaker.clarify.ModelConfig to use application/x-parquet for content_type & accept_type

4 participants