feat: parse connector id from tool parameters map

yuye-aws commented 2 months ago

Description

Parse connector id from previous_node_inputs into the parameters map

{
  "id": "create_connector_tool",
  "type": "create_tool",
  "previous_node_inputs": {
    "create_connector": "connector_id"
  },
  "user_inputs": {
    "parameters": {},
    "name": "ConnectorTool",
    "type": "ConnectorTool"
  }
}

Check List

[x] New functionality includes testing.
[x] New functionality has been documented.
~[ ] API changes companion pull request created.~
[x] Commits are signed per the DCO using --signoff.
~[ ] Public documentation issue/PR created.~ Already documented

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.

yuye-aws commented 2 months ago

Not sure what are the expected UTs and ITs in the PR.

codecov[bot] commented 2 months ago

Codecov Report

Attention: Patch coverage is 84.61538% with 2 lines in your changes missing coverage. Please review.

Project coverage is 77.55%. Comparing base (60458a6) to head (52d68df). Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...rg/opensearch/flowframework/workflow/ToolStep.java	84.61%	0 Missing and 2 partials :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #846 +/- ## ============================================ + Coverage 77.38% 77.55% +0.17% - Complexity 963 966 +3 ============================================ Files 97 97 Lines 4536 4531 -5 Branches 423 422 -1 ============================================ + Hits 3510 3514 +4 + Misses 845 835 -10 - Partials 181 182 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

dbwiddis commented 2 months ago

Thanks for the PR! I see you copied what we already did for model ID which works great. I wonder if there's a more efficient way to do it but we can refactor later. :)

Not sure what are the expected UTs and ITs in the PR.

For UT: Looks like our existing unit tests just confirm that we generate a tool step. We could update those to include things in the map and verify that they exist in the MLToolSpec object in the output... which we probably should have already done. ☹️

But this is one of those things probably better tested in an integ test (or API Spec test). Can you add a new test method to FlowFrameworkRestApiIT that tests a Tool (or tools) with inputs from both a connector and a model? You'd basically:

create a template in src/test/resources/template with appropriate JSON (probably need to create an agent with those tool(s))
copy testSearchWorkflows() and change it to execute your template
provision the template
call an API to verify the output (in this case you'd probably get the workflow ID after provisioning, use that to query the workflow state to get the resource ID of the agent, then use the agent API to query the agent and make sure it has the connector ID in the JSON

From the standpoint of this PR, doing a manual test and showing the output here is probably sufficient as we have a plan to eventually test all our templates via api spec and that will eventually cover this... but any help you can provide improving out UT/IT would be appreciated.

yuye-aws commented 2 months ago

For UT: Looks like our existing unit tests just confirm that we generate a tool step. We could update those to include things in the map and verify that they exist in the MLToolSpec object in the output... which we probably should have already done. ☹️

Updated

yuye-aws commented 2 months ago

Thanks for the PR! I see you copied what we already did for model ID which works great. I wonder if there's a more efficient way to do it but we can refactor later. :)

It should be a good idea to iterate over a set of [CONNECTOR_ID, MODEL_ID, AGENT_ID]. Hoping that the code can get merged before 2.17, we can refactor the code later in 2.18.

yuye-aws commented 2 months ago

call an API to verify the output (in this case you'd probably get the workflow ID after provisioning, use that to query the workflow state to get the resource ID of the agent, then use the agent API to query the agent and make sure it has the connector ID in the JSON

Not sure how to accomplish this step. It takes me some while to get the agent id.

dbwiddis commented 2 months ago

Not sure how to accomplish this step. It takes me some while to get the agent id.

This one's probably better handled by an api spec test CC @junweid

yuye-aws commented 2 months ago

Not sure how to accomplish this step. It takes me some while to get the agent id.

This one's probably better handled by an api spec test CC @JunweiD

Just figured out. Pushing the code now.

yuye-aws commented 2 months ago

@dbwiddis Can you help review the IT code? Thanks!

yuye-aws commented 2 months ago

Not sure whether the current CI error is attributed to this PR. Can someone take a look?

yuye-aws commented 2 months ago

Can you also write a secured integ test for this in FlowFrameworkSecureRestApiIT?

I can add an IT. Before that, can you tell me its difference between FlowFrameworkRestApiIT? I do not wish to implement duplicated ITs.

owaiskazi19 commented 2 months ago

Can you also write a secured integ test for this in FlowFrameworkSecureRestApiIT?

I can add an IT. Before that, can you tell me its difference between FlowFrameworkRestApiIT? I do not wish to implement duplicated

FlowFrameworkSecureRestApiIT is for security enabled integ tests and FlowFrameworkRestApiIT is for security disabled integ tests.

yuye-aws commented 2 months ago

Can you also write a secured integ test for this in FlowFrameworkSecureRestApiIT?

I can add an IT. Before that, can you tell me its difference between FlowFrameworkRestApiIT? I do not wish to implement duplicated

FlowFrameworkSecureRestApiIT is for security enabled integ tests and FlowFrameworkRestApiIT is for security disabled integ tests.

FlowFrameworkSecureRestApiIT is mainly about access control. Not sure if you expect to see a similar method like testCreateAndProvisionConnectorToolAgentFrameworkWorkflow.

owaiskazi19 commented 2 months ago

FlowFrameworkSecureRestApiIT is mainly about access control. Not sure if you expect to see a similar method like testCreateAndProvisionConnectorToolAgentFrameworkWorkflow.

So since you are adding a new template file createconnector-createconnectortool-createflowagent.json. We can add a new secured integ tests for create and provision with access control.

dbwiddis commented 2 months ago

FlowFrameworkSecureRestApiIT is mainly about access control. Not sure if you expect to see a similar method like testCreateAndProvisionConnectorToolAgentFrameworkWorkflow.

Does the ML Commons client connector API behave differently with/without the security plugin installed? If so, it's appropriate.

yuye-aws commented 2 months ago

Does the ML Commons client connector API behave differently with/without the security plugin installed? If so, it's appropriate.

No. I don't think so. Connector is accessing remote services. Security plugin does not matter.

dbwiddis commented 2 months ago

No. I don't think so. Connector is accessing remote services. Security plugin does not matter.

Then I don't think a security integ test is needed. @owaiskazi19 do you feel differently?

owaiskazi19 commented 2 months ago

No. I don't think so. Connector is accessing remote services. Security plugin does not matter.

Then I don't think a security integ test is needed. @owaiskazi19 do you feel differently?

Agreed. @yuye-aws can you resolve the other comments then and we can get this in?

yuye-aws commented 2 months ago

@dbwiddis @owaiskazi19 This PR has been updated according to your comments. Can you help review again?

yuye-aws commented 2 months ago

There seems to be a flakey test. Need to rerun this workflow.

dbwiddis commented 2 months ago

There seems to be a flakey test. Need to rerun this workflow.

Yeah, macos multinode runs into memory issues sometimes (3 nodes in docker + deploying a local model), will rerun, but sometimes we just merge if that's the only failure.

opensearch-project / flow-framework