-
-
Notifications
You must be signed in to change notification settings - Fork 25.8k
CI allow to run only selected tests but on all random seeds #23026
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
Conversation
test_kmeans_plusplus_norms test_kmeans_elkan_results
TODO: skip azure linting (partially because next jobs depend on its success) |
@thomasjpfan do you think it would be possible to create a workflow that triggers on some comment in the PR and pushes to the branch ? Also do you know why the linux docker job fails because I'm just trying to add a new env var ? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thomasjpfan do you think it would be possible to create a workflow that triggers on some comment in the PR and pushes to the branch ?
Yes. The easiest way to do it is with GitHub Actions. Although, we would need to be careful about who can activate the command tho.
build_tools/azure/posix.yml
Outdated
@@ -37,6 +37,7 @@ jobs: | |||
TEST_DOCSTRINGS: 'false' | |||
CREATE_ISSUE_ON_TRACKER: 'false' | |||
SHOW_SHORT_SUMMARY: 'false' | |||
SELECTED_TESTS: $[ dependencies.git_commit.outputs['commit.selected'] ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rather than having git_commit
parses the commit message and pass it here, can we have a new step in poxis.yml
that parses the commit message and pass it along to the next steps?
The upside is that the test configuration is responsible for parsing the commit to select tests and not the git_commit
job. The downside is that we need to repeat a little bit of code for extracting the commit, but I think it's easier for future maintainers compared to using a dependencies.git_commit
.
build_tools/azure/posix-docker.yml
Outdated
@@ -84,6 +85,7 @@ jobs: | |||
-e OPENBLAS_NUM_THREADS=$OPENBLAS_NUM_THREADS | |||
-e SKLEARN_SKIP_NETWORK_TESTS=$SKLEARN_SKIP_NETWORK_TESTS | |||
-e BLAS=$BLAS | |||
-e SELECTED_TESTS=$SELECTED_TESTS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also do you know why the linux docker job fails because I'm just trying to add a new env var ?
The error is weird. My guess is that the brackets ([]
) in $SELECTED_TESTS
is making bash parse the command weirdly. Maybe this works:
-e SELECTED_TESTS=$SELECTED_TESTS | |
-e SELECTED_TESTS="$SELECTED_TESTS" |
test_kmeans_plusplus_norms test_kmeans_elkan_results
test_kmeans_plusplus_norms test_kmeans_elkan_results
test_kmeans_plusplus_norms test_kmeans_elkan_results
test_kmeans_plusplus_norms test_kmeans_elkan_results
test_kmeans_plusplus_norms test_kmeans_elkan_results
test_kmeans_plusplus_norms test_kmeans_elkan_results
test_kmeans_plusplus_norms test_kmeans_elkan_results
test_kmeans_plusplus_norms test_kmeans_elkan_results
build_tools/azure/posix-docker.yml
Outdated
# <test_name_2> | ||
# ... | ||
if [[ "$COMMIT_MESSAGE" =~ \[all\ random\ seeds\] ]]; then | ||
message=$(echo $COMMIT_MESSAGE | sed -n -e 's/^.*all\ random\ seeds\] //p') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think unix commands harder to maintain in the long run. What do you think of using a simple Python script to parse the commit message and return the selected tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, I moved that in a python script
test_kmeans_plusplus_norms test_kmeans_elkan_results
test_kmeans_plusplus_norms test_kmeans_elkan_results
test_kmeans_plusplus_norms test_kmeans_elkan_results
test_kmeans_plusplus_norms test_kmeans_elkan_results
test_kmeans_plusplus_norms test_kmeans_elkan_results
test_kmeans_plusplus_norms test_kmeans_elkan_results
test_kmeans_plusplus_norms test_kmeans_elkan_results
test_kmeans_plusplus_norms test_kmeans_elkan_results
test_kmeans_plusplus_norms test_kmeans_elkan_results
test_kmeans_plusplus_norms test_kmeans_elkan_results
test_kmeans_plusplus_norms test_kmeans_elkan_results
test_kmeans_plusplus_norms test_kmeans_elkan_results
test_kmeans_plusplus_norms test_kmeans_elkan_results
test_kmeans_plusplus_norms test_kmeans_elkan_results
test_kmeans_plusplus_norms test_kmeans_elkan_results
test_kmeans_plusplus_norms test_kmeans_elkan_results
test_kmeans_plusplus_norms test_kmeans_elkan_results
test_kmeans_plusplus_norms test_kmeans_elkan_results
test_kmeans_plusplus_norms test_kmeans_elkan_results
test_kmeans_plusplus_norms test_kmeans_elkan_results
test_kmeans_plusplus_norms test_kmeans_elkan_results
test_kmeans_plusplus_norms test_kmeans_elkan_results
test_kmeans_plusplus_norms test_kmeans_elkan_results
test_kmeans_plusplus_norms test_kmeans_elkan_results
test_kmeans_plusplus_norms test_kmeans_elkan_results
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM as well!
When using the global_random_seed fixture in new tests, we check that it works locally on all random seeds but it can happen that some seeds still fail on different platform (see #23014).
This PR gives the possibility to run the CI (azure) on only a given set of tests, for all random seeds, based on the commit message. It will trigger if the commit message is of the form: