fix: support maps in step parameters #2661
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
dpatro
reviewed
Sep 24, 2021
src/sagemaker/workflow/properties.py
Outdated
| Args: | ||
| path (str): The parent path of the PropertiesMap instance. | ||
| shape_name (str): The botocore sagemaker service model shape name. | ||
| root_shape_name (str): The botocore sagemaker service model shape name. |
There was a problem hiding this comment.
root_shape_name is not present in the arguments. We should remove this from doc
staubhp
reviewed
Sep 24, 2021
| @@ -855,6 +855,113 @@ def test_one_step_callback_pipeline(sagemaker_session, role, pipeline_name, regi | |||
| pass | |||
|
|
|||
|
|
|||
| def test_steps_with_map_params_pipeline( | |||
There was a problem hiding this comment.
I might be missing it but how is this testing map params?
There was a problem hiding this comment.
assert at line 947 and 948 are checking hyperparameter values? Do we need different asserts?
There was a problem hiding this comment.
Updated for testing hyperparameters
jayatalr
force-pushed
the
master
branch
2 times, most recently
from
ef8150d to
cb7ed39
Compare
staubhp
approved these changes
Sep 24, 2021
dpatro
approved these changes
Sep 24, 2021
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
mufaddal-rohawala
approved these changes
Sep 28, 2021
ahsan-z-khan
approved these changes
Sep 28, 2021
navinsoni
reviewed
Sep 28, 2021
| @@ -67,6 +67,8 @@ def __init__( | |||
| for key, info in members.items(): | |||
| if Properties._shapes.get(info["shape"], {}).get("type") == "list": | |||
There was a problem hiding this comment.
I think second param should be list here in get
| @@ -67,6 +67,8 @@ def __init__( | |||
| for key, info in members.items(): | |||
| if Properties._shapes.get(info["shape"], {}).get("type") == "list": | |||
| self.__dict__[key] = PropertiesList(f"{path}.{key}", info["shape"]) | |||
| elif Properties._shapes.get(info["shape"], {}).get("type") == "map": | |||
| self.__dict__[key] = PropertiesMap(f"{path}.{key}", info["shape"]) | |||
There was a problem hiding this comment.
I am little confused here. info[shape] is map here so why does constructor has str in it
EthanShouhanCheng
pushed a commit
to SissiChenxy/sagemaker-python-sdk
that referenced
this pull request
Jan 11, 2022
Co-authored-by: Ahsan Khan <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue #, if available:
Description of changes: This change is made to support map types in step properties.
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.