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.

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

Modifies use case template format and adds graph validation when provisioning #119

Merged
merged 17 commits into from
Oct 31, 2023

Conversation

joshpalis
Copy link
Member

@joshpalis joshpalis commented Oct 27, 2023

Description

Hoping to merge in #118 first prior to this PR.

This PR achieves multiple things

  • Modifies the use case template to separate workflow inputs into previous_node_inputs and user_inputs as described by [DOCS] Use Case Template Format #117
  • Introduces a workflow-step.json to define the expected inputs and outputs for each workflow step
  • Adds graph validation based on the workflow-step.json. Basically for each ProcessNode in a topologically sorted graph, we retrieve all the expected outputs of its predecessor nodes, combine this with the given user inputs (WorkflowData content) for the current node, and compare this list with the expected inputs of the current node type. If validation fails, we return to the user a list of missing inputs
  • Adds tests to ensure that each registered WorkflowStep in the WorkflowStepFactory has a corresponding WorkflowStepValidator in the workflow-steps.json. This is a guard-rail to ensure that each new step that is registered has their inputs/outputs defined for use in validation

Example create workflow request to register and deploy a model (with missing create_connector step that provides the connector_id needed for the register_model_step)

curl -i -XPOST "localhost:9200/_plugins/_flow_framework/workflow" -H "Content-Type:application/json" --data '{"name":"deploy-register-model","description":"test case","use_case":"TEST_USE_CASE","version":{"template":"1.0.0","compatibility":["2.12.0","3.0.0"]},"workflows":{"provision":{"nodes":[{"id":"workflow_step_1","type":"register_model","user_inputs":{"name":"openAI-gpt-3.5-turbo","function_name":"remote","description":"test model"}},{"id":"workflow_step_2","type":"deploy_model","user_inputs":{}}],"edges":[{"source":"workflow_step_1","dest":"workflow_step_2"}]}}}'
HTTP/1.1 200 OK
X-OpenSearch-Version: OpenSearch/3.0.0-SNAPSHOT (opensearch)
content-type: application/json; charset=UTF-8
content-length: 38

{"workflow_id":"ewChbosBllTp7O9ki-A5"}%     

Executing async provisioning for workflow ewChbosBllTp7O9ki-A5

curl -i -XPOST "localhost:9200/_plugins/_flow_framework/workflow/ewChbosBllTp7O9ki-A5/_provision" 
HTTP/1.1 200 OK
X-OpenSearch-Version: OpenSearch/3.0.0-SNAPSHOT (opensearch)
content-type: application/json; charset=UTF-8
content-length: 38

{"workflow_id":"ewChbosBllTp7O9ki-A5"}%     

OpenSearch logs show that graph validation failed and the connector_id field was not provided by user_inputs or predecessor_node_inputs

[2023-10-27T00:56:02,613][INFO ][o.o.f.w.CreateIndexStep  ] [dev-dsk-jpalis-2c-27c8aa11.us-west-2.amazon.com] create index:.plugins-ai-global-context
[2023-10-27T00:56:20,580][ERROR][o.o.f.t.ProvisionWorkflowTransportAction] [dev-dsk-jpalis-2c-27c8aa11.us-west-2.amazon.com] Provisioning failed for workflow ewChbosBllTp7O9ki-A5 : org.opensearch.flowframework.exception.FlowFrameworkException: Invalid graph, missing the following required inputs : [connector_id]

Issues Resolved

Part of #88
#18
#67
#117

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.

…s into previous_node_inputs and user_inputs, adds graph validation after topologically sorting a workflow into a list of ProcessNode

Signed-off-by: Joshua Palis <[email protected]>
@github-actions github-actions bot added the backport 2.x backport PRs to 2.x branch label Oct 27, 2023
Signed-off-by: Joshua Palis <[email protected]>
@codecov
Copy link

codecov bot commented Oct 27, 2023

Codecov Report

Merging #119 (8ba9d0b) into main (7d9cd84) will increase coverage by 1.96%.
The diff coverage is 93.45%.

@@             Coverage Diff              @@
##               main     #119      +/-   ##
============================================
+ Coverage     67.09%   69.05%   +1.96%     
- Complexity      276      302      +26     
============================================
  Files            35       37       +2     
  Lines          1319     1393      +74     
  Branches        125      132       +7     
