-
Notifications
You must be signed in to change notification settings - Fork 37
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added a noop workflow step to delete model group #376
Added a noop workflow step to delete model group #376
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #376 +/- ##
============================================
+ Coverage 72.30% 72.34% +0.03%
- Complexity 578 581 +3
============================================
Files 72 73 +1
Lines 2986 2990 +4
Branches 231 231
============================================
+ Hits 2159 2163 +4
Misses 723 723
Partials 104 104 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Owais Kazi <[email protected]>
2814a22
to
f0e7630
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few minor comments. A high-level question: why is this a noop instead of doing the actual deletion? Could you add some context in the PR description?
src/main/java/org/opensearch/flowframework/common/WorkflowResources.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/workflow/ModelGroupStep.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/flowframework/workflow/ModelGroupStep.java
Outdated
Show resolved
Hide resolved
Just a heads up, flaky integration tests will be resolved by #377 |
Signed-off-by: Owais Kazi <[email protected]>
0c6177b
to
7f76221
Compare
Added more details. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I assume failing tests are the flaky ones mentioned.
(cherry picked from commit 324a56a) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this all works but why do we need a whole class that's a clone of NoopStep
?
Why not just do this?
REGISTER_MODEL_GROUP(RegisterModelGroupStep.NAME, WorkflowResources.MODEL_GROUP_ID, NoopStep.NAME),
Description
Added a noop workflow step to delete model group. The reason of the noop step is that the model group is deleted on its own when there are no model in it. To handle the null case in deprovisioning for register model group step, created this noop workflow step to avoid any conflict with the resources created.
Issues Resolved
Fixes #365
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.