Feature Group Testing #45

BriannaRoskind · 2021-06-16T17:35:26Z

Summary:
Generated Feature Group controller code and wrote passing integration tests for the Feature Group controller.

Files Added:

test/e2e/tests/test_feature_group.py - Contains Create and Delete integration test for feature group
test/e2e/resources/feature_group.yaml - yaml file for feature group creation in integration testing.
pkg/resource/feature_group/custom_delta.go - Sets default spec values for feature group fields (DisableGlueTableCreation and S3StorageConfig) if the user did not define them.

Files Edited:

test/e2e/service_bootstrap.py - Attached AmazonSageMakerFeatureStoreAccess Role Policy to the SageMaker Execution Role for testing purposes. (Feature Store requires this permission for the Feature Group creation and access)
generator.yaml - Commented out feature group from the ignore list. Also added feature group exceptions, hooks, and fields.
pkg/resource/feature_group/delta.go - added the CustumSetDefault() function from custom_delta.go to the newResourceDelta() function.

… exceptions.

…n the service_bootstrap.py.

… a tmp folder for testing efficiency of the feature group integration tests.

ack-bot · 2021-06-16T17:35:34Z

Hi @BriannaRoskind. Thanks for your PR.

I'm waiting for a aws-controllers-k8s member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

…after the delete requeue PR is merged.

…the Feature Group DeleteFailed state after the delete requeue PR is merged.

jaypipes

@BriannaRoskind I believe you forgot to git add pkg/resource/featuregroup/ :) In addition, please do remember to write a full PR description and summary message.

Thanks!
-jay

…ntroller into main

… commit.

…eation and S3StorageConfig by creating a custom_delta.go file. This sets their default values if the user did not specify them, and it is called via the delta_pre_compare hook (added in generator.yaml).

jaypipes · 2021-06-21T22:13:23Z

apis/v1alpha1/types.go


+// A group of versioned models in the model registry.
+type ModelPackageGroup struct {
+	CreationTime *metav1.Time `json:"creationTime,omitempty"`


I see the CreationTime field has been added to a whole bunch of resources and type definitions... is this because you are using a new(er) version of aws-sdk-go to build the sagemaker-controller? If so, it would be best to regenerate the sagemaker-controller with that new aws-sdk-go version but without adding the new FeatureGroup resource. That way, we can isolate the changes in this PR to only those introduced due to the new FeatureGroup resource. Make sense?

The funny thing is... I don't actually see a change to the go.mod file in this PR... @BriannaRoskind did you happen to regenerate the sagemaker-controller and pass in a specific aws-sdk-go version to generate the controller with but forgot to change the go.mod file in the sagemaker-controller repo?

+1 please check this out. types unrelated to this resources are getting added like ModelPackage and Notebook

jaypipes · 2021-06-21T22:16:45Z

test/e2e/tests/test_feature_group.py

+        # time out errors at a 10 second wait period.
+        _, deleted = k8s.delete_custom_resource(reference, 3, 15)
+        assert deleted
+        # TODO: Once the delete requeue PR is merged,


which PR is the above referring to? :)

good question. I guess she is looking for this https://github.com/aws-controllers-k8s/runtime/pull/21/files

surajkota

Few comments.
Were you planning to create a separate PR to handle requeueOnCreate until Created?

You can also handle long-running delete like in here - https://github.com/aws-controllers-k8s/elasticache-controller/pull/34/files

surajkota · 2021-06-21T23:54:06Z

apis/v1alpha1/types.go


+// A group of versioned models in the model registry.
+type ModelPackageGroup struct {
+	CreationTime *metav1.Time `json:"creationTime,omitempty"`


+1 please check this out. types unrelated to this resources are getting added like ModelPackage and Notebook

surajkota · 2021-06-21T23:58:33Z

generator.yaml

+        - IncompleteSignature
+        - MissingAuthenticationToken
+        - NotAuthorized
+        - ValidationError


as discussed remove the auth related errors and ValidationError from terminal. Validation mainly because I have seen SM service wrap a lot of different error under this and we cant be sure if all of them are terminal. Auth related because these can be corrected without changing the resource itself

surajkota · 2021-06-22T00:00:57Z

generator.yaml

+    fields:
+      FailureReason:
+        is_read_only: true
+        is_printable: true


update code gen and use new format aws-controllers-k8s/code-generator#82

surajkota · 2021-06-22T00:03:21Z

pkg/resource/feature_group/custom_delta.go

+		}
+	}
+
+	// ResolvedOutputS3URI has a timestamped generated default value, it cannot be null.


nit: it cannot be null is a bit confusing, IMO you can remove these words and the meaning of the sentence will remain same

test/e2e/resources/feature_group.yaml

surajkota · 2021-06-22T00:06:50Z

test/e2e/service_bootstrap.py

        RoleName=role_name, PolicyArn="arn:aws:iam::aws:policy/AmazonS3FullAccess"
    )
