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.

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

[HLRC][ML] Add ML get model snapshots API #35487

Conversation

edsavage
Copy link
Contributor

Relates #29827

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@edsavage edsavage changed the title [HLRC] Add ML get model snapshots API [HLRC][ML] Add ML get model snapshots API Nov 13, 2018
@edsavage edsavage added the :ml Machine learning label Nov 13, 2018
@elasticmachine
Copy link
Collaborator

Pinging @elastic/ml-core

GetModelSnapshotsResponse response = execute(request, machineLearningClient::getModelSnapshots,
machineLearningClient::getModelSnapshotsAsync);

assertThat(response.count(), equalTo(3L));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm, I think you mean for this to be 1L, this goes for similar tests below.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review Ben - very thorough! The response count represents the total number of snapshots associated with the job hence is constant regardless of the for of a (valid) query. 3L is correct in these instances.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, as that is not how the other AbstractResponse<T> classes act. I suppose if the tests pass it is OK.

Usually, count() represents the number of items returned (basically, the same size as the list itself).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works like the total_hits of elasticsearch. So it will return as many docs match the query despite of how many fit in a page.

// end::get-model-snapshots-snapshot-id

// Set snapshot id to null as it is incompatible with other args
request.setSnapshotId(null); // <1>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// <1> is unnecessary.

include-tagged::{doc-tests-file}[{api}-response]
--------------------------------------------------
<1> The count of categories that were matched
<2> The categories retrieved
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

categories should probably be snapshots

@edsavage edsavage merged commit e7b7d52 into elastic:master Nov 14, 2018
edsavage added a commit that referenced this pull request Nov 14, 2018
@edsavage edsavage deleted the feature/add-ml-get-model-snapshot-info-to-hlrc branch November 14, 2018 14:43
@tomcallahan tomcallahan removed the :ml Machine learning label Dec 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants