-
Notifications
You must be signed in to change notification settings - Fork 83
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
Fix most ML APIs #3153
Fix most ML APIs #3153
Conversation
The failure is:
|
my bad, messed up an import, fixing it now |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment has been minimized.
This comment has been minimized.
@pquentin I just understood the source of my confusion: it seems like DiscoveryNode can be written in 2 ways, one is implemented in
with the id as an upper level json object, while the method
with the id inside the node object. Both ways seem to be accepted and present in the recordings. |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as outdated.
This comment was marked as outdated.
4b86700
to
0ea6aed
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
Following you can find the validation results for the APIs you have changed.
You can validate these APIs yourself by using the |
Co-authored-by: Quentin Pradet <[email protected]>
Co-authored-by: Quentin Pradet <[email protected]>
Co-authored-by: Quentin Pradet <[email protected]>
This comment was marked as outdated.
This comment was marked as outdated.
1 similar comment
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I've covered every API now! Thanks a lot for doing this.
specification/ml/_types/inference.ts
Outdated
/** | ||
* Should the tokenizer lower case the text | ||
* @server_default false | ||
*/ | ||
do_lower_case?: boolean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you mind creating a base class as done in the Elasticsearch server code to avoid missing fields shared by all tokenization configs like this in the future?
Co-authored-by: Quentin Pradet <[email protected]>
Co-authored-by: Quentin Pradet <[email protected]>
Co-authored-by: Quentin Pradet <[email protected]>
Co-authored-by: Quentin Pradet <[email protected]>
This comment was marked as outdated.
This comment was marked as outdated.
finished addressing the reviews, thank you so much @pquentin for double checking everything! |
This comment was marked as outdated.
This comment was marked as outdated.
1 similar comment
Following you can find the validation results for the APIs you have changed.
You can validate these APIs yourself by using the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! LGTM. Thanks for iterating. Let's merge this! 🎉
(And next time, let's try to split this into more PRs 😅)
* updated discovery node class * correct import * fixes around ml * more ml fixes, tentative discoverynode fix * more ml fixes * even more ml fixes * other ml fixes * modeling discovery node * correct discovery nodes optionals * correcting types * removed deprecated params * Update specification/ml/_types/TrainedModel.ts Co-authored-by: Quentin Pradet <[email protected]> * Update specification/ml/_types/TrainedModel.ts Co-authored-by: Quentin Pradet <[email protected]> * Update specification/ml/_types/TrainedModel.ts Co-authored-by: Quentin Pradet <[email protected]> * Update specification/ml/_types/TrainedModel.ts Co-authored-by: Quentin Pradet <[email protected]> * Update specification/ml/_types/TrainedModel.ts Co-authored-by: Quentin Pradet <[email protected]> * addressing reviews * Update specification/ml/_types/inference.ts Co-authored-by: Quentin Pradet <[email protected]> * Update specification/ml/_types/inference.ts Co-authored-by: Quentin Pradet <[email protected]> * Update specification/ml/_types/inference.ts Co-authored-by: Quentin Pradet <[email protected]> * Update specification/ml/post_data/MlPostJobDataResponse.ts Co-authored-by: Quentin Pradet <[email protected]> * more review addressing * merge fixes --------- Co-authored-by: Quentin Pradet <[email protected]> (cherry picked from commit ea3e9b6)
* updated discovery node class * correct import * fixes around ml * more ml fixes, tentative discoverynode fix * more ml fixes * even more ml fixes * other ml fixes * modeling discovery node * correct discovery nodes optionals * correcting types * removed deprecated params * Update specification/ml/_types/TrainedModel.ts Co-authored-by: Quentin Pradet <[email protected]> * Update specification/ml/_types/TrainedModel.ts Co-authored-by: Quentin Pradet <[email protected]> * Update specification/ml/_types/TrainedModel.ts Co-authored-by: Quentin Pradet <[email protected]> * Update specification/ml/_types/TrainedModel.ts Co-authored-by: Quentin Pradet <[email protected]> * Update specification/ml/_types/TrainedModel.ts Co-authored-by: Quentin Pradet <[email protected]> * addressing reviews * Update specification/ml/_types/inference.ts Co-authored-by: Quentin Pradet <[email protected]> * Update specification/ml/_types/inference.ts Co-authored-by: Quentin Pradet <[email protected]> * Update specification/ml/_types/inference.ts Co-authored-by: Quentin Pradet <[email protected]> * Update specification/ml/post_data/MlPostJobDataResponse.ts Co-authored-by: Quentin Pradet <[email protected]> * more review addressing * merge fixes --------- Co-authored-by: Quentin Pradet <[email protected]> (cherry picked from commit ea3e9b6)
* updated discovery node class * correct import * fixes around ml * more ml fixes, tentative discoverynode fix * more ml fixes * even more ml fixes * other ml fixes * modeling discovery node * correct discovery nodes optionals * correcting types * removed deprecated params * Update specification/ml/_types/TrainedModel.ts Co-authored-by: Quentin Pradet <[email protected]> * Update specification/ml/_types/TrainedModel.ts Co-authored-by: Quentin Pradet <[email protected]> * Update specification/ml/_types/TrainedModel.ts Co-authored-by: Quentin Pradet <[email protected]> * Update specification/ml/_types/TrainedModel.ts Co-authored-by: Quentin Pradet <[email protected]> * Update specification/ml/_types/TrainedModel.ts Co-authored-by: Quentin Pradet <[email protected]> * addressing reviews * Update specification/ml/_types/inference.ts Co-authored-by: Quentin Pradet <[email protected]> * Update specification/ml/_types/inference.ts Co-authored-by: Quentin Pradet <[email protected]> * Update specification/ml/_types/inference.ts Co-authored-by: Quentin Pradet <[email protected]> * Update specification/ml/post_data/MlPostJobDataResponse.ts Co-authored-by: Quentin Pradet <[email protected]> * more review addressing * merge fixes --------- Co-authored-by: Quentin Pradet <[email protected]> (cherry picked from commit ea3e9b6)
References for the fixes:
detectors
- DetectorUpdateaggs
as alias foraggregations
, both acceptedignore_throttled
(could not find server proof), body parameter job_iddetectors
uses different class, DetectorUpdateThe only thins missing server-side proof is
ignore_throttled
as query parameter inMlPutJobRequest
, I couldn't find it in parent classes either, so I'm trusting the validation on that one.