Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Separates RegisterModelStep into RegisterLocalModelStep and RegisterRemoteModelStep. Adds GetMLTaskStep. Handles optional params #155

Merged
merged 13 commits into from
Nov 13, 2023
Merged

Separates RegisterModelStep into RegisterLocalModelStep and RegisterRemoteModelStep. Adds GetMLTaskStep. Handles optional params #155

merged 13 commits into from
Nov 13, 2023

Conversation

joshpalis
Copy link
Member

@joshpalis joshpalis commented Nov 7, 2023 •
edited
Loading

Description

Creates a new workflow step to register a local model based on API documentation

Creates a separate workflow step to register a remote model based on API Documentation.

Creates a GetMLTaskStep to retrieve the model ID given the task ID outputted by the RegisterLocalModelStep
(Necessary to first merge in opensearch-project/ml-commons#1616 prior to this PR)

Note : I have not added the internal connector configuration capability to the RegisterRemoteModelStep since we already have a CreateConnectorStep, but have marked it as a TODO later down the line

Issues Resolved

#149
Part of #88

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.

@joshpalis joshpalis added the backport 2.x backport PRs to 2.x branch label Nov 7, 2023
@codecov Codecov
Copy link

codecov bot commented Nov 7, 2023 •
edited
Loading

Codecov Report

Merging #155 (cbd5af7) into main (56ccb1d) will increase coverage by 1.09%.
The diff coverage is 90.37%.

❗ Current head cbd5af7 differs from pull request most recent head 5796db5. Consider uploading reports for the commit 5796db5 to get more accurate results

@@             Coverage Diff              @@
##               main     #155      +/-   ##
============================================
+ Coverage     68.39%   69.48%   +1.09%     
- Complexity      339      357      +18     
============================================
  Files            45       45              
  Lines          1585     1642      +57     
  Branches        143      153      +10     
============================================
+ Hits           1084     1141      +57     
+ Misses          452      447       -5     
- Partials         49       54       +5
Files Coverage Δ
...g/opensearch/flowframework/common/CommonValue.java 100.00% <ø> (ø)
...owframework/rest/AbstractSearchWorkflowAction.java 67.85% <ø> (ø)
...pensearch/flowframework/util/RestHandlerUtils.java 0.00% <ø> (ø)
...nsearch/flowframework/workflow/ModelGroupStep.java 88.67% <100.00%> (ø)
...ch/flowframework/workflow/WorkflowStepFactory.java 100.00% <100.00%> (ø)
...framework/indices/FlowFrameworkIndicesHandler.java 29.18% <0.00%> (ø)
...ensearch/flowframework/workflow/GetMLTaskStep.java 92.85% <92.85%> (ø)
...lowframework/workflow/RegisterRemoteModelStep.java 88.23% <86.95%> (ø)
...flowframework/workflow/RegisterLocalModelStep.java 91.13% <91.13%> (ø)

... and 8 files with indirect coverage changes

@joshpalis
Copy link
Member Author

Still need to handle create connector optional params, will include in the next commit

@owaiskazi19
Copy link
Member

Still need to handle create connector optional params, will include in the next commit

@joshpalis raise a separate PR for that.

owaiskazi19
owaiskazi19 reviewed Nov 8, 2023
View reviewed changes
Copy link
Member

@owaiskazi19 owaiskazi19 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we planning to have a separate PR for optional params of Register Steps?

src/main/java/org/opensearch/flowframework/workflow/RegisterLocalModelStep.java Outdated Show resolved Hide resolved
src/main/java/org/opensearch/flowframework/workflow/RegisterRemoteModelStep.java Outdated Show resolved Hide resolved
@joshpalis
Copy link
Member Author

@owaiskazi19 optional params have been handled in this PR

@joshpalis joshpalis mentioned this pull request Nov 10, 2023
Closed
owaiskazi19
owaiskazi19 approved these changes Nov 10, 2023
View reviewed changes
Copy link
Member

@owaiskazi19 owaiskazi19 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for patiently addressing all the comment.

@joshpalis joshpalis changed the title Separates RegisterModelStep into RegisterLocalModelStep and RegisterRemoteModelStep. Handles optional params Separates RegisterModelStep into RegisterLocalModelStep and RegisterRemoteModelStep. Adds GetMLTaskStep. Handles optional params Nov 10, 2023
dbwiddis
dbwiddis approved these changes Nov 13, 2023
View reviewed changes
Copy link
Member

@dbwiddis dbwiddis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, and commented on the retry TODO issue

@joshpalis
Merge main
5796db5 
Signed-off-by: Joshua Palis <[email protected]>
@joshpalis joshpalis merged commit 2142874 into opensearch-project:main Nov 13, 2023
19 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Nov 13, 2023
@github-actions
Separates RegisterModelStep into RegisterLocalModelStep and RegisterR…
d4a97b3 
…emoteModelStep. Adds GetMLTaskStep. Handles optional params (#155)

* added RegisterRemoteModelStep and tests

Signed-off-by: Joshua Palis <[email protected]>

* Adding RegisterLocalModelStep, fixing tests, adding input/ouput definitions to workflow step json

Signed-off-by: Joshua Palis <[email protected]>

* Fixing javadoc warnings, fixing log message

Signed-off-by: Joshua Palis <[email protected]>

* Addressing PR comments,making description field optional for RegisterRemoteModelStep and RegisterLocalModelStep

Signed-off-by: Joshua Palis <[email protected]>

* moving modelConfig builder before adding allConfig

Signed-off-by: Joshua Palis <[email protected]>

* handling optional description field for remote/local model

Signed-off-by: Joshua Palis <[email protected]>

* Removing poolingMode, modelMaxLenth, normalizeResult

Signed-off-by: Joshua Palis <[email protected]>

* adding modelType to required fields check

Signed-off-by: Joshua Palis <[email protected]>

* Fixing RegisterLocalModelStep to output a task ID instead of a model id

Signed-off-by: Joshua Palis <[email protected]>

* Adding GetMLTaskStep and tests

Signed-off-by: Joshua Palis <[email protected]>

* Adding todo for GetMLTask retry capability

Signed-off-by: Joshua Palis <[email protected]>

---------

Signed-off-by: Joshua Palis <[email protected]>
(cherry picked from commit 2142874)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request Nov 13, 2023
Merged
joshpalis added a commit that referenced this pull request Nov 13, 2023
@opensearch-trigger-bot @github-actions @joshpalis
[Backport 2.x] Separates RegisterModelStep into RegisterLocalModelSte…
4813fc0 
…p and RegisterRemoteModelStep. Adds GetMLTaskStep. Handles optional params (#161)

Separates RegisterModelStep into RegisterLocalModelStep and RegisterRemoteModelStep. Adds GetMLTaskStep. Handles optional params (#155)

* added RegisterRemoteModelStep and tests



* Adding RegisterLocalModelStep, fixing tests, adding input/ouput definitions to workflow step json



* Fixing javadoc warnings, fixing log message



* Addressing PR comments,making description field optional for RegisterRemoteModelStep and RegisterLocalModelStep



* moving modelConfig builder before adding allConfig



* handling optional description field for remote/local model



* Removing poolingMode, modelMaxLenth, normalizeResult



* adding modelType to required fields check



* Fixing RegisterLocalModelStep to output a task ID instead of a model id



* Adding GetMLTaskStep and tests



* Adding todo for GetMLTask retry capability



---------


(cherry picked from commit 2142874)

Signed-off-by: Joshua Palis <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: Joshua Palis <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dbwiddis dbwiddis dbwiddis approved these changes

@owaiskazi19 owaiskazi19 owaiskazi19 approved these changes

@ohltyler ohltyler Awaiting requested review from ohltyler ohltyler is a code owner

@amitgalitz amitgalitz Awaiting requested review from amitgalitz amitgalitz is a code owner

@jackiehanyang jackiehanyang Awaiting requested review from jackiehanyang jackiehanyang is a code owner

Assignees
No one assigned
Labels
backport 2.x backport PRs to 2.x branch
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@joshpalis @owaiskazi19 @dbwiddis