Skip to content

[DOC] MLCommons Tools: Add attributes to toolspec #878

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

Merged
merged 5 commits into from
May 9, 2025

Conversation

pyek-bot
Copy link
Contributor

Description

Adds attributes field for tools in ml-commons toolspec

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.

@pyek-bot
Copy link
Contributor Author

Attributes is actually a map that can accept any key/value pairs and these are provided to the model for added context. Primarily use input_schema and strict to share context about the expected schema to the model. Any guidelines on how to define this in the spec?

@dblock
Copy link
Member

dblock commented Apr 23, 2025

Attributes is actually a map that can accept any key/value pairs and these are provided to the model for added context. Primarily use input_schema and strict to share context about the expected schema to the model. Any guidelines on how to define this in the spec?

An open structure with arbitrary key/value pairs is additionalProperties: true type of thing.

Start by adding/identifying a test into tests/default that you can run and that returns these fields.

Copy link
Contributor

github-actions bot commented Apr 23, 2025

Changes Analysis

Commit SHA: 20ce07c
Comparing To SHA: c042c5f

API Changes

Summary

├─┬Paths
│ └─┬/_plugins/_ml/tools/{tool_name}
│   └─┬GET
│     └─┬Parameters
│       └─┬Schema
│         └──[➕] enum (24204:13)
└─┬Components
  ├──[➕] schemas (59122:7)
  ├─┬ml._common___ToolItems
  │ ├──[➕] properties (59142:9)
  │ ├──[➕] properties (59136:9)
  │ ├─┬parameters
  │ │ └──[🔀] $ref (59141:11)❌ 
  │ └─┬include_output_in_agent_response
  │   └──[🔀] $ref (59122:13)❌ 
  └─┬ml._common___Tool
    └─┬attributes
      └──[🔀] $ref (59122:13)❌ 

Document Element Total Changes Breaking Changes
components 6 3
paths 1 0
  • BREAKING Changes: 3 out of 7
  • Modifications: 3
  • Additions: 4
  • Breaking Modifications: 3

Report

The full API changes report is available at: https://github.com/opensearch-project/opensearch-api-specification/actions/runs/14917927391/artifacts/3090308855

API Coverage

Before After Δ
Covered (%) 663 (64.94 %) 663 (64.94 %) 0 (0 %)
Uncovered (%) 358 (35.06 %) 358 (35.06 %) 0 (0 %)
Unknown 53 53 0

Copy link
Contributor

github-actions bot commented Apr 23, 2025

Spec Test Coverage Analysis

Total Tested
603 601 (99.67 %)

Signed-off-by: Pavan Yekbote <[email protected]>
@pyek-bot
Copy link
Contributor Author

pyek-bot commented May 8, 2025

Attributes is actually a map that can accept any key/value pairs and these are provided to the model for added context. Primarily use input_schema and strict to share context about the expected schema to the model. Any guidelines on how to define this in the spec?

An open structure with arbitrary key/value pairs is additionalProperties: true type of thing.

Start by adding/identifying a test into tests/default that you can run and that returns these fields.

Thanks @dblock! I was able to validate the schema by running tests/plugins/ml/ml/agents.yaml & tests/plugins/ml/ml/tools.yaml tests.

@pyek-bot
Copy link
Contributor Author

pyek-bot commented May 8, 2025

Do you mind reviewing the PR please? Thanks!

Copy link
Collaborator

@Xtansia Xtansia left a comment

Choose a reason for hiding this comment

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

Changes generally look good, have added some comments for how to get the CI failures to green. Please also add an entry to the CHANGELOG.md

@pyek-bot
Copy link
Contributor Author

pyek-bot commented May 8, 2025

Thanks for the review @Xtansia ! I have addressed the comments, please take a look. Will track the CI.

@dblock
Copy link
Member

dblock commented May 8, 2025

This PR should include tests that return these new structures.

@pyek-bot
Copy link
Contributor Author

pyek-bot commented May 8, 2025

This PR should include tests that return these new structures.

/tests/plugins/ml/ml/tools.yaml (specifically /_plugins/_ml/tools) and /tests/plugins/ml/ml/agents.yaml (specifically /_plugins/_ml/agents/_register) already cover these scenarios.

When I modify the attributes spec, these tend to fail.

Copy link
Collaborator

@Xtansia Xtansia left a comment

Choose a reason for hiding this comment

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

Thanks @pyek-bot!

Can confirm it's covered by existing tests, as I had to add a version of the attributes property to get tests passing in #889

@Xtansia Xtansia merged commit e685fe5 into opensearch-project:main May 9, 2025
32 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants