-
Notifications
You must be signed in to change notification settings - Fork 105
[9.1] Adds new 'none' and 'recursive' chunking strategies to Inference APIs #4841
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
Conversation
Following you can find the validation changes against the target branch for the APIs. No changes detected. You can validate these APIs yourself by using the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Not sure if we could use an @ext_doc_id
here? We do now have the ability to customize the link text so it doesn't just read External documentation.
Yes, I just learned that we can add external doc links for parameters too. It's on my list to update this PR along with other chunking reference updates. Thanks so much for your review! |
@@ -85,6 +85,7 @@ ccr-put-follow,https://www.elastic.co/docs/api/doc/elasticsearch/operation/opera | |||
ccr-resume-auto-follow-pattern,https://www.elastic.co/docs/api/doc/elasticsearch/operation/operation-ccr-resume-auto-follow-pattern,https://www.elastic.co/guide/en/elasticsearch/reference/8.18/ccr-resume-auto-follow-pattern.html, | |||
ccs-network-delays,https://www.elastic.co/docs/solutions/search/cross-cluster-search#ccs-network-delays,, | |||
ccs-privileges,https://www.elastic.co/docs/deploy-manage/remote-clusters/remote-clusters-cert#remote-clusters-privileges-ccs,, | |||
chunking-strategies,https://www.elastic.co/docs/explore-analyze/elastic-inference/inference-api#chunking-strategies, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to add a Description
field here to avoid the default link text?
Chunking strategies
probably works
Sorry if you've already planned to do this :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried this a few times, but it didn’t work, unfortunately.
I added a description to the table.csv file like this:
chunking-strategies,https://www.elastic.co/docs/explore-analyze/elastic-inference/inference-api#chunking-strategies,https://www.elastic.co/guide/en/kibana/8.18/inference-endpoints.html,Chunking strategies documentation
However, I didn’t see any results in the elasticsearch-openapi.json file, neither the previous URL nor the link description:
"strategy": {
"externalDocs": {
"url": "https://www.elastic.co/docs/explore-analyze/elastic-inference/inference-api#chunking-strategies"
},
"description": "The chunking strategy: `sentence`, `word`, `none` or `recursive`.\n\n * If `strategy` is set to `recursive`, you must also specify:\n\n- `max_chunk_size`\n- either `separators` or `separator_group`\n\nLearn more about different chunking strategies in the External documentation.",
"default": "sentence",
"type": "string"
}
To understand what’s going on, I tested the exact same case that @lcawl referenced here:
#4772 (comment)
t worked perfectly for me too. The OpenAPI file was updated exactly as Lisa described in her comment.
My only guess is that the example Lisa tested wasn't for a parameter-level externalDocs
link, but rather for a general API description, which might be treated differently by the tooling?
Could that explain the difference? Any ideas? Or am I missing something here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My only guess is that the example Lisa tested wasn't for a parameter-level externalDocs link, but rather for a general API description, which might be treated differently by the tooling?
Could that explain the difference? Any ideas? Or am I missing something here?
That might be it, I think you can only have one ext-id per API too IIRC, don't know if there's a previous one defined for this API.
There's as many exceptions as there are rules I fear 😠
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also know it that way that you can have only one ext-id
- but here, the ext-id
seems to work, what doesn’t work is the unique link text 🤔
@lcawl, do you happen to have any idea why this might not be working?
6e7ca65
to
732d600
Compare
Co-authored-by: Liam Thompson <[email protected]>
…e APIs (#4841) * Adds new chunking strategies * Adds external link * Adds a sentence to point to the link * Resolves merge conflict * Update specification/inference/_types/Services.ts Co-authored-by: Liam Thompson <[email protected]> --------- Co-authored-by: Liam Thompson <[email protected]> (cherry picked from commit ba5cf21)
This PR adds the new 'none' and 'recursive' chunking strategies to Inference APIs.
Related issue: https://github.com/elastic/developer-docs-team/issues/308