Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix Get trained models statistics API types #2763

Merged
merged 3 commits into from
Aug 6, 2024
Merged

Fix Get trained models statistics API types #2763

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
7303fc3
Fix Get trained models statistics API types
svalbuena Aug 2, 2024
9563caa
Push schema changes
svalbuena Aug 2, 2024
cabc6a3
Use EpochTime<UnitMillis>
svalbuena Aug 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions output/schema/schema.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions output/typescript/types.ts
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions specification/ml/_types/TrainedModel.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ export class TrainedModelDeploymentStats {
* The deployment stats for each node that currently has the model allocated.
* In serverless, stats are reported for a single unnamed virtual node.
*/
nodes: TrainedModelDeploymentNodesStats
nodes: TrainedModelDeploymentNodesStats[]
flobernd marked this conversation as resolved.
Show resolved Hide resolved
/** The number of allocations requested. */
number_of_allocations: integer
/** The number of inference requests that can be queued before new requests are rejected. */
Expand Down Expand Up @@ -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
Copy link
Member

@flobernd flobernd Aug 2, 2024 •
edited
Loading

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.

Copy link
Contributor Author

@svalbuena svalbuena Aug 2, 2024 •
edited
Loading

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_timefield 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.

Copy link
Member

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

Copy link
Contributor Author

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?

}

export class TrainedModelSizeStats {
Expand Down
Loading