Repository navigation
feature: Add HubContentStep and Session.import_hub_content - #6565
rohangujarathi wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
🤖 AI code review
This PR adds Session.import_hub_content(...) in sagemaker-core and a HubContentStep pipeline step in sagemaker-mlops, plus a StepTypeEnum.HUB_CONTENT value and workflow export. The implementation closely follows existing patterns: import_hub_content routes through _intercept_create_request like other producers, dict documents are serialized via the shared convert_job_config_document_to_string (so pipeline variables become a Join), step_args is validated with validate_step_args_input, and properties is shaped from the botocore DescribeHubContentResponse shape. I verified the imports (json, format_tags, _append_project_tags, RequestType, _JobStepArguments, Properties), the base-class signatures, and that arguments reads a shallow copy of step_args.args without mutating the captured request. Unit and integ tests are comprehensive. No correctness, security, or backward-compatibility issues found; the implementation is clean.
Note: the PR body asks maintainers to hold the merge until the HubContent step type is available service-side (otherwise CreatePipeline/integ tests fail). That is operational context for reviewers, not a code issue.
Reviewed commit 909574f. Automated review; verify before acting.
909574f to
e5d26ec
Compare
There was a problem hiding this comment.
🤖 AI code review
This PR adds Session.import_hub_content() (sagemaker-core) and HubContentStep (sagemaker-mlops), a ConfigurableRetryStep that imports content into a SageMaker hub within a pipeline. The producer routes through _intercept_create_request so it returns step args under a PipelineSession, and a dict HubContentDocument is encoded the same way JobStep encodes JobConfigDocument so pipeline variables resolve at execution time. The implementation closely follows the established JobStep/TrainingStep patterns, the StepTypeEnum addition and workflow export are correct, and the unit tests cover request shape, dict/string documents, pipeline-variable encoding, producer validation, properties, and the plain-Session path. I found no correctness or security issues; the code is consistent with existing conventions. Note: the PR body asks reviewers to hold the merge until the HubContent step type is rolled out in Pipelines (otherwise the integ tests fail) — that is a release-coordination item, not a code concern.
Reviewed commit e5d26ec. Automated review; verify before acting.
e5d26ec to
e9882e0
Compare
There was a problem hiding this comment.
🤖 AI code review
This PR adds Session.import_hub_content() in sagemaker-core (routing through _intercept_create_request so it returns step args under a PipelineSession, serializing a dict HubContentDocument at request time) and a HubContentStep in sagemaker-mlops, plus StepTypeEnum.HUB_CONTENT and the workflow export. The implementation closely mirrors the established JobStep/JobConfigDocument pattern: the arguments property shallow-copies the captured request and encodes a dict document via convert_job_config_document_to_string for pipeline-variable resolution, and to_request folds in cache config. Imports (json, format_tags, _append_project_tags, validate_step_args_input) are all present at the base commit, tag/empty-value handling matches existing methods, and unit + integ tests cover capture, serialization, optional fields, producer validation, properties, and pipeline definition. I found no correctness, security, or backward-compatibility issues. Note: the PR body asks reviewers to hold the merge until the HubContent step type is rolled out server-side — a process note, not something that changes the code review.
Reviewed commit e9882e0. Automated review; verify before acting.
Issue #, if available:
Description of changes:
Adds a pipeline step that imports content into a SageMaker hub.
Session.import_hub_content(...)(sagemaker-core): builds anImportHubContentrequest and routes it through_intercept_create_request, so under aPipelineSessionit returns step arguments instead of calling the service.hub_content_documentcan be a string or a dict; a dict is serialized when the request is sent.HubContentStep(sagemaker-mlops): aConfigurableRetryStepthat acceptsstep_argsfromimport_hub_contentonly, supportscache_configandretry_policies, and exposespropertiesshaped likeDescribeHubContentResponse. A dict document is encoded inargumentsthe same wayJobStepencodesJobConfigDocument, so pipeline variables inside it are resolved at execution time.StepTypeEnum.HUB_CONTENT = "HubContent"and thesagemaker.mlops.workflowexport.Example:
When
hub_content_versionis omitted, the service assigns the next version on each execution.The HubContent step type is being rolled out in SageMaker Pipelines; please hold the merge until it is available, otherwise
CreatePipelinerejects the definition and the new integ tests fail.Testing done:
sagemaker-mlops/tests/unit/workflow/test_hub_content_step.py: capture without a service call, request shape, unset optional fields, dict and string documents, pipeline-variable document encoding, step request with cache and retry config, producer validation, properties, pipeline definition, plainSessioncall.TestImportHubContent(sagemaker-core/tests/unit/helper/test_session_helper.py): required fields, dict serialization, all optional fields and tags, empty optional values, service error propagation.test_job_step.pyenum member set updated; existingtest_inference_lineage_steps.py,test_job_step.pyandtest_session_helper.pypass.sagemaker-mlops/tests/integ/workflow/test_hub_content_step.py: an import with a parameterized document feeding aConditionSteponHubContentStatusandHubContentType, unpinned reruns importing a new version, and a missing hub failing the step. All 3 pass.Merge Checklist
General
Tests
unique_name_from_baseto create resource names in integ tests (if appropriate)By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.