+    # This policy accommodates use of the feature group controller.
+    iam.attach_role_policy(


did you check what is present in this policy that is not in SagemakerFullAccess?

surajkota · 2021-06-22T00:16:18Z

test/e2e/tests/test_feature_group.py

+        # time out errors at a 10 second wait period.
+        _, deleted = k8s.delete_custom_resource(reference, 3, 15)
+        assert deleted
+        # TODO: Once the delete requeue PR is merged,


good question. I guess she is looking for this https://github.com/aws-controllers-k8s/runtime/pull/21/files

surajkota · 2021-06-22T00:18:02Z

test/e2e/tests/test_feature_group.py

+            k8s.get_resource_arn(resource)
+            == get_sagemaker_feature_group(feature_group_name)["FeatureGroupArn"]
+        )
+


add check for created status

… we still see the ACK.Recoverable Condition being set alternatingly to true/false.

…print format, and removed authorization related errors from the terminal_codes of feature group.

…in test_feature_group.py, but the test file has not yet been tested.

ack-bot · 2021-06-25T23:12:07Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: BriannaRoskind
To complete the pull request process, please assign vijtrip2 after the PR has been reviewed.
You can assign the PR to them by writing /assign @vijtrip2 in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

…est version with wait for Completing status.

… status before asserting we have reached Created status.

Ubuntu added 4 commits June 15, 2021 19:14

Added feature group controller and tests (updated generator.yaml with…

e3970e5

… exceptions.

Feature group test passes. Needed to keep in the extra policy added i…

64f08ac

…n the service_bootstrap.py.

Added tests back into test file that had been previously relocated to…

ce3110d

… a tmp folder for testing efficiency of the feature group integration tests.

Added more comments to test_feature_group.py.

0a5d24a

BriannaRoskind requested review from mbaijal and surajkota as code owners June 16, 2021 17:35

ack-bot requested review from a-hilaly and jaypipes June 16, 2021 17:35

ack-bot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 16, 2021

Ubuntu added 2 commits June 16, 2021 19:36

Added TODO to revisint how feature group resolved DeleteFailed state …

a468b5b

…after the delete requeue PR is merged.

Added a #TODO to check that the feature group controller can resolve …

8d697cf

…the Feature Group DeleteFailed state after the delete requeue PR is merged.

jaypipes suggested changes Jun 16, 2021

View reviewed changes

Ubuntu added 2 commits June 16, 2021 19:47

Merge branch 'main' of https://github.com/BriannaRoskind/sagemaker-co…

d1da0f5

…ntroller into main

Added pkg/resource/feature_group files that were left out of previous…

e1de2c9

… commit.

BriannaRoskind requested a review from jaypipes June 16, 2021 20:11

Fixed feature group reconciling errors for fields: DisableGlueTableCr…

67638ad

…eation and S3StorageConfig by creating a custom_delta.go file. This sets their default values if the user did not specify them, and it is called via the delta_pre_compare hook (added in generator.yaml).

BriannaRoskind mentioned this pull request Jun 21, 2021

Feature group sample #46

Merged

jaypipes reviewed Jun 21, 2021

View reviewed changes

Added reconciling correction for feature group DataCatalogConfig.

636a9dc

surajkota requested changes Jun 22, 2021

View reviewed changes

Ubuntu and others added 4 commits June 24, 2021 18:42

Code compiles with runtime version 0.2.3. If a feature group is made,…

e2bee67

… we still see the ACK.Recoverable Condition being set alternatingly to true/false.

updated generator.yaml feature group fields to have updated code-gen …

a687f5e

…print format, and removed authorization related errors from the terminal_codes of feature group.

Changed comments in custom_delta to read easier, and added an assert …

14bb56e

…in test_feature_group.py, but the test file has not yet been tested.

Merge branch 'aws-controllers-k8s:main' into main

c6da470

Ubuntu added 2 commits June 28, 2021 14:41

Completed git merge, need to update feature group testing file to lat…

954c95d

…est version with wait for Completing status.

Added waiting helper function to ensure feature group passes Creating…

5c562bb

… status before asserting we have reached Created status.

BriannaRoskind closed this Jun 28, 2021

BriannaRoskind deleted the branch aws-controllers-k8s:main June 28, 2021 18:55

BriannaRoskind deleted the main branch June 28, 2021 18:55

BriannaRoskind restored the main branch June 28, 2021 19:37

Feature Group Testing #45

Feature Group Testing #45

Uh oh!

Conversation

BriannaRoskind commented Jun 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ack-bot commented Jun 16, 2021

Uh oh!

jaypipes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

surajkota Jun 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

surajkota left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

surajkota Jun 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ack-bot commented Jun 25, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

BriannaRoskind commented Jun 16, 2021 •

edited

Loading

surajkota Jun 22, 2021 •

edited

Loading

surajkota Jun 22, 2021 •

edited

Loading