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

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

Open
ylwu-amzn opened this issue Apr 24, 2024 · 6 comments
Open
Labels
enhancement New feature or request

Comments

@ylwu-amzn
Copy link
Collaborator

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

@ylwu-amzn ylwu-amzn added enhancement New feature or request untriaged and removed untriaged labels Apr 24, 2024
@yuye-aws
Copy link
Member

yuye-aws commented Apr 24, 2024

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

@yuye-aws
Copy link
Member

yuye-aws commented Apr 24, 2024

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
Copy link
Collaborator Author

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

@Hailong-am
Copy link
Contributor

@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
Copy link
Member

xluo-aws commented May 8, 2024

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
Copy link
Collaborator Author

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

4 participants