[ML] Flag updates from Inference #131725
[ML] Flag updates from Inference #131725
Conversation
elasticsearchmachine
added
the
serverless-linked
|
Pinging @elastic/ml-core (Team:ML) |
prwhelan
changed the title
|
|
||
| public void setFromInference(boolean fromInference) { | ||
| this.fromInference = fromInference; | ||
| this.isInternal = fromInference; |
There was a problem hiding this comment.
It looks confusing the setFromInference also sets isInternal.
...src/main/java/org/elasticsearch/xpack/core/ml/action/UpdateTrainedModelDeploymentAction.java
Outdated
Show resolved
Hide resolved
| import org.elasticsearch.xpack.core.ml.utils.ExceptionsHelper; | ||
|
|
||
| import java.io.IOException; | ||
| import java.util.Objects; |
There was a problem hiding this comment.
I'm missing a bit of context: why do we need to distinguish between these cases?
There was a problem hiding this comment.
Is there a corresponding Serverless PR?
There was a problem hiding this comment.
Yeah, let me ping you with the internal documentation
There was a problem hiding this comment.
I'm missing a bit of context: why do we need to distinguish between these cases?
We need to allow updates to num_allocations in serverless that originate from the AdaptiveAllocationsScalerService (ADAPTIVE_ALLOCATIONS), but we want to disallow updates from users (API and INFERENCE). The only alternative I thought of was refactoring AdaptiveAllocationsScalerService to update directly rather than through the API, but that felt more intrusive.
| // we changed over from a boolean to an enum | ||
| // when it was a boolean, true came from adaptive allocations and false came from the rest api | ||
| // treat "inference" as if it came from the api | ||
| out.writeBoolean(isInternal()); |
There was a problem hiding this comment.
Do we need to determine if source == Source.ADAPTIVE_ALLOCATIONS here? Since this will return true for Source.INFERENCE as well?
There was a problem hiding this comment.
Previously, we set the boolean to true if the source was either from the inference update api or the adaptive allocations autoscaler. out.writeBoolean(isInternal()) preserves this logic (i think). It means the stream reader will think an inference api call is an adaptive allocations api call, but that only affects serverless which is only mixed cluster during a rolling update.
|
|
||
| public boolean isInternal() { | ||
| return isInternal; | ||
| return source == Source.INFERENCE || source == Source.ADAPTIVE_ALLOCATIONS; |
There was a problem hiding this comment.
Can you confirm that we do want Source.INFERENCE here for all the usage of isInternal() below?
There was a problem hiding this comment.
Confirmed! Yeah inference update code previously set isInternal to true (back when the boolean existed)
|
|
||
| public static final ParseField TIMEOUT = new ParseField("timeout"); | ||
|
|
||
| private static final TransportVersion INFERENCE_UPDATE_ML = TransportVersion.fromName("inference_update_ml"); |
There was a problem hiding this comment.
I think this name INFERENCE_UPDATE_ML isn't particularly clear.
What about something like UPDATE_TRAINED_MODEL_DEPLOYMENT_REQUEST_SOURCE or so? 🤷
| import static org.elasticsearch.xpack.core.ml.action.StartTrainedModelDeploymentAction.Request.NUMBER_OF_ALLOCATIONS; | ||
|
|
||
| public class UpdateTrainedModelDeploymentAction extends ActionType<CreateTrainedModelAssignmentAction.Response> { | ||
| public enum Source { |
There was a problem hiding this comment.
Isn't it cleaner to move this into Request (so UpdateTrainedModelDeploymentAction.Request.Source)?
| public enum Source { | ||
| API, | ||
| ADAPTIVE_ALLOCATIONS, | ||
| INFERENCE |
There was a problem hiding this comment.
nit INFERENCE is pretty vague, from the usage it appears to mean the request comes from the Inference API
| INFERENCE | |
| INFERENCE_API |
5 participants
Flag updates from Inference so Serverless can detect them.
Swap tests to set adaptive allocations rather than num allocations to pass in serverless.