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

Changing resources created format #207

Closed
wants to merge 2 commits into from
Closed

Changing resources created format #207

wants to merge 2 commits into from

Conversation

amitgalitz
Copy link
Member

Description

Added updates to resources_created section for register model, create index, create ingest pipeline and create connector steps.

Changed the format of creating a resource to:

 {
                "_index": ".plugins-workflow-state",
                "_id": "RvEFF4wBKbeLfRazqAdh",
                "_score": 1.0,
                "_source": {
                    "workflow_id": "RvEFF4wBKbeLfRazqAdh",
                    "state": "PROVISIONING",
                    "provisioning_progress": "IN_PROGRESS",
                    "provision_start_time": 1701193321252,
                    "resources_created": [
                        {
                            "workflow_step_name": "create_connector",
                            "workflow_step_id": "workflow_step_1-amit-test",
                            "connector_id": "TfEGF4wBKbeLfRazCwcp"
                        },
                        {
                            "workflow_step_name": "register_remote_model",
                            "workflow_step_id": "workflow_step_2",
                            "model_id": "T_EGF4wBKbeLfRazCwdx"
                        }
                    ],
                    "provision_end_time": 1701193305145
                }
            }

Issues Resolved

resolves #173
resolves #136

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 Nov 28, 2023
dbwiddis
dbwiddis approved these changes Nov 29, 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. Note #215 will change the occurrences of data.get(0) to currentNodeInputs, whichever PR gets merged first will need to rebase for the other.

@dbwiddis
Copy link
Member

Should this PR be against the feature/agent_framework branch?

@amitgalitz
Copy link
Member Author

Should this PR be against the feature/agent_framework branch?

Tbh not sure if we wanted to merge to feature branch and then port to main and 2.x or merge to main then 2.x then feature branch? @owaiskazi19 what was your preferred route?

@dbwiddis
Copy link
Member

If the feature's mostly standalone it can go to main and add the feature backport tag.

The problem is this one edits nearly every workflow step and will create a conflict with #215. Much easier to resolve if both PRs are on the same branch and you can rebase. Otherwise we'll end up with the conflicts when we eventually try to rebase main and 2.x with the feature branch.

I guess my position is:

Whatever we do, don't merge this to main and #215 to feature branch (without backport) because merge conflicts on cherry-picks are ugly

@amitgalitz
adding retry on conflict
27400f1 
Signed-off-by: Amit Galitzky <[email protected]>
@amitgalitz
Copy link
Member Author

If the feature's mostly standalone it can go to main and add the feature backport tag.

The problem is this one edits nearly every workflow step and will create a conflict with #215. Much easier to resolve if both PRs are on the same branch and you can rebase. Otherwise we'll end up with the conflicts when we eventually try to rebase main and 2.x with the feature branch.

I guess my position is:

Whatever we do, don't merge this to main and #215 to feature branch (without backport) because merge conflicts on cherry-picks are ugly

If you don't think rebasing will be too much work on your end I don't mind making sure I backport this immediately to feature branch, I do want to get this merged soon though and not leave it hanging cause it solves some update issues and has the base line for how to change resources that can easily be copied to new steps.

@owaiskazi19
Copy link
Member

Should this PR be against the feature/agent_framework branch?

Tbh not sure if we wanted to merge to feature branch and then port to main and 2.x or merge to main then 2.x then feature branch? @owaiskazi19 what was your preferred route?

We can get this on main and then backport to 2.x and feature/agent_framework

@owaiskazi19
Copy link
Member

@amitgalitz Don't we need to add resources_created for ModelGroupStep or if any else workflow steps if we missed?

owaiskazi19
owaiskazi19 reviewed Nov 30, 2023
View reviewed changes
src/main/java/org/opensearch/flowframework/model/ResourceCreated.java
}
}
if (workflowStepName == null || resourceId == null) {
throw new IOException("A ResourceCreated object requires both a workflowStepName and resourceId.");
if (workflowStepName == null || workflowStepId == null || resourceId == null) {
Copy link
Member

Choose a reason for hiding this comment

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

This should be different conditions with their own logger.error message.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed on new PR: #231

src/main/java/org/opensearch/flowframework/workflow/WorkflowStepFactory.java
stepMap.put(CreateIndexStep.NAME, new CreateIndexStep(clusterService, client, flowFrameworkIndicesHandler));
stepMap.put(CreateIngestPipelineStep.NAME, new CreateIngestPipelineStep(client, flowFrameworkIndicesHandler));
stepMap.put(RegisterLocalModelStep.NAME, new RegisterLocalModelStep(mlClient, flowFrameworkIndicesHandler));
stepMap.put(RegisterRemoteModelStep.NAME, new RegisterRemoteModelStep(mlClient, flowFrameworkIndicesHandler));
stepMap.put(DeployModelStep.NAME, new DeployModelStep(mlClient));
stepMap.put(CreateConnectorStep.NAME, new CreateConnectorStep(mlClient, flowFrameworkIndicesHandler));
stepMap.put(ModelGroupStep.NAME, new ModelGroupStep(mlClient));
Copy link
Member

Choose a reason for hiding this comment

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

What about the other workflow steps? Aren't we storing the resources created for them?

Copy link
Member Author

Choose a reason for hiding this comment

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

Didn't add it to deployModel and modelGroupStep. Model group is a miss by me, adding this now, for DeployModelStep we have no resource being created.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed on #231

@amitgalitz
Copy link
Member Author

Closing in favor of merging to the feature branch first: #231

@amitgalitz amitgalitz closed this Nov 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@owaiskazi19 owaiskazi19 owaiskazi19 left review comments

@dbwiddis dbwiddis dbwiddis approved these changes

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

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

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

Assignees
No one assigned
Labels
backport feature/agent_framework backport 2.x backport PRs to 2.x branch
Projects
None yet
Milestone
No milestone
3 participants
@amitgalitz @dbwiddis @owaiskazi19