Skip to content

MachineLearning.GetTrainedModelsStats always throws a deserialization exception #8271

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

Closed
svalbuena opened this issue Jul 26, 2024 · 7 comments
Labels
8.x Relates to a 8.x client version Category: Bug

Comments

@svalbuena
Copy link
Contributor

svalbuena commented Jul 26, 2024

Elastic.Clients.Elasticsearch version: 8.14.6

Elasticsearch version: 8.14.3

.NET runtime version: 8.0

Operating system version: Windows

Description of the problem including expected versus actual behavior:
I'm using the MachineLearning.GetTrainedModelsStatsAsync call to get the stats of a trained model, this request always results in System.Text.Json.JsonException: The JSON value could not be converted to System.Int32. Path: $.trained_model_stats[0].model_size_stats.required_native_memory_bytes. Invoking the route manually http://localhost:9200/_ml/trained_models/intfloat__e5-small-v2/_stats works.
Upon inspection of the Model class the response is deserialized into, I found the required_native_memory_bytes is defined as an int rather than ByteSize as the other fields of the same type:

namespace Elastic.Clients.Elasticsearch.MachineLearning;

public sealed partial class TrainedModelSizeStats
{
	/// <summary>
	/// <para>The size of the model in bytes.</para>
	/// </summary>
	[JsonInclude, JsonPropertyName("model_size_bytes")]
	public Elastic.Clients.Elasticsearch.ByteSize ModelSizeBytes { get; init; }

	/// <summary>
	/// <para>The amount of memory required to load the model in bytes.</para>
	/// </summary>
	[JsonInclude, JsonPropertyName("required_native_memory_bytes")]
	public int RequiredNativeMemoryBytes { get; init; }
}

The required_native_memory_bytes is much bigger than an int.

Steps to reproduce:

  1. Deploy an ML model
  2. Use the .NET elastic client to invoke the model like await client.MachineLearning.GetTrainedModelsStatsAsync(new Ids("intfloat__e5-small-v2"), cancellationToken);
  3. The code throws a deserialization exception

Expected behavior
The TrainedModelSizeStats class should type the RequiredNativeMemoryBytes field as Elastic.Clients.Elasticsearch.ByteSize, and the GetTrainedModelsStatsAsync call should succeed.

@svalbuena
Copy link
Contributor Author

svalbuena commented Jul 26, 2024

spec issue: elastic/elasticsearch-specification#2739

@flobernd
Copy link
Member

@svalbuena Thank you for creating the spec PR! The fix should already be included in the latest patch release which I did on Monday :-)

@svalbuena
Copy link
Contributor Author

Thanks @flobernd !
I had a look at how the code generation works, I really like doing a spec-first approach where both the server api (mostly the controllers and DTO classes) and all clients are generated from the spec. I'm not sure what happens in Elastic for the server-side, but for the clients I saw that the spec is generated from the TS client and the rest of clients are generated from the spec. I'm curious of why this is done like this instead of generating the TS client from the spec as well, do you know?

@flobernd
Copy link
Member

flobernd commented Aug 1, 2024

@svalbuena I think you did not get this completely right here. The spec is created by hand (*.ts files) and processed into a schema.json file by the spec compiler. There is another step, the compiler-ts, that produces *.ts output from the schema.json again. These files are not part of any official TS client, but only used internally to validate the structure against actual ES JSON requests/responses.

The TS bindings for the official JS client are as well generated from the spec 🙂

Regarding server-side: Currently, the controllers etc. are not generated from the spec, but I agree, that such approach could have a lot of benefits. For a new project, I would always chose this path as well, but it's often hard to change workflows for a software like ElasticSearch that exists for such long time.

@svalbuena
Copy link
Contributor Author

@flobernd ahh gotcha, so the .ts files that are just used to generate the spec. why not editing the spec .json directly? is it to have some type-safety/compilation checks that you would not have by working against the json directly?

@flobernd
Copy link
Member

flobernd commented Aug 1, 2024

@svalbuena Yes, mainly because it's way more convenient writing TS code than raw JSON 🙂 The compiled schema.json has more than 200.000 lines right now and besides that, we make use of organizing code by grouping multiple *.ts files in directories, etc. JSON as well does not easily allow to include comments, etc.

We as well explored other projects like e.g. Microsoft TypeSpec as a potential replacement for our custom TS based spec format, but currently this does not fulfill our requirements.

@svalbuena
Copy link
Contributor Author

@flobernd I've opened a follow-up pr to this, seems like there were two other type errors that are failing the request, this time I manually edited the .net client locally and verified there are no other errors left elastic/elasticsearch-specification#2763

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.x Relates to a 8.x client version Category: Bug
Projects
None yet
Development

No branches or pull requests

3 participants