Prevent threaded Dask workers in DistRDF backend - #22393
Conversation
Test Results 23 files 23 suites 3d 13h 59m 54s ⏱️ Results for commit 30314f0. ♻️ This comment has been updated with latest results. |
5bb11cb to
edb506a
Compare
2eda199 to
0c91ac5
Compare
vepadulano
left a comment
There was a problem hiding this comment.
Thank you @JAGANNATHANJP ! Before proceeding with the CI, I would like to request you to add a test for this new behaviour, perhaps in roottest/python/distrdf/backends/check_backend.py
|
Added a test covering the case where scheduler_info() does not provide worker information during Dask backend initialization. |
|
Resolved the merge conflict with the latest master and updated the branch. |
|
Hi @JAGANNATHANJP , Apparently there are still some conflicts, see e.g. https://github.com/root-project/root/actions/runs/28595013336/job/85566036258?pr=22393 |
c4078d9 to
de24d80
Compare
|
Dear @JAGANNATHANJP , The test suite is failing on all platforms with the following error Did you run the test suite locally before resubmitting your changes? |
|
Hi @vepadulano, Apologies for that! The failure was caused by an accidental indentation error during the last merge conflict resolution (df.GetValue() got shifted outside the pytest.warns block). I have fixed the indentation in commit 9a58aa7 and verified the full test suite locally inside my build environment. All 14 tests in check_backend.py (including the new Dask worker validation test and test_initialization_method) passed 100% cleanly locally. Could you please approve running the CI checks when you have a moment? Thank you! |
|
Hi @vepadulano, I investigated the recent CI failures. The failure on fedora44 (pyunittests-roottest-python-distrdf-backends-all) was caused by the Dask LocalCluster attempting to spin up a web dashboard on port 8787 during the test, which crashed on the CI runner due to port conflicts/restrictions. I have pushed a commit to explicitly pass dashboard_address=None in check_backend.py to ensure it runs completely silently. (Note: The other failures on Alma8/Alma9/Mac appear to be unrelated upstream C++ IO and CMake flakes). Could you please review when the CI finishes? Thanks! |
vepadulano
left a comment
There was a problem hiding this comment.
Thanks! There are still some things to cleanup.
|
Hi @vepadulano, I have addressed all your review comments and also fixed the 3 CI failures from the previous run. The failures were caused by a pytest.raises(...) line in check_backend.py exceeding the ruff line length limit, which is now fixed in commit c446a5b. Could you please review when the CI finishes? Thank you! |
c446a5b to
2ea26ae
Compare
vepadulano
left a comment
There was a problem hiding this comment.
Thanks for this nice piece of work @JAGANNATHANJP ! I went through the changes and I believe they look good. The last remaining test failure was just a warning raised by the dask cluster finding the same address used twice, which I fixed using the :0 syntax in the LocalCluster initialization to use a random address to avoid tripping Dask. I am running the CI now to make sure the tests pass on all platforms. No further action is required from your side, thanks again!
Using Dask workers with more than one thread does not provide any advantage, seeing how the RDataFrame computation graph runs in C++ and the Python threads are limited by the GIL anyway. Explicitly disallow using distributed RDataFrame with Dask when the workers have been configured to use more than one thread. Multithreading in RDataFrame can already be achieved on a single node via `ROOT.EnableImplicitMT`. Distributed processing runs with multiple processes instead.
2ea26ae to
30314f0
Compare
|
Thank you so much @vepadulano for the review, guidance, and help with the cluster address fix! Glad to contribute to ROOT! |
This PR prevents unsupported threaded Dask workers in DistRDF.
Distributed RDataFrame with Dask threads may lead to crashes and does not provide advantages due to Python GIL limitations. This change validates worker configuration at backend initialization and raises a RuntimeError when threaded workers are detected.
Suggested configuration: