search

opensearch-project / skills

Apache License 2.0
6 stars 29 forks source link

Proposal: move tools which doesn't depend on other plugins to ml-commons #294

Open ylwu-amzn opened 5 months ago

ylwu-amzn commented 5 months ago

Is your feature request related to a problem?

We created skills repo for decoupling ml-commons and other plugins. So ml-commons doesn't need to depends on other plugins, this can void circular dependency. But now we have SearchIndexTool and VisualizationTool in skills repo which doesn't need other plugins. This will add risk for the release process: skills repo needs to wait for other plugin dependencies ready first, then we can build skills to release version. That means skills repo will be the last one to merge to release version and we have little time to fix bugs/failed ITs etc. For example, in future we have 100 tools in skills repo, but only 10 tools depends on other plugins. But we still need to wait to the last minute to test these 90 tools because other plugins not ready. Propose move these tools which doesn't depend on other plugins to ml-commons. So we can build these tools to release version as early as possible, test and fix bugs rather than always wait until the last minute.

What solution would you like? Move tools which doesn't depend on other plugins to ml-commons, and follow this rule to reduce release risk.

What alternatives have you considered? No

Do you have any additional context? No

yuye-aws commented 5 months ago

PR to remove search index tool in skills repo: https://github.com/opensearch-project/skills/pull/295 PR (draft) to add search index tool in ml-commons repo: https://github.com/opensearch-project/ml-commons/pull/2356 Please merge these two PRs at the same time. The ml-commons PR should merge first.

yuye-aws commented 5 months ago

Do I also need to include the integration test into the ml-commons repo? The problem is a bit tricky because all tools do not have integration tests in ml-commons repo. Besides, SearchIndexToolIT extends the base class BaseAgentToolsIT in the skills repo. If we need to move the integration tests to the ml-commons repo, should BaseAgentToolsIT be moved together? I am concerned with the code duplication.

ylwu-amzn commented 5 months ago

@Hailong-am Can you share the PR link of ml-commons for visualization tool ?

Hailong-am commented 5 months ago

@Hailong-am Can you share the PR link of ml-commons for visualization tool ?

I will link it to here once it's ready for review.

xluo-aws commented 5 months ago

Yaliang, I don't feel ml-commons repo is the right place to host those tools. Understand this concern, but it seems most tools we built have dependencies on other plugins so move 2 of the 10+ tools out won't help much. With more tools on the way, we can decide whether it's worth to build another skills repo without plugin dependencies or if there is any better way to solve this problem.

ylwu-amzn commented 4 months ago

@xluo-aws, Sorry that missed your comment. It's the reality of today that most tools has dependencies on other plugins. This is more for future maintenance consideration. Actually this is not one way door. Placing tools in ml-commons or another repo doesn't impact user interface. We can have a discussion about pros/cons when we have enough tools which doesn't depend on other plugins.