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] Downloaded elser models should be easily sharable between spaces #169771

Closed
jgowdyelastic opened this issue Oct 25, 2023 · 7 comments · Fixed by #169939
Closed

[ML] Downloaded elser models should be easily sharable between spaces #169771

jgowdyelastic opened this issue Oct 25, 2023 · 7 comments · Fixed by #169939
Assignees

Comments

@jgowdyelastic
Copy link
Member

jgowdyelastic commented Oct 25, 2023

If the user has downloaded an elser model and then navigates to a different space, then the elser model appears to be not downloaded and so they can click the download button again. For a deployed model, this then throws an error as the model already exists in elasticsearch.

A work around is to share the model to the different space via the stack management page. But this is not obvious.

We should be warn the user that the model already exists and give them the option to instantly assign it to the current space.
If they do not have permission to assign spaces, we should inform them that they need an admin to help.

Alternatively, we could just assign the space automatically as part of a download endpoint, if we see that it has already been downloaded and the call to download is coming from a different space which the elser trained model doesn't exist in.
Currently there is no download endpoint, instead the PUT trained model config endpoint is called from the client side. If we decided we need to run additional space checks or assign the model to * after creation, we will probably need to wrap is all inside a dedicated download endpoint.

@jgowdyelastic jgowdyelastic self-assigned this Oct 25, 2023
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui (:ml)

@mdefazio
Copy link
Contributor

Out of curiosity, would this make sense to just be assigned / available to all spaces by default?

If a space does not have ML access, then they wouldn't see this trained model view anyway, correct? Why would I not want a model to be available in a space? (Perhaps a silly question...)

Would model permissions make sense as an Elasticsearch role privilege similar to how we handle indices?
Screenshot--2023-10-25--dfasdfa - Roles - Elastic

@jgowdyelastic
Copy link
Member Author

jgowdyelastic commented Oct 25, 2023

@mdefazio

Out of curiosity, would this make sense to just be assigned / available to all spaces by default?

This is a very good point. we could do this by default, although it might be tricky to give it an initial space of *. we might have to reassign the space automatically once it is ready.

If we do assign * automatically, we should be aware that this situation could come up again if the user manually reassigns the elser model's spaces.
As in,

  • elser is downloaded, we automatically assign it to *
  • the user manually changes it to something else, e.g. default
  • the user goes to space2 and tries to download elser
  • we're back to this problem where it's already downloaded, but in a different space.

Or,

  • elser is downloaded using the es endpoint
  • the user uses ML's sync feature in kibana which automatically assigns saved objects to the current space.

granted these scenarios are unlikely.
if we're happy that the current work around with manually assigning the spaces in management is ok, I think assigning to * by default makes sense.

We could also do both ideas.
by default assign to *.
when downloading through kibana, reassign the space if it's not available in the current space.

@mdefazio
Copy link
Contributor

@jgowdyelastic Thanks for this explanation and context—very helpful.

@jgowdyelastic
Copy link
Member Author

jgowdyelastic commented Oct 26, 2023

we could just assign the space automatically as part of a download endpoint

This is not technically possible. I had forgotten that there is a restriction on space assignment where you cannot assign a new space to a saved object from a space where the saved object isn't already a member.
As in, given a saved object in space1 only. you cannot add or remove spaces to it when in space2

We have requested a removal of this restriction before, and it looks like some work has taken place, but it is quite complicated and looks to have been parked. #131254

The only option left I can see is to automatically assign elser models to * when creating them.
We will also have to update the sync functionality to ensure elser models are assigned to * when syncing.

@droberts195
Copy link
Contributor

automatically assign elser models to * when creating them

This could be generalised as automatically assigning models to * if the model ID begins with .. Model IDs beginning with . are reserved for our tightly controlled internal models.

@jgowdyelastic
Copy link
Member Author

This could be generalised as automatically assigning models to * if the model ID begins with .. Model IDs beginning with . are reserved for our tightly controlled internal models.

Yes, that actually makes it easier as we won't have to retrieve the IDs of known elser models.

jgowdyelastic added a commit that referenced this issue Nov 6, 2023
Fixes #169771

Adds a new endpoint
`/internal/ml/trained_models/install_elastic_trained_model/:modelId`
which wraps the `putTrainedModel` call to start the download of the
elser model. It then reassigns the saved object's space to be `*`.

Also updates the saved object sync call to ensure any internal models
(ones which start with `.`) are assigned to the `*` space, if they've
needed syncing.

It is still possible for a user to reassign the spaces for an elser
model and get themselves into the situation covered described in
#169771.
In this situation, I believe the best we can do is suggest the user
adjusts the spaces via the stack management page.

At the moment a `Model already exists` error is displayed in a toast. In
a follow up PR we could catch this and show more information to direct
the user to the stack management page.

---------

Co-authored-by: Dima Arnautov <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants