Block commas in model description #1692
Conversation
Signed-off-by: Ryan Bogan <[email protected]>
Signed-off-by: Ryan Bogan <[email protected]>
ryanbogan
requested review from
heemin32,
navneet1v,
VijayanB,
vamshin,
jmazanec15,
naveentatikonda,
junqiu-lei,
martin-gaievski and
luyuncheng
as code owners
|
Rolling upgrade failures unrelated: #1691 |
| @@ -123,6 +126,9 @@ public ModelMetadata( | |||
| this.state = new AtomicReference<>(Objects.requireNonNull(modelState, "modelState must not be null")); | |||
| this.timestamp = Objects.requireNonNull(timestamp, "timestamp must not be null"); | |||
| this.description = Objects.requireNonNull(description, "description must not be null"); | |||
| if (this.description.contains(",")) { | |||
| throw new IllegalArgumentException("Model description cannot contain any commas: ','"); | |||
There was a problem hiding this comment.
nit: may be "model" instead of "Model"? i see every other exception message doesn't start with caps.
There was a problem hiding this comment.
All of the null check exceptions are lower case, but the other IllegalArgumentException earlier in the method that validates the max dimensions starts with a cap:
https://github.com/opensearch-project/k-NN/pull/1692/files#diff-b1d2b535ac05d02aee28ed7164936b0f35b893746c09eb24e705c9e1cfd3fed2R118
|
Can we add the check in the rest handler as well as ModelMetadata? Im just thinking we should fail fast on this when possible (i.e. before any transport requests are sent or state is serialized) |
Signed-off-by: Ryan Bogan <[email protected]>
Signed-off-by: Ryan Bogan <[email protected]>
src/test/java/org/opensearch/knn/plugin/action/RestTrainModelHandlerIT.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Ryan Bogan <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1692 +/- ##
============================================
+ Coverage 84.92% 84.98% +0.06%
- Complexity 1375 1459 +84
============================================
Files 172 176 +4
Lines 5605 5854 +249
Branches 553 597 +44
============================================
+ Hits 4760 4975 +215
- Misses 612 628 +16
- Partials 233 251 +18 ☔ View full report in Codecov by Sentry. |
|
The backport to To backport manually, run these commands in your terminal: # Fetch latest updates from GitHub
git fetch
# Create a new working tree
git worktree add .worktrees/backport-2.x 2.x
# Navigate to the new working tree
cd .worktrees/backport-2.x
# Create a new branch
git switch --create backport/backport-1692-to-2.x
# Cherry-pick the merged commit of this pull request and resolve the conflicts
git cherry-pick -x --mainline 1 011775a208002547c9080fc9423399ea500faa52
# Push it to GitHub
git push --set-upstream origin backport/backport-1692-to-2.x
# Go back to the original working tree
cd ../..
# Delete the working tree
git worktree remove .worktrees/backport-2.xThen, create a pull request where the |
* Block commas in model description Signed-off-by: Ryan Bogan <[email protected]> * Add changelog entry Signed-off-by: Ryan Bogan <[email protected]> * Add check in rest handler Signed-off-by: Ryan Bogan <[email protected]> * Extract if statement into ModelUtil method Signed-off-by: Ryan Bogan <[email protected]> * Remove ingestion from integ test Signed-off-by: Ryan Bogan <[email protected]> --------- Signed-off-by: Ryan Bogan <[email protected]>
* Block commas in model description Signed-off-by: Ryan Bogan <[email protected]> * Add changelog entry Signed-off-by: Ryan Bogan <[email protected]> * Add check in rest handler Signed-off-by: Ryan Bogan <[email protected]> * Extract if statement into ModelUtil method Signed-off-by: Ryan Bogan <[email protected]> * Remove ingestion from integ test Signed-off-by: Ryan Bogan <[email protected]> --------- Signed-off-by: Ryan Bogan <[email protected]>
Description
When commas are present in the model description, the model metadata is not parsed correctly, resulting in an exception being thrown when ingesting data to or searching on a model-based index. This PR throws an IllegalArgumentException if the user attempts to create a model with a comma in the model description.
Issues Resolved
#1337
#1479
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.