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

[ML] adding new PUT trained model vocabulary endpoint #77387

Merged
merged 6 commits into from
Sep 8, 2021

Conversation

benwtrent
Copy link
Member

@benwtrent benwtrent commented Sep 7, 2021

This commit removes the ability to set the vocabulary location in the model config.
This opts instead for sane defaults to be set and used. Wrapping this up in an
API.

The index is now always the internally managed .ml-inference-native index
and the document ID is always <model_id>_vocabulary

This API only works for pytorch/nlp type models.

@benwtrent benwtrent added >enhancement :ml Machine learning v8.0.0 Team:Clients Meta label for clients team labels Sep 7, 2021
@elasticmachine elasticmachine added the Team:ML Meta label for the ML team label Sep 7, 2021
@elasticmachine
Copy link
Collaborator

Pinging @elastic/clients-team (Team:Clients)

@elasticmachine
Copy link
Collaborator

Pinging @elastic/ml-core (Team:ML)

…ry.asciidoc

Co-authored-by: Lisa Cawley <lcawley@elastic.co>
Copy link
Member

@davidkyle davidkyle left a comment

Choose a reason for hiding this comment

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

LGTM

"parts":{
"model_id":{
"type":"string",
"description":"The ID of the trained model for this definition part"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"description":"The ID of the trained model for this definition part"
"description":"The ID of the trained model for this vocabulary"

if ((inferenceConfig instanceof NlpConfig) == false) {
listener.onFailure(
new ElasticsearchStatusException(
"cannot put vocabulary for model [{}] as it is not an nlp model",
Copy link
Member

Choose a reason for hiding this comment

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

nlp or NLP?

It's NLP in the docs but there is not a precedent in code

Copy link
Member Author

Choose a reason for hiding this comment

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

error messages are simply docs for failures :). So, I can switch it to be NLP.

parser.declareObject(
ConstructingObjectParser.optionalConstructorArg(),
(p, c) -> {
if (ignoreUnknownFields == false) {
Copy link
Member

Choose a reason for hiding this comment

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

++ the caller can't set this but the code creates the config so it is visible

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>enhancement :ml Machine learning Team:Clients Meta label for clients team Team:ML Meta label for the ML team v8.0.0-alpha2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants