Allow . chars in model_id #108426 #136783
Conversation
elasticsearchmachine
added
needs:triage
prwhelan
added
:ml
|
Pinging @elastic/ml-core (Team:ML) |
There was a problem hiding this comment.
The issue #108426 describes a problem with creating an inference endpoint with . characters at the start of the inference ID. This PR is making changes to allowed characters in a trained model alias, which is unrelated.
The regex that is used to check inference IDs is MlStrings.VALID_ID_CHAR_PATTERN, and it's tested in MlStringsTests and PutInferenceModelActionTests
...test/java/org/elasticsearch/xpack/core/ml/action/PutTrainedModelAliasActionRequestTests.java
Outdated
Show resolved
Hide resolved
...in/core/src/main/java/org/elasticsearch/xpack/core/ml/action/PutTrainedModelAliasAction.java
Outdated
Show resolved
Hide resolved
This reverts commit 5163448.
|
@DonalEvans I fixed it according to your comments. Can you take a look? |
| */ | ||
| private static final Pattern VALID_ID_CHAR_PATTERN = Pattern.compile("[a-z0-9](?:[a-z0-9_\\-\\.]*[a-z0-9])?"); | ||
| private static final Pattern VALID_ID_CHAR_PATTERN = | ||
| Pattern.compile("^[a-z0-9.](?:[a-z0-9_\\-.]*[a-z0-9.])?$"); |
There was a problem hiding this comment.
I'm not sure that we want to allow IDs to end with ., the issue motivating this change only mentions allowing IDs to start with .
There was a problem hiding this comment.
Ok changed to allow only to start with .
There was a problem hiding this comment.
Thanks for putting up the PR!
I'm not sure we want to allow inference endpoints that start with .. With the coming of dynamic inference endpoints from EIS we'll need a way to avoid users accidentally naming something the same thing that EIS will. Currently the only way to guarantee that is to prevent users from using inference endpoints that start with a ..
I'll chat with the team about this and let you know what we decide.
5 participants
Aligned regex to allow. characters at the beginning and end of the model_id as mentioned in the issue.
Also added a test to cover such a case and an error message to match the current state
Fix #108426