feature: Added support for feature group schema change and feature parameters #3206
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3206 +/- ##
==========================================
+ Coverage 88.82% 88.84% +0.01%
==========================================
Files 203 203
Lines 17913 17938 +25
==========================================
+ Hits 15912 15937 +25
Misses 2001 2001
Continue to review full report at Codecov.
|
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 |
| @@ -537,6 +538,66 @@ def describe(self, next_token: str = None) -> Dict[str, Any]: | |||
| feature_group_name=self.name, next_token=next_token | |||
| ) | |||
|
|
|||
| def update(self, feature_additions: Sequence[FeatureDefinition]) -> Dict[str, Any]: | |||
There was a problem hiding this comment.
feature_definitions is one field in FeatureGroup class. Do we need to append the new features in this field?
There was a problem hiding this comment.
This is actually a very good idea. I think we should. I'll submit a new version tomorrow.
[Not sure why this is not displayed in File Changed section. Thats why missed this one.]
There was a problem hiding this comment.
Actually after a second thought, I think let's skip this and keep it as it is. Because, that is also true for update_feature_metadata. Plus, if we append what's the value? In any way, this feature definition is used in backend for schema validation, in the studio side, customers are not actively using the definitions for any purpose.
There was a problem hiding this comment.
FeatureDefinition only has FeatureName and FeatureType. UpdateFeatureMetadata should not update this field. We should append the new features in this field.
There was a problem hiding this comment.
We should, if this update API is completed synchronously. But in reality, that's not the case. There are two parts in the whole update procedure. First this API call which is synchronous and then, asynchronous workflow. Let's assume this synchronous API succeeds and we append the feature definitions BUT what happens if the asynchronous workflow fails? Then customers will get false impressions that features are actually added in the backend.
Lets keep things simple and the same behavior as of now. If customers want to know they can call describe API.
Later we can publish another SDK method, which will be a combined version of sync + async which will append features in the class as well. But for now, this should be enough. Makes sense?
| Returns: | ||
| Response dict from service. | ||
| """ | ||
| return self.sagemaker_session.update_feature_metadata( |
There was a problem hiding this comment.
Shall we verify the feature is in feature_definitions before calling API?
There was a problem hiding this comment.
Lets depend on backend for that, lets not have that check here. I am hoping backend will react immediately and response with the error message.
There was a problem hiding this comment.
what kind of error is thrown? Can we write an integ test to make sure that's caught in the code correctly?
There was a problem hiding this comment.
I dont think thats a good idea to check that exception in SDK, backend should have that check both in unit and integration tests, having the same here would be a redundant.
| if parameter_additions is not None | ||
| else [], |
There was a problem hiding this comment.
Do we need this if/else?
There was a problem hiding this comment.
parameter_additions = [parameter_addition.to_dict() for parameter_addition in (parameter_additions or [])]
Unless you have code gaurantines that parameter_additions are not None.
There was a problem hiding this comment.
@imingtsou parameter_additions can be None and parameter_addition in parameter_additions would not work on None type. Error will be TypeError: 'NoneType' object is not iterable. Please see unit tests for different use cases.
@XuetaoYin's code snippet would work but that is analogous to current implementation. Just a different style or personal preference.
| request = { | ||
| "FeatureGroupName": feature_group_name, | ||
| "FeatureName": feature_name, | ||
| } | ||
|
|
||
| if description is not None: | ||
| request["Description"] = description | ||
| if parameter_additions is not None: | ||
| request["ParameterAdditions"] = parameter_additions | ||
| if parameter_removals is not None: | ||
| request["ParameterRemovals"] = parameter_removals | ||
|
|
||
| return self.sagemaker_client.update_feature_metadata(**request) |
There was a problem hiding this comment.
Why do we need to process the request?
There was a problem hiding this comment.
Because any field can be None and that needs a check.
There was a problem hiding this comment.
Shall we call the API with None value? I assume we can do something like other APIs?
self.sagemaker_client.update_feature_metadata(
FeatureGroupName=featureGroupName,
FeatureName=featureName,
...
)
There was a problem hiding this comment.
I did this for a purpose. I exactly don't remember at this moment why, but I think either I was getting error or if I did not do this, description got removed for none. If you want I can double check. But i dont think its blocking. Can we move forward?
There was a problem hiding this comment.
We can close this one. Please address the above comment.
There was a problem hiding this comment.
Done. Let me know if you have any.
There was a problem hiding this comment.
I still have this one opened:
feature_definitions is one field in FeatureGroup class. Do we need to append the new features in this field?
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 |
| parameter_additions=[ | ||
| parameter_addition.to_dict() for parameter_addition in parameter_additions | ||
| ] | ||
| if parameter_additions is not None | ||
| else [], |
There was a problem hiding this comment.
Can we do this outside return in a varible
There was a problem hiding this comment.
I refactored the code a bit different way for more readability, please check and let me know.
| ] | ||
| if parameter_additions is not None | ||
| else [], | ||
| parameter_removals=parameter_removals if parameter_removals is not None else [], |
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 |
navinsoni
changed the title
…rameters (aws#3206) Co-authored-by: Mizanur Rahman <[email protected]>
…rameters (aws#3206) Co-authored-by: Mizanur Rahman <[email protected]>
Issue #, if available:
Description of changes:
Added support for feature group schema change and feature parameters
Testing done:
Integration test
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.