fix: alt configs model deployment and training issues #4833
Conversation
malav-shastri
temporarily deployed
to
auto-approve
GitHub Actions
Inactive
| @@ -259,6 +259,7 @@ def _add_instance_type_to_kwargs( | |||
| sagemaker_session=kwargs.sagemaker_session, | |||
| model_type=kwargs.model_type, | |||
| config_name=kwargs.config_name, | |||
| hub_arn=kwargs.hub_arn, | |||
There was a problem hiding this comment.
We're going to be finding these lines forever aren't we....
There was a problem hiding this comment.
this is an optional field but looks like integ tests could be a possible solution here
There was a problem hiding this comment.
Yes, we desperately need integ tests for (at least):
- ModelReference fine-tuning
- ModelReference deploy
- Alt config deploy
src/sagemaker/jumpstart/types.py
Outdated
| @@ -1278,7 +1278,7 @@ def from_json(self, json_obj: Dict[str, Any]) -> None: | |||
| json_obj (Dict[str, Any]): Dictionary representation of spec. | |||
| """ | |||
| if self._is_hub_content: | |||
| json_obj = walk_and_apply_json(json_obj, camel_to_snake) | |||
| json_obj = walk_and_apply_json(json_obj, camel_to_snake, ["metrics"]) | |||
There was a problem hiding this comment.
nit- you can remove ["metrics"] since it is the default value
malav-shastri
temporarily deployed
to
auto-approve
GitHub Actions
Inactive
malav-shastri
temporarily deployed
to
auto-approve
GitHub Actions
Inactive
malav-shastri
temporarily deployed
to
auto-approve
GitHub Actions
Inactive
malav-shastri
temporarily deployed
to
auto-approve
GitHub Actions
Inactive
| lambda metric_definition: metric_definition["Name"] | ||
| != instance_specific_metric_name, | ||
| default_metric_definitions, | ||
| if instance_specific_metric_definitions: |
There was a problem hiding this comment.
nit:
if not instance_specific_metric_definitions:
return default_metric_definitions
| @@ -10,16 +10,20 @@ | |||
| # 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. | |||
| # pylint: skip-file | |||
There was a problem hiding this comment.
nit: generally not a fan of skipping entire file, we should fix in a follow-up PR
There was a problem hiding this comment.
me and @bencrabtree just discuss this, there's one apparently pylint false positive which we are not able to get rid of. Skipping it to unblock customer but sure I can followup
| new[new_key] = [] | ||
| for item in value: | ||
| _walk_and_apply_json(item, new=new[new_key]) | ||
| if (stop_keys and new_key not in stop_keys) or stop_keys is None: |
There was a problem hiding this comment.
nit:
if stop_keys and new_key in stop_keys:
new[new_key] = value
else:
...
There was a problem hiding this comment.
+1, conditional is too complex. If you want to keep this condition, create a utils and unit-test separately please.
malav-shastri
temporarily deployed
to
auto-approve
GitHub Actions
Inactive
malav-shastri
temporarily deployed
to
auto-approve
GitHub Actions
Inactive
malav-shastri
temporarily deployed
to
auto-approve
GitHub Actions
Inactive
|
|
||
|
|
||
| def camel_to_snake(camel_case_string: str) -> str: | ||
| """Converts camelCaseString or UpperCamelCaseString to snake_case_string.""" | ||
| snake_case_string = re.sub("(.)([A-Z][a-z]+)", r"\1_\2", camel_case_string) | ||
| if "-" in snake_case_string: | ||
| # remove any hyphen from the string for accurate conversion. |
There was a problem hiding this comment.
nit: unnecessary comment
| def walk_and_apply_json(json_obj: Dict[Any, Any], apply) -> Dict[Any, Any]: | ||
| """Recursively walks a json object and applies a given function to the keys.""" | ||
| def walk_and_apply_json( | ||
| json_obj: Dict[Any, Any], apply, stop_keys: Optional[List[str]] = ["metrics"] |
There was a problem hiding this comment.
non-blocking: apply typing please
| new[new_key] = [] | ||
| for item in value: | ||
| _walk_and_apply_json(item, new=new[new_key]) | ||
| if (stop_keys and new_key not in stop_keys) or stop_keys is None: |
There was a problem hiding this comment.
+1, conditional is too complex. If you want to keep this condition, create a utils and unit-test separately please.
| for config_name in config_names | ||
| } | ||
| if metadata_configs | ||
| else {} |
There was a problem hiding this comment.
ternary + list-comprehension is a recipe for unreadability. Consider:
if metadata_configs:
return { ... }
else:
return {}
Issue #, if available:
Description of changes:
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.