[CI] Check test files for if __name__... snippet
#25322
Conversation
Yard1
requested review from
sven1977,
gjoliver,
avnishn,
ArturNiederfahrenhorst,
smorad,
maxpumperla,
kouroshHakha and
krfricke
as code owners
|
@ericl let me know if something like that would be desired for core, data, serve, etc. tests. Can make followup PRs. |
|
Definitely desired for Serve! |
python/ray/ml/BUILD
Outdated
| @@ -335,3 +335,12 @@ py_library( | |||
| name = "ml_lib", | |||
| srcs = glob(["**/*.py"], exclude=["tests/*.py"]), | |||
| ) | |||
|
|
|||
| # Check if all tests have the pytest if __name__... snippet. | |||
There was a problem hiding this comment.
Wondering if there is an easy way to do this for all BUILD files. Otherwise, it's very easy to forget to add this for new BUILD files that are created.
There was a problem hiding this comment.
I am no bazel expert, but I couldn't really find anything that would allow that. All solutions either involved modifying the BUILD files a little (this PR) or completely
|
Another approach can be:
|
|
@simon-mo I am a bit worried that may catch things we don't want (eg. the tests in docs don't need this snippet as they are all running in main). This seems to be more configurable, at the cost of having to modify BUILD files. Definetely a good idea, though! Happy to disscuss the approach here. |
There was a problem hiding this comment.
This is great, the problem definitely has bit me in the past.
I would suggest to follow @simon-mo's approach though and add this as a linter step. The reason is that it's easy to miss tests that are either not captured by the glob or to forgert to include this in new BUILD files (especially as we may be moving to a more package-like BUILD structure in the future). Also, this test is arguably a linter check, so it would be great to include it there.
My proposal here would be:
- We add a tag
no_mainfor all tests that don't expect the__name__part - We run a bazel query (something like
bazel query 'kind(py_test.*, tests(python/...) except attr(tags, "\bno_main\b", python/...))') we can parse this to json with yq via--output xml | xq . - Extract the files to check and run your test against it.
I don't think we lose any flexibility with the tag but we would make sure that everything declared as a py_test will be caught.
What do you think?
|
Sure, that sounds good. Let me implement that |
| @@ -624,7 +625,8 @@ def train_with_backend( | |||
| _, eval_preds, _ = model.evaluate(dataset=dataset) | |||
| assert backend.df_engine.compute(eval_preds) is not None | |||
|
|
|||
| return model.model.get_weights() | |||
| ret = True | |||
There was a problem hiding this comment.
Why can't we continue to use get_weights() here?
There was a problem hiding this comment.
API has changed and that method is no longer present. As the return value wasn't being used, I just switched to this
Why are these changes needed?
Bazel operates by simply running the python scripts given to it in
py_test. If the script doesn't invoke pytest on itself in theif _name__ == "__main__"snippet, no tests will be ran, and the script will pass. This has led to several tests (indeed, some are fixed in this PR) that, despite having been written, have never ran in CI. This PR adds a lint check to check allpy_testsources for the presence ofif _name__ == "__main__"snippet, and will fail CI if there are any detected without it. This system is only enabled for libraries right now (tune, train, air, rllib), but it could be trivially extended to other modules if approved.Related issue number
Checks
scripts/format.shto lint the changes in this PR.