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

Add WorkflowStep Factory and implement XContent-based Template Parsing #47

Merged
merged 27 commits into from
Sep 28, 2023
Merged

Add WorkflowStep Factory and implement XContent-based Template Parsing #47

merged 27 commits into from
Sep 28, 2023

Conversation

dbwiddis
Copy link
Member

@dbwiddis dbwiddis commented Sep 19, 2023

Description

  1. Creates a WorkflowStepFactory class to map a template "step type" to the correct instance
  2. Parses the entire Use Case Template into XContent
  3. Process the user inputs section for the workflow into WorkflowData params
  4. Processes the user inputs section for each node into WorkflowData content
  5. Refactors code to separate parsing functionality (WorkflowNode et. al.) from execution functionality (ProcessNode et. al.). Uses the WorfklowStep and Workflow Data when creating the ProcessNode.
  6. Implements an executor service to use the OpenSearch thread pool for the async execution.
  7. Added tests.
  8. Fixed the ProcessNodeTests bug

Issues Resolved

Fixes #41
Fixes #42
Fixes #46
Fixes #50
Fixes #55

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.

@github-actions github-actions bot added the backport 2.x backport PRs to 2.x branch label Sep 19, 2023
@codecov
Copy link

codecov bot commented Sep 21, 2023

Codecov Report

Merging #47 (2fca71b) into main (1d22bee) will increase coverage by 13.05%.
The diff coverage is 90.14%.

@@              Coverage Diff              @@
##               main      #47       +/-   ##
=============================================
+ Coverage     73.87%   86.93%   +13.05%     
- Complexity       65      163       +98     
=============================================
  Files             8       13        +5     
  Lines           245      574      +329     
  Branches         22       75       +53     
=============================================
+ Hits            181      499      +318     
+ Misses           54       52        -2     
- Partials         10       23       +13     
Files Coverage Δ
...nsearch/flowframework/model/PipelineProcessor.java 100.00% <100.00%> (ø)
...a/org/opensearch/flowframework/model/Workflow.java 100.00% <100.00%> (ø)
...search/flowframework/workflow/CreateIndexStep.java 84.84% <100.00%> (ø)
...owframework/workflow/CreateIngestPipelineStep.java 90.62% <ø> (ø)
...opensearch/flowframework/workflow/ProcessNode.java 58.69% <88.88%> (ø)
.../opensearch/flowframework/FlowFrameworkPlugin.java 0.00% <0.00%> (ø)
...g/opensearch/flowframework/model/WorkflowEdge.java 84.84% <84.84%> (ø)
.../flowframework/workflow/WorkflowProcessSorter.java 92.85% <92.85%> (ø)
...g/opensearch/flowframework/model/WorkflowNode.java 90.54% <90.54%> (ø)
...a/org/opensearch/flowframework/model/Template.java 92.36% <92.36%> (ø)
... and 1 more

@dbwiddis dbwiddis changed the title [WIP] WorkflowStep Factory Add WorkflowStep Factory and implement XContent-based Template Parsing Sep 22, 2023
@dbwiddis dbwiddis marked this pull request as ready for review September 23, 2023 06:01
@dbwiddis
Copy link
Member Author

Flipping back to draft to integrate the commits from #24 and #44 into the factory class (and delete my demo version).

@dbwiddis dbwiddis marked this pull request as draft September 25, 2023 21:14
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
@dbwiddis dbwiddis marked this pull request as ready for review September 26, 2023 02:32
Signed-off-by: Daniel Widdis <widdis@gmail.com>
@dbwiddis
Copy link
Member Author

And flipping back to ready for review, after rebasing and also completing tasks from #42 and fixing #50.

Copy link
Member

@joshpalis joshpalis left a comment

Choose a reason for hiding this comment

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

initial pass, still need to go over the test classes

Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
@dbwiddis
Copy link
Member Author

