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

joshpalis · 2023-11-07T23:24:51Z

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.

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

…itions to workflow step json Signed-off-by: Joshua Palis <[email protected]>

codecov · 2023-11-07T23:27:53Z

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 · 2023-11-08T00:28:27Z

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

owaiskazi19 · 2023-11-08T18:20:11Z

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

@joshpalis raise a separate PR for that.

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

owaiskazi19

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

src/main/java/org/opensearch/flowframework/workflow/RegisterLocalModelStep.java

src/main/java/org/opensearch/flowframework/workflow/RegisterRemoteModelStep.java

joshpalis · 2023-11-08T23:23:04Z

@owaiskazi19 optional params have been handled in this PR

…RemoteModelStep and RegisterLocalModelStep Signed-off-by: Joshua Palis <[email protected]>

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

src/main/java/org/opensearch/flowframework/workflow/RegisterLocalModelStep.java

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

src/main/java/org/opensearch/flowframework/workflow/RegisterLocalModelStep.java

src/main/resources/mappings/workflow-steps.json

src/main/java/org/opensearch/flowframework/workflow/RegisterLocalModelStep.java

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

src/main/java/org/opensearch/flowframework/workflow/RegisterRemoteModelStep.java

owaiskazi19

LGTM! Thanks for patiently addressing all the comment.

dbwiddis

LGTM, and commented on the retry TODO issue

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

…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>

…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]>

joshpalis added 2 commits November 7, 2023 18:52

added RegisterRemoteModelStep and tests

9bb74eb

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

Adding RegisterLocalModelStep, fixing tests, adding input/ouput defin…

d93a581

…itions to workflow step json Signed-off-by: Joshua Palis <[email protected]>

joshpalis added the backport 2.x backport PRs to 2.x branch label Nov 7, 2023

joshpalis requested review from dbwiddis, owaiskazi19, ohltyler, amitgalitz and jackiehanyang as code owners November 7, 2023 23:24

Merge branch 'main' into registerModel

a5997b7

Fixing javadoc warnings, fixing log message

366f05e

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

owaiskazi19 reviewed Nov 8, 2023

View reviewed changes

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 added 2 commits November 8, 2023 23:29

Addressing PR comments,making description field optional for Register…

5f0a357

…RemoteModelStep and RegisterLocalModelStep Signed-off-by: Joshua Palis <[email protected]>

moving modelConfig builder before adding allConfig

285eb6f

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

owaiskazi19 reviewed Nov 10, 2023

View reviewed changes

src/main/java/org/opensearch/flowframework/workflow/RegisterLocalModelStep.java Show resolved Hide resolved

handling optional description field for remote/local model

4866219

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

owaiskazi19 reviewed Nov 10, 2023

View reviewed changes

src/main/java/org/opensearch/flowframework/workflow/RegisterLocalModelStep.java Outdated Show resolved Hide resolved

owaiskazi19 reviewed Nov 10, 2023

View reviewed changes

src/main/java/org/opensearch/flowframework/workflow/RegisterLocalModelStep.java Outdated Show resolved Hide resolved

owaiskazi19 reviewed Nov 10, 2023

View reviewed changes

src/main/resources/mappings/workflow-steps.json Show resolved Hide resolved

owaiskazi19 reviewed Nov 10, 2023

View reviewed changes

src/main/java/org/opensearch/flowframework/workflow/RegisterLocalModelStep.java Show resolved Hide resolved

owaiskazi19 reviewed Nov 10, 2023

View reviewed changes

src/main/java/org/opensearch/flowframework/workflow/RegisterLocalModelStep.java Outdated Show resolved Hide resolved

joshpalis added 4 commits November 10, 2023 00:41

Removing poolingMode, modelMaxLenth, normalizeResult

8348a24

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

adding modelType to required fields check

71eace6

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

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

a0d8a6d

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

Adding GetMLTaskStep and tests

a16c79e

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

joshpalis mentioned this pull request Nov 10, 2023

[FEATURE] Add transport request retry capability for async workflow steps #158

Closed

Adding todo for GetMLTask retry capability

cbd5af7

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

owaiskazi19 reviewed Nov 10, 2023

View reviewed changes

src/main/java/org/opensearch/flowframework/workflow/RegisterRemoteModelStep.java Show resolved Hide resolved

owaiskazi19 approved these changes Nov 10, 2023

View reviewed changes

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 approved these changes Nov 13, 2023

View reviewed changes

Merge main

5796db5

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

joshpalis merged commit 2142874 into opensearch-project:main Nov 13, 2023
19 checks passed

opensearch-trigger-bot bot mentioned this pull request Nov 13, 2023

[Backport 2.x] Separates RegisterModelStep into RegisterLocalModelStep and RegisterRemoteModelStep. Adds GetMLTaskStep. Handles optional params #161

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

joshpalis commented Nov 7, 2023 •

edited

Loading

codecov bot commented Nov 7, 2023 •

edited

Loading

joshpalis commented Nov 8, 2023

owaiskazi19 commented Nov 8, 2023

owaiskazi19 left a comment

joshpalis commented Nov 8, 2023

owaiskazi19 left a comment

dbwiddis left a comment

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

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

Conversation

joshpalis commented Nov 7, 2023 • edited Loading

Description

Issues Resolved

codecov bot commented Nov 7, 2023 • edited Loading

Codecov Report

joshpalis commented Nov 8, 2023

owaiskazi19 commented Nov 8, 2023

owaiskazi19 left a comment

Choose a reason for hiding this comment

joshpalis commented Nov 8, 2023

owaiskazi19 left a comment

Choose a reason for hiding this comment

dbwiddis left a comment

Choose a reason for hiding this comment

joshpalis commented Nov 7, 2023 •

edited

Loading

codecov bot commented Nov 7, 2023 •

edited

Loading