-
Notifications
You must be signed in to change notification settings - Fork 144
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 flaky model tests #1429
Merged
ryanbogan
merged 4 commits into
opensearch-project:main
from
ryanbogan:fix_flaky_model_tests
Jan 29, 2024
Merged
Fix flaky model tests #1429
ryanbogan
merged 4 commits into
opensearch-project:main
from
ryanbogan:fix_flaky_model_tests
Jan 29, 2024
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Signed-off-by: Ryan Bogan <rbogan@amazon.com>
Signed-off-by: Ryan Bogan <rbogan@amazon.com>
Signed-off-by: Ryan Bogan <rbogan@amazon.com>
Signed-off-by: Ryan Bogan <rbogan@amazon.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1429 +/- ##
============================================
- Coverage 84.92% 84.84% -0.08%
+ Complexity 1274 1273 -1
============================================
Files 167 167
Lines 5186 5186
Branches 491 491
============================================
- Hits 4404 4400 -4
- Misses 574 577 +3
- Partials 208 209 +1 ☔ View full report in Codecov by Sentry. |
jmazanec15
approved these changes
Jan 29, 2024
martin-gaievski
approved these changes
Jan 29, 2024
opensearch-trigger-bot bot
pushed a commit
that referenced
this pull request
Jan 29, 2024
* Fix flaky model tests in k-NN Signed-off-by: Ryan Bogan <rbogan@amazon.com> * Remove * imports Signed-off-by: Ryan Bogan <rbogan@amazon.com> * Minor change Signed-off-by: Ryan Bogan <rbogan@amazon.com> * Add changelog entry Signed-off-by: Ryan Bogan <rbogan@amazon.com> --------- Signed-off-by: Ryan Bogan <rbogan@amazon.com> (cherry picked from commit 9e4251e)
ryanbogan
added a commit
that referenced
this pull request
Jan 30, 2024
* Fix flaky model tests in k-NN Signed-off-by: Ryan Bogan <rbogan@amazon.com> * Remove * imports Signed-off-by: Ryan Bogan <rbogan@amazon.com> * Minor change Signed-off-by: Ryan Bogan <rbogan@amazon.com> * Add changelog entry Signed-off-by: Ryan Bogan <rbogan@amazon.com> --------- Signed-off-by: Ryan Bogan <rbogan@amazon.com> (cherry picked from commit 9e4251e) Co-authored-by: Ryan Bogan <rbogan@amazon.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Since
ModelDaoTests
extendsKNNSingleNodeTestCase
, a node is started at the beginning of each test and stopped at the end of each test. TheTrainingJobClusterStateListenerClass
contains aclusterChanged
method that is called each time a new cluster is started. Within this method, a separate thread runs through all existing models and marks any that are stuck in training as failed. This can result in a scenario that causes flaky tests.In this scenario, a node is started at the beginning of a test, calling the
clusterChanged
method. The test creates a model (or multiple), verifies the expected behavior, and then stops the node. Meanwhile, the other thread is in the process of looping through any existing models and potentially updating their model state parameter. If the node stops before this process is complete, an exception will be thrown saying that the model index (.opensearch-knn-models) does not exist.To fix the flaky test situation, the
TrainingJobClusterStateListener
class has been mocked to no-op theclusterChanged
method in the setup for the class. This means that there will be no separate thread accessing the models for the entireModelDaoTests
class. In the teardown after the class, the mocked static is closed so it will not affect any other test classes.Issues Resolved
#1376
#1413
Check List
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.