============================================
+ Hits            885      962      +77     
+ Misses          390      386       -4     
- Partials         44       45       +1     
Files Coverage Δ
...g/opensearch/flowframework/common/CommonValue.java 100.00% <ø> (ø)
...arch/flowframework/indices/FlowFrameworkIndex.java 100.00% <ø> (ø)
...arch/flowframework/model/ProvisioningProgress.java 100.00% <ø> (ø)
...java/org/opensearch/flowframework/model/State.java 100.00% <ø> (ø)
...g/opensearch/flowframework/model/WorkflowNode.java 91.35% <100.00%> (+0.81%) ⬆️
...rch/flowframework/model/WorkflowStepValidator.java 100.00% <100.00%> (ø)
...nsearch/flowframework/model/WorkflowValidator.java 100.00% <100.00%> (ø)
...search/flowframework/workflow/CreateIndexStep.java 85.29% <100.00%> (ø)
...owframework/workflow/CreateIngestPipelineStep.java 90.62% <100.00%> (ø)
.../flowframework/workflow/WorkflowProcessSorter.java 97.80% <100.00%> (+0.70%) ⬆️
... and 3 more

... and 4 files with indirect coverage changes

Signed-off-by: Joshua Palis <[email protected]>
@joshpalis joshpalis marked this pull request as ready for review October 27, 2023 22:17
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.

Looks good overall with minor comments. Thanks for adding the validation. This is a great addition.
Let's try to break the PR into multiple PRs next time for easy review.

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.

Looks great, a few suggestions.

joshpalis pushed a commit that referenced this pull request Oct 31, 2023
* Remove Demo Classes, add No-Op Step

Signed-off-by: Daniel Widdis <[email protected]>

* Add test coverage

Signed-off-by: Daniel Widdis <[email protected]>

* Revert enum javadocs as they are handled by #119

Signed-off-by: Daniel Widdis <[email protected]>

---------

Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Owais Kazi <[email protected]>
Co-authored-by: Owais Kazi <[email protected]>
opensearch-trigger-bot bot pushed a commit that referenced this pull request Oct 31, 2023
* Remove Demo Classes, add No-Op Step

Signed-off-by: Daniel Widdis <[email protected]>

* Add test coverage

Signed-off-by: Daniel Widdis <[email protected]>

* Revert enum javadocs as they are handled by #119

Signed-off-by: Daniel Widdis <[email protected]>

---------

Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Owais Kazi <[email protected]>
Co-authored-by: Owais Kazi <[email protected]>
(cherry picked from commit 9dffc0a)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
joshpalis pushed a commit that referenced this pull request Oct 31, 2023
Remove Demo Classes, add No-Op Step (#129)

* Remove Demo Classes, add No-Op Step



* Add test coverage



* Revert enum javadocs as they are handled by #119



---------




(cherry picked from commit 9dffc0a)

Signed-off-by: Daniel Widdis <[email protected]>
Signed-off-by: Owais Kazi <[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: Owais Kazi <[email protected]>
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.

Sorry, one last change please (but I clicked approve so once that's fixed I don't need to re-review.)

Signed-off-by: Joshua Palis <[email protected]>
@joshpalis joshpalis merged commit ac76a44 into opensearch-project:main Oct 31, 2023
21 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request Oct 31, 2023
…isioning (#119)

* Simplifying Template format, removing operations, resources created, user outputs

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

* Initial commit, modifies use case template to seperate workflow inputs into previous_node_inputs and user_inputs, adds graph validation after topologically sorting a workflow into a list of ProcessNode

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

* Adding tests

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

* Adding validate graph test

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

* Addressing PR comments, moving sorting/validating prior to executing async, adding success test case for graph validation

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

* Adding javadocs

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

* Moving validation prior to updating workflow state to provisioning

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

* Addressing PR comments Part 1

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

* Addressing PR comments Part 2 : Moving field names to common value class and using constants

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

* Adding definition for noop workflow step

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

* Addressing PR comments Part 3

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

---------

Signed-off-by: Joshua Palis <[email protected]>
(cherry picked from commit ac76a44)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
owaiskazi19 pushed a commit that referenced this pull request Oct 31, 2023
…ation when provisioning (#134)

Modifies use case template format and adds graph validation when provisioning (#119)

* Simplifying Template format, removing operations, resources created, user outputs



* Initial commit, modifies use case template to seperate workflow inputs into previous_node_inputs and user_inputs, adds graph validation after topologically sorting a workflow into a list of ProcessNode



* Adding tests



* Adding validate graph test



* Addressing PR comments, moving sorting/validating prior to executing async, adding success test case for graph validation



* Adding javadocs



* Moving validation prior to updating workflow state to provisioning



* Addressing PR comments Part 1



* Addressing PR comments Part 2 : Moving field names to common value class and using constants



* Adding definition for noop workflow step



* Addressing PR comments Part 3



---------


(cherry picked from commit ac76a44)

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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x backport PRs to 2.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants