feat: Curated public hub #4011
feat: Curated public hub #4011
Conversation
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
| # distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF | ||
| # ANY KIND, either express or implied. See the License for the specific | ||
| # language governing permissions and limitations under the License. | ||
| """This module accessors for the SageMaker JumpStart Curated Hub.""" |
There was a problem hiding this comment.
just confirming: "Curated Hub" is the public term for this?
There was a problem hiding this comment.
+1 we should confirm naming conventions with Product
| """Retrieves s3 reference for model inference artifact""" | ||
| return create_s3_object_reference_from_bucket_and_key( | ||
| self.get_bucket(), f"{model_specs.model_id}/{model_specs.version}/infer.tar.gz" | ||
| ) |
There was a problem hiding this comment.
is this always the pattern of the inference s3 artifact? it's dangerous when things like this are hardcoded into the SDK, do we have to be careful.
There was a problem hiding this comment.
This is the format of where we will store the artifact in the HubBucket S3 - as long as it stays standardized here it will always be consistent
There was a problem hiding this comment.
Won't the Palatine Describe call have the most authoritative source of this info when "accessing"?
For Palatine model creation, there is a long term implication of how we store now vs what the Studio FE does. If we deviate from current conventions, then it becomes more difficult to do the p0 async storage work. Could we make the logic consistent with Studio?
There was a problem hiding this comment.
+1, don't make these kind of assumptions Chris. Here you should retrieve the specs file, and derive the correct S3 keys from it. Same comment throughout the module.
| ) -> S3ObjectReference: | ||
| """Retrieves s3 reference for model traiing script""" | ||
| return create_s3_object_reference_from_bucket_and_key( | ||
| self.get_bucket(), f"{model_specs.model_id}/{model_specs.version}/sourcedir.tar.gz" |
There was a problem hiding this comment.
same question here regarding uri path pattern
| f"{model_specs.model_id}/{model_specs.version}/training/sourcedir.tar.gz", | ||
| ) | ||
|
|
||
| def get_demo_notebook_s3_reference(self, model_specs: JumpStartModelSpecs) -> S3ObjectReference: |
There was a problem hiding this comment.
is there any way we can store these paths in the spec itself?
There was a problem hiding this comment.
Do you mean as an instance of the type? Yeah we can, but it'll be missing fields like demo-notebook
| def _get_training_dataset_prefix( | ||
| self, model_specs: JumpStartModelSpecs | ||
| ) -> str: # Studio expects the same format as public hub bucket | ||
| studio_model_metadata = self._studio_metadata_map[model_specs.model_id] # TODO: verify this |
There was a problem hiding this comment.
this is dangerous since there's a coupling now with studio metadata. we should put everything we need in the same metadata
| from sagemaker.jumpstart.curated_hub.accessors.jumpstart_s3_accessor import JumpstartS3Accessor | ||
|
|
||
|
|
||
| class PublicHubS3Accessor(JumpstartS3Accessor): |
There was a problem hiding this comment.
what's the difference b/w public hub and jumpstart s3 accessor?
There was a problem hiding this comment.
also, if we're making a new folder for accessors, we should relocate the current accessors.py to there
There was a problem hiding this comment.
The JumpstartS3Accessor is the interface for JumpStart s3 content storage locations, whereas the PublicHubS3Accessor is specific for the public JS bucket. This is different from the Palatine accessor, curated_hub_s3_accessor
+1 to your above comment, will address when merging into production repo
There was a problem hiding this comment.
The problem though is that CuratedHubS3Accessor implements JumpstartS3Accessor even though it is not related to JumpStart necessarily but rather Palatine. That can cause some confusion.
Could we perhaps go without the interface or rename to ModelS3Accessor ?
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
src/sagemaker/jumpstart/curated_hub/accessors/public_hub_s3_accessor.py
Outdated
Show resolved
Hide resolved
| self, resource_name: str, src: S3ObjectReference, dst: S3ObjectReference | ||
| ): | ||
| if not self._is_s3_object_different(src, dst): | ||
| print( |
There was a problem hiding this comment.
nit: can we use the logging module?
There was a problem hiding this comment.
Also, will this cause a lot to be printed to stdout, if most object as the same? or this is an unusual case
| ) | ||
| return | ||
|
|
||
| print(f"Copying {resource_name} from {src.bucket}/{src.key} to {dst.bucket}/{dst.key}...") |
There was a problem hiding this comment.
don't we need to do s3://{src.bucket}/{src.key} and s3://{dst.bucket}/{dst.key}
There was a problem hiding this comment.
True yeah, but it's just a print statement so I didn't bother adding the s3:// prefix haha. If it's confusing I'll throw it in
| "ERROR: encountered an exception when calling copy from" | ||
| + f" {src.bucket}/{src.key} to {dst.bucket}/{dst.key}: {str(ex)}" | ||
| ) | ||
| raise ex |
There was a problem hiding this comment.
should we log the stack trace? otherwise, you can get some cryptic error like KeyError and not know which line causes it
There was a problem hiding this comment.
+1, I'll add a stack trace expansion in the parallel copy secion
| f"Copying {resource_name} from {src.bucket}/{src.key} to {dst.bucket}/{dst.key} complete!" | ||
| ) | ||
|
|
||
| def _is_s3_object_different(self, src: S3ObjectReference, dst: S3ObjectReference) -> bool: |
There was a problem hiding this comment.
doesn't this not work if there is a difference in the multi-part upload chunk size?
There was a problem hiding this comment.
This will only be invoked on the initial equality check to avoid re-uploading - so in this case it'll be an entire object compared to an entire object
If the naming is confusing, I can change it to is_s3_object_etag_different to be a bit more accurate
| response = self._s3_client.head_object(Bucket=object.bucket, Key=object.key) | ||
| return response.pop("ETag") | ||
| except Exception: | ||
| return "" |
There was a problem hiding this comment.
For this case, can we return None? That seems cleaner than empty string
| hub_bucket_s3_uri = f"s3://{hub_s3_bucket_name}" | ||
| self._sm_client.create_hub( | ||
| HubName=hub_name, | ||
| HubDescription="This is a curated hub.", # TODO verify description |
There was a problem hiding this comment.
can we store these as a constant somewhere
| self._sm_client.describe_hub_content( | ||
| HubName=self.curated_hub_name, | ||
| HubContentName=model_specs.model_id, | ||
| HubContentType="Model", |
There was a problem hiding this comment.
I would make this a constant or enum
|
|
||
| def delete_all_versions_of_model(self, model_specs: JumpStartModelSpecs): | ||
| """Deletes all versions of a model in the Private Hub.""" | ||
| print(f"Deleting model {model_specs.model_id} from curated hub...") |
There was a problem hiding this comment.
same comment on using logging module
| # distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF | ||
| # ANY KIND, either express or implied. See the License for the specific | ||
| # language governing permissions and limitations under the License. | ||
| """This module data structures for Private Hub content.""" |
There was a problem hiding this comment.
i would call this file types.py or something like that
| OTHER = "Other" | ||
|
|
||
|
|
||
| @dataclass |
There was a problem hiding this comment.
this is a cool construct, definitely makes everything cleaner
| DefaultDeploymentConfig: Optional[DefaultDeploymentConfig] | ||
|
|
||
|
|
||
| def convert_public_model_hyperparameter_to_hub_hyperparameter( |
There was a problem hiding this comment.
can we move this to another module? this file seems to only have classes/enums, not utility functions
| self, | ||
| curated_hub_name: str, | ||
| import_to_preexisting_hub: bool = False, | ||
| region: str = "us-west-2", |
There was a problem hiding this comment.
can we use this constant? https://github.com/aws/sagemaker-python-sdk/blob/master/src/sagemaker/jumpstart/constants.py#L130
| self.studio_metadata_map = get_studio_model_metadata_map_from_region(self._region) | ||
| self._init_clients() | ||
|
|
||
| def _get_preexisting_hub_and_s3_bucket_names(self) -> Optional[Tuple[str, str]]: |
There was a problem hiding this comment.
this seems pretty complicated, we should make sure it's unit tested
| name_of_hub_already_on_account = preexisting_hub[0] | ||
|
|
||
| if not import_to_preexisting_hub: | ||
| raise Exception( |
There was a problem hiding this comment.
can we raise a more specific exception?
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
Issue #, if available:
Description of changes:
This is a POC for a new feature, the JumpStart Curated Public Hub. This hub will allow a user to create a Private Hub that contains a subset of models provided in the JumpStart (JS) public hub.
Testing done:
Merge Checklist
Put an
xin the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your pull request.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.