Skip to content

[Python] Fix RDF Pythonization tests with builtin_llvm=OFF #18192

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

Merged
merged 2 commits into from
Mar 31, 2025

Conversation

guitargeek
Copy link
Contributor

@guitargeek guitargeek commented Mar 28, 2025

On my machine, the RDF pythonization tests don't run when I build ROOT with builtin_llvm=OFF. There is a crash unless I do import numba in the beginning.

I guess it's not a priority to understand the underlying problem because not many people build ROOT like this and the workaround is easy, but at least the tests should be green.

We do the same "magically ordered imports" also in other Python tests where ROOT doesn't work together with xgboost because of std::regexp symbol clashes, unless you do the import in a certain order:

#15183

Copy link

github-actions bot commented Mar 29, 2025

Test Results

    19 files      19 suites   5d 4h 0m 37s ⏱️
 2 737 tests  2 736 ✅ 0 💤 1 ❌
50 183 runs  50 182 ✅ 0 💤 1 ❌

For more details on these failures, see this check.

Results for commit f89db46.

♻️ This comment has been updated with latest results.

@guitargeek guitargeek requested a review from bellenot as a code owner March 29, 2025 17:36
@guitargeek guitargeek force-pushed the numba_crash branch 7 times, most recently from f7d13b1 to 2c0c142 Compare March 29, 2025 22:08
@guitargeek guitargeek requested a review from couet as a code owner March 29, 2025 22:08
@guitargeek guitargeek force-pushed the numba_crash branch 2 times, most recently from f89db46 to fbe69e0 Compare March 31, 2025 09:18
Copy link
Member

@vepadulano vepadulano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's unfortunate :( but I agree it wouldn't be the first time we see something like this and changing the test is ok. I left a minor comment

Copy link
Member

@vepadulano vepadulano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Let's still wait for the CI to finish before merging

On my machine, the RDF pythonization tests don't run when I build ROOT
with `builtin_llvm=OFF`. There is a crash unless I do `import numba` in
the beginning.

I guess it's not a priority to understand the underlying problem because
not many people build ROOT like this and the workaround is easy, but at
least the tests should be green.

We do the same "magically ordered imports" also in other Python tests
where ROOT doesn't work together with xgboost because of `std::regexp`
symbol clashes, unless you do the import in a certain order:

root-project#15183
@guitargeek guitargeek merged commit 9889438 into root-project:master Mar 31, 2025
13 of 22 checks passed
@guitargeek guitargeek deleted the numba_crash branch March 31, 2025 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants