-
Notifications
You must be signed in to change notification settings - Fork 73
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 Get trained models statistics API types #2763
Conversation
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.
Hi @svalbuena, thanks again for your contribution 🙂 I left some comments.
@@ -120,7 +120,7 @@ export class TrainedModelInferenceStats { | |||
/** The number of inference calls where all the training features for the model were missing. */ | |||
missing_all_fields_count: integer | |||
/** The time when the statistics were last updated. */ | |||
timestamp: DateTime | |||
timestamp: long |
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.
I think DateTime
might have been correct here, as it's defined as:
/**
* A date and time, either as a string whose format can depend on the context (defaulting to ISO 8601), or a
* number of milliseconds since the Epoch. Elasticsearch accepts both as input, but will generally output a string
* representation.
*/
export type DateTime = string | EpochTime<UnitMillis>
We have to check the server-code in order to verify if it should be DateTime
or just EpochTime<UnitMillis>
.
In any way:
The .NET client correctly uses DateTimeOffset
as the CLR type, but fails to deserialize it from the epoch-time representation. This is something that must be fixed on the client itself and not in the spec.
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.
interesting, in this case it seems EpochTime<UnitMillis>
is the right type. I've checked other classes that use the same kind of value like start_time_in_millis
of AsyncSearchResponseBase and it uses EpochTime for the one that is long. (I don't find any example of the start_time
field in the tests I've done nor in https://www.elastic.co/guide/en/elasticsearch/reference/current/async-search.html).
I'll set the field to be EpochTime for now until you find the right answer or fix for the .net client.
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.
Yes, this seems right. I think this should be the corresponding server code:
https://github.com/elastic/elasticsearch/blob/02c494963a59610a0be07c7d54337017a1e5beaf/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/inference/TrainedModelStatsService.java#L282C58-L282C106
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.
so EpochTime<UnitMillis>
looks good? anything else to address in this PR?
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.
LGTM from my side, thank you!
Let's wait for a second pair of eyes to double-check it.
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.
LGTM as well!
@svalbuena Since validation only runs from branches, I opened #2770 with your changes, which allowed validation to pass. Your pull request did not affect the reported failures, since apparently tests did not surface this specific issue. (ML request issues are tracked in #2621.) Thanks, merging! |
* Fix Get trained models statistics API types * Push schema changes * Use EpochTime<UnitMillis> (cherry picked from commit d1a0565)
* Fix Get trained models statistics API types * Push schema changes * Use EpochTime<UnitMillis> (cherry picked from commit d1a0565)
Fixes #2762, I've tested these changes by downloading the .net client project, doing the equivalent changes done in this spec PR, and using it in the project I'm working on to invoke the
GET http://localhost:9200/_ml/trained_models/intfloat__e5-small-v2/_stats
endpoint, I now get a successful response.Sample response of
GET http://localhost:9200/_ml/trained_models/intfloat__e5-small-v2/_stats
Closes #2770