-
Notifications
You must be signed in to change notification settings - Fork 28.9k
[SPARK-13023][PROJECT INFRA][BRANCH-1.6] Fix handling of root module in modules_to_test() #12743
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
…in modules_to_test() This is a 1.6 branch backport of SPARK-13023. There's a minor bug in how we handle the `root` module in the `modules_to_test()` function in `dev/run-tests.py`: since `root` now depends on `build` (since every test needs to run on any build test), we now need to check for the presence of root in `modules_to_test` instead of `changed_modules`.
|
LGTM pending Jenkins. |
|
Test build #57172 has finished for PR 12743 at commit
|
|
Thanks. Merging to branch 1.6. |
…in modules_to_test() This is a 1.6 branch backport of SPARK-13023 based on JoshRosen's 41f0c85. There's a minor bug in how we handle the `root` module in the `modules_to_test()` function in `dev/run-tests.py`: since `root` now depends on `build` (since every test needs to run on any build test), we now need to check for the presence of root in `modules_to_test` instead of `changed_modules`. Author: Yin Huai <[email protected]> Closes #12743 from yhuai/1.6build.
…in modules_to_test() This is a 1.6 branch backport of SPARK-13023 based on JoshRosen's apache@41f0c85. There's a minor bug in how we handle the `root` module in the `modules_to_test()` function in `dev/run-tests.py`: since `root` now depends on `build` (since every test needs to run on any build test), we now need to check for the presence of root in `modules_to_test` instead of `changed_modules`. Author: Yin Huai <[email protected]> Closes apache#12743 from yhuai/1.6build. (cherry picked from commit f4af6a8)
| modules_to_test = modules_to_test.union(determine_modules_to_test(module.dependent_modules)) | ||
| # If we need to run all of the tests, then we should short-circuit and return 'root' | ||
| if modules.root in modules_to_test: | ||
| return [modules.root] |
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.
Hi @yhuai and @JoshRosen, I realised that #13806 is being failed due to this one.
In the case of my PR, it corrects two files in sql and core. Since it seems fixing core module triggers all tests by root value from determine_modules_for_files.
So, changed_modules becomes as below:
['root', 'sql']
and dependent_modules becaomes as below:
['pyspark-mllib', 'pyspark-ml', 'hive-thriftserver', 'sparkr', 'mllib', 'examples', 'pyspark-sql']
Now, modules_to_test does not include root, this checking is skipped but then both changed_modules and modules_to_test are being merged after that. So, this includes root module to test.
This ends up with failing with the message below (e.g. https://amplab.cs.berkeley.edu/jenkins/job/SparkPullRequestBuilder/60990/consoleFull):
Error: unrecognized module 'root'. Supported modules: pyspark-core, pyspark-sql, pyspark-streaming, pyspark-ml, pyspark-mllib
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.
It seems okay for branch-2.0 and master as root is being checked after merging both chaned_modules and modules_to_test. https://github.com/apache/spark/blob/master/dev/run-tests.py#L119-L122
This is a 1.6 branch backport of SPARK-13023 based on @JoshRosen's 41f0c85.
There's a minor bug in how we handle the
rootmodule in themodules_to_test()function indev/run-tests.py: sincerootnow depends onbuild(since every test needs to run on any build test), we now need to check for the presence of root inmodules_to_testinstead ofchanged_modules.