-
Notifications
You must be signed in to change notification settings - Fork 24
feat(sample): featurestore node updates #1028
Conversation
@sai-chaithu Are you a Googler? |
No |
Then we should get someone on the team to approve this. I'll try to find someone. |
This change was asked by @diemtvu |
@dizcology @meltsufin PTAL |
Hi @lesv, I think @sai-chaithu works on behalf of Spring ML. We were running into a Thanks, |
@nayaknishant This is mergable, and I'm happy if you merge it. I'd also be happy to work with you to increase the quota where appropriate, and clean up the testing project(s). Even better is I can show you how to do the increases yourself. Anyway, ping me if you'd like help or need me to do something -- I'm only here because Blunderbus asked me to be, which is why I don't merge non-googler code to a project I don't know anything about. |
Thanks Les! Yep, I understand. I'll reach out with any questions I have 🙂 |
Hey @sai-chaithu I took a look at the Node.js Feature Store (FS) tests (prefix of the FSs created are Thanks, |
@nayaknishant while testing from my local all the featurestores created are deleted except for one of the entityTypeIds for which I have updated the tear down function. Can you please re-run the test cases and check |
Sorry for the slow response. The latest failure shows this error. I wonder if it's something wrong with the
|
Also, I can't see the any operations after the line 217 (FeatureValuesSamplesTest.java) in our server log, i.e no Export of BatchRead. So is this possible that it's already failed at createBigQueryDataset? |
One more request, can you also use different FS id construction in each test, for example 'temp__'. It would help to know which test fail to clean up the resource properly. |
@diemtvu , the test might have failed before bigQuery Dataset was created, due to which in tear down function the delete bigQuery could find the dataset to delete. |
Sure will add that, in the current run the test failed due to CreateFeaturestoreSampleTest, FeaturestoreSamplesTest test cases. Both the test failed with exception "Task was cancelled" before they could create the featurestore. For the FS id's please confirm if the below Prefix are ok,
|
Let's use the file name to make it easy to find, may be stripped the 'SampleTest' postfix if that is common, but also ok to leave it as is. E.g FeatureValuesSamplesTest -> temp_feature_values_ or temp_feature_values_samples_test_ Thanks |
Regarding failure, I found out we have some flaky due to the number of CreateFeatureStore in all the tests. Probably remove CreateFeaturestoreSampleTest as it is covered by the others already. May even go further to merge FeaturestoreSamplesTest and FeatureValuesSamplesTest and FeatureSamplesTest. |
Merged FeatureValuesSamplesTest, FeatureSamplesTest into one test case and FeaturestoreSamplesTest, EntityTypeSamplesTest into one test case and also removed CreateFeaturestoreSampleTest. One featurestore will be created in each test case. |
Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:
Fixes #945 ☕️