Reorganize tests and modify pytest invocation to support pytest 8 #6876
Conversation
|
If merging in |
|
I'm afraid I haven't had time to look into this yet, and I probably won't for a while. @csadorf if you think this is important we may need to coordinate a handoff to get it in sooner. Otherwise I'll get around to it when I can. |
I don't think it's very urgent at the moment. I am going to preemptively target this for 25.10. |
30211b2 to
7fd3f28
Compare
|
Came here to merge this but between the last time I looked at this and now it seems the scope grew quite a bit. It went from "let's remove the pin" to a whole sale reorganisation of the testing. It is not clear to me why cuml is special in the sense that we need to specify The docs say that Overall, there must be a better way. The way to run tests is already pretty convoluted (compared to industry standard "run |
csadorf
left a comment
There was a problem hiding this comment.
Considering the extent and scope of this PR, I'd like to review this before merge.
|
@vyasr Can you make sure that the PR title represents the changes here? |
cuml is not special. We already have this set in most RAPIDS libraries, including raft, cugraph, and ucxx. We also had to do a similar move of the files in the corresponding cudf PR. I have not taken the time to fully understand how pytest's rules for discovery have changed in pytest 8 or why they impact different libraries in subtly different ways, but they very clearly do based on what we've observed.
I would be happy if one could be found. However, in searching for one please note that cuml merging this PR is now a blocker for CI in other places. Since pytest 8 now requires a newer version of pytest-asyncio and pytest-asyncio made some breaking changes, the merge of the corresponding ucxx PR has made combined environments with both ucxx and cuml unsolvable, which blocks testing of RAPIDS in CCCL's CI, which hurts our ability to ensure that they don't merge breaking changes on us. One way or another we need to get the pin removed soon, so please account for that when deciding how to prioritize merging this PR. |
|
Yes - this is essentially the same types of changes (test reorganizations) that we've already made in other RAPIDS libraries. I support merging this soon, in order to unbreak our nightly CI and combined devcontainers and such. |
csadorf
left a comment
There was a problem hiding this comment.
I had a closer look at this and have come to the conclusion that cuML's pytest import mode should probably be append (or importlib), but not prepend, because we typically want to test the installed (and compiled) version rather than importing directly from source.
I've performed a small investigation by running pytest in prepend mode without any other tricks (like cd'ing into the cuml/tests/ directory) and found that pytest will, as one of its first actions during collection, try to import the cuml.benchmark.automated.dask.conftest module which is within a package. This causes pytest to walk up the path and try to import the cuml root module, which will eventually fail with
ERROR - ModuleNotFoundError: No module named 'cuml.internals.logger'
because that is a Cython module.
Removing that conftest module, we run into the next problem: we are defining a number of pytest plugins within python/cuml/cuml/tests/conftest.py, which is not allowed as of pytest version 0.4.
The current hybrid approach of distributing our test modules within a cuml/tests/ directory, but not actually having a cuml.tests namespace, poses the worst of both worlds. We cannot utilize the ability to run tests via pyargs, but we run into a ton of problems when trying to invoke pytest from the root directory.
I have been hoping to address some of these quirks for quite some time and I believe that while somewhat rushed, the changes here pose an improvement. Hopefully we can then take the time and maybe revise our tests directory structure more holistically in a follow-up and either enable pytest --pyargs cuml or at least enable basic test execution from the repository root via pytest without the need for various wrapper scripts.
|
/merge |
6e73095
into
rapidsai:branch-25.10
Contributes to rapidsai/build-planning#105
To make this work, I had to run all tests with pytest's
--import-mode=appendargument to get around issues with the new defaultimportlibmode. That also necessitated moving the tests out of the cuml package source directory to avoid conflicts, then adding__init__.pyfiles to each of the test directories to avoid import conflicts with the variousconftest.pyfiles.