Skip to content

[ML] fixing testTwoJobsWithSameRandomizeSeedUseSameTrainingSet tests#62976

Merged
benwtrent merged 1 commit intoelastic:masterfrom
benwtrent:test/ml-dfa-random-seed-test-fix
Sep 29, 2020
Merged

[ML] fixing testTwoJobsWithSameRandomizeSeedUseSameTrainingSet tests#62976
benwtrent merged 1 commit intoelastic:masterfrom
benwtrent:test/ml-dfa-random-seed-test-fix

Conversation

@benwtrent
Copy link
Copy Markdown
Member

@benwtrent benwtrent commented Sep 28, 2020 •
edited
Loading

This fixes the two test failures.

The shard failure seems to be due to the .ml-stats index being in the middle of being created.

See:

elasticsearch/x-pack/plugin/ml/src/main/java/org/elasticsearch/xpack/ml/action/TransportStartDataFrameAnalyticsAction.java

Lines 391 to 405 in d1e963e

private void getProgress(DataFrameAnalyticsConfig config, ActionListener<List<PhaseProgress>> listener) {
GetDataFrameAnalyticsStatsAction.Request getStatsRequest = new GetDataFrameAnalyticsStatsAction.Request(config.getId());
executeAsyncWithOrigin(client, ML_ORIGIN, GetDataFrameAnalyticsStatsAction.INSTANCE, getStatsRequest, ActionListener.wrap(
statsResponse -> {
List<GetDataFrameAnalyticsStatsAction.Response.Stats> stats = statsResponse.getResponse().results();
if (stats.isEmpty()) {
// The job has been deleted in between
listener.onFailure(ExceptionsHelper.missingDataFrameAnalytics(config.getId()));
} else {
listener.onResponse(stats.get(0).getProgress());
}
},
listener::onFailure
));
}

We fail grabbing the progress on starting the second job.

This commit splits the creating/starting of the two jobs.

We still want to fail on shard failures like this as knowing the previous progress (if it exists) is vitally important.

closes #62064
closes #55807

@benwtrent benwtrent added >test Issues or PRs that are addressing/adding tests v8.0.0 v7.10.0 labels Sep 28, 2020
@davidkyle davidkyle added the :ml Machine learning label Sep 29, 2020
@elasticmachine
Copy link
Copy Markdown
Collaborator

elasticmachine commented Sep 29, 2020

Pinging @elastic/ml-core (:ml)

droberts195
droberts195 approved these changes Sep 29, 2020
View reviewed changes
Copy link
Copy Markdown

@droberts195 droberts195 left a comment

Choose a reason for hiding this comment

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

This is OK as a fix for the test, but I don't like that we're having to change tests that are doing things that users could be doing (running two jobs together on a clean system).

It all goes back to the problem that searches treat indices that don't exist differently to indices that have just been created and are not yet ready to use (but which are obviously empty as they've only just been created). I will bump the thread about doing something about that.

@benwtrent
Copy link
Copy Markdown
Member Author

benwtrent commented Sep 29, 2020

It all goes back to the problem that searches treat indices that don't exist differently to indices that have just been created and are not yet ready to use

100% agree. I wish this "fix" was not necessary.

@benwtrent benwtrent merged commit 471403e into elastic:master Sep 29, 2020
benwtrent added a commit to benwtrent/elasticsearch that referenced this pull request Sep 29, 2020
@benwtrent
[ML] fixing testTwoJobsWithSameRandomizeSeedUseSameTrainingSet tests (e…
d537d29 
…lastic#62976)

This fixes the two test failures.

The shard failure seems to be due to the .ml-stats index being in the middle of being created.
@benwtrent benwtrent mentioned this pull request Sep 29, 2020
Merged
benwtrent added a commit that referenced this pull request Sep 29, 2020
@benwtrent
[7.x] [ML] fixing testTwoJobsWithSameRandomizeSeedUseSameTrainingSet …
2b9032a 
…tests (#62976) (#62999)

* [ML] fixing testTwoJobsWithSameRandomizeSeedUseSameTrainingSet tests (#62976)

This fixes the two test failures.

The shard failure seems to be due to the .ml-stats index being in the middle of being created.
@benwtrent benwtrent deleted the test/ml-dfa-random-seed-test-fix branch October 19, 2020 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@droberts195 droberts195 droberts195 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

:ml Machine learning >test Issues or PRs that are addressing/adding tests v7.10.0 v8.0.0-alpha1

Projects

None yet

Milestone

No milestone

5 participants

@benwtrent @elasticmachine @droberts195 @jakelandis @davidkyle