To ease review, a general summary of the main part of this PR:

  • Parsing. Starts with Template, which also parses workflow, which also parses WorkflowEdge and WorkflowNode.
  • Process Sorting: takes the WorflowNodes and WorkflowEdges and creates the sorted ProcessNode sequence with links to predecessor ProcessNodes (no change from previous code, just separated out the parsing part)
  • WorkflowStepFactory: takes the node "type" field and generates a unique instance of the correct workflow step

Additional changes:

  • Made the sorter and step factory singletons taking the needed arguments that will go into the process nodes and workflow steps
  • Passed an executor service around to have better control of threading. This can be changed in the future away from the generic one.
  • Misc. cleanup of warnings (casting, deprecation)

Signed-off-by: Daniel Widdis <widdis@gmail.com>
Signed-off-by: Daniel Widdis <widdis@gmail.com>
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.

Initial Pass! Thanks for adding XContent functionality to the project

Signed-off-by: Daniel Widdis <widdis@gmail.com>
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 a minor comment/question.

@joshpalis
Copy link
Member

Thanks @dbwiddis for adding the Pipeline Processor parsers!

@dbwiddis dbwiddis merged commit 734f9c2 into opensearch-project:main Sep 28, 2023
10 checks passed
@dbwiddis dbwiddis deleted the step-factory branch September 28, 2023 00:58
opensearch-trigger-bot bot pushed a commit that referenced this pull request Sep 28, 2023
#47)

* Add WorkflowStepFactory class

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* Add XContent classes representing Template JSON

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* Add parse methods for the Template XContent

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* Cleanup parsing, javadocs, and demo output

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* Refactor to use field name constants, get tests working again

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* Separate WorkflowNode and ProcessNode functionality

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* Fix demos to align with template field names

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* Add workflow, node, and edge tests

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* Add Template tests

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* Refactor TemplateParser to WorkflowProcessSorter

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* Test exceptional cases

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* Finish up exceptional cases

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* Fix a template field name bug in demo

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* Rebase with #34

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* Rebase changes from #54

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* Integrate thread pool executor service

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* Fix flaky ProcessNodeTests by removing orTimeout

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* Rebase and refactor with #44

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* Fix demos and remove DataDemo

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* Use non-deprecated mapping method for CreateIndexStep

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* Eliminate casting and deprecation warnings on test classes

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* Remove unused/leftover demo class

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* Typo

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* Don't offer steps as an alternative to nodes

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* Move Workflow into package with all the other parsing classes

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* Move process sequencing classes into workflow package

Signed-off-by: Daniel Widdis <widdis@gmail.com>

* Add PipelineProcessor class and XContent parsing, rename package

Signed-off-by: Daniel Widdis <widdis@gmail.com>

---------

Signed-off-by: Daniel Widdis <widdis@gmail.com>
(cherry picked from commit 734f9c2)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
dbwiddis pushed a commit that referenced this pull request Sep 28, 2023
…Template Parsing (#60)

Add WorkflowStep Factory and implement XContent-based Template Parsing (#47)

* Add WorkflowStepFactory class



* Add XContent classes representing Template JSON



* Add parse methods for the Template XContent



* Cleanup parsing, javadocs, and demo output



* Refactor to use field name constants, get tests working again



* Separate WorkflowNode and ProcessNode functionality



* Fix demos to align with template field names



* Add workflow, node, and edge tests



* Add Template tests



* Refactor TemplateParser to WorkflowProcessSorter



* Test exceptional cases



* Finish up exceptional cases



* Fix a template field name bug in demo



* Rebase with #34



* Rebase changes from #54



* Integrate thread pool executor service



* Fix flaky ProcessNodeTests by removing orTimeout



* Rebase and refactor with #44



* Fix demos and remove DataDemo



* Use non-deprecated mapping method for CreateIndexStep



* Eliminate casting and deprecation warnings on test classes



* Remove unused/leftover demo class



* Typo



* Don't offer steps as an alternative to nodes



* Move Workflow into package with all the other parsing classes



* Move process sequencing classes into workflow package



* Add PipelineProcessor class and XContent parsing, rename package



---------


(cherry picked from commit 734f9c2)

Signed-off-by: Daniel Widdis <widdis@gmail.com>
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
3 participants