-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
[ci] Fix R 3.6 tests, dask tests, compatibility with dask>=2024.3.1 #6357
Conversation
It seems to fix the issue in #6353 but there's a new failure, unfortunately, when setting up clang-18 (https://github.com/microsoft/LightGBM/actions/runs/8231989739/job/22510837736?pr=6357). I'm a bit surprised that it uses the Debian setup instructions on |
Thanks! I've added label LightGBM/.github/release-drafter.yml Lines 3 to 15 in b27d81e
That's used by that
Those debian jobs do get scheduled onto an LightGBM/.github/workflows/r_package.yml Line 280 in b27d81e
When possible, could you put a bit of the relevant logs in plaintext for comments like that? That helps other people experiencing similar issues to find our solution from search engines. I know I've benefitted a lot from finding other random GitHub threads when faced with some unexpected warning or error. Here's what I see:
I found some similar posts on the internet that might have useful information:
I'm not sure what's happening there. If you can't find a resolution, drop |
I just retried this, and the So I just pushed a commit removing that job for now, and will put up an issue documenting the need to restore it if/when this is merged. |
Looks like in the time since this was started, other CI issues have emerged. I'm going to push commits here to try to address those... we'll have to get all of them resolved on one branch to be able to unblock the project's CI. Issue 1: new required dependency for
|
Looks like the commits I pushed did help resolve #6366 🎉 And that now the Dask tests are running... but failing because they're incompatible with the latest version of Also looks like we've picked up ANOTHER blocking CI issue, that seems unrelated to the rest of these 😭 Issue 3: r-sanitizers jobs segfaulting while installing packagesDocumented in #6367 I've updated the PR description and title to reflect that this is addressing all of these issues. |
With the changes in f1aa403, the Dask tests are passing 🎉 But the latest I just pushed e789b15 to try to fix that. |
Alright I think this is now working and that merging it would unblock CI for the project. Since I've pushed a lot of commits here ... @borchero @jmoralez could one of you take another look? I've tried to update the description to summarize the changes. The most important thing to note: this does not "fix" the R jobs using |
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.
Thanks for the fixes @jameslamb! Sorry I left this in a half-done state, I didn't have enough time to see this through last week 🫣
I can't approve my own PR but consider this an approval 😄
# But in exchange, it's less likely that 'import lightgbm' will fail for | ||
# dask-related reasons, which is beneficial for any workloads that are using | ||
# lightgbm but not its Dask functionality. | ||
except (ImportError, ValueError): |
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.
In the future, we might catch ValueError
and re-raise it if the error message is not "Must install dask-expr to activate query planning". Obviously, this isn't ideal either as dask
may change the message at any time...
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.
In any case, reading through #6365, I find it surprising that the ValueError
is raised at all. If dask
is a non-installed optional dependency, the ImportError
should be raised first. If it is installed, it does not seem to be an issue, at least, for conda-forge
-- is the pip
package for dask
not configured correctly? 🤔
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.
re-raise it if the error message is not "Must install dask-expr to activate query planning"
I'd rather not take on any coupling to specific error messages. I hope my PR to dask
will be accepted, and then we could remove this addition of ValueError
.
If dask is a non-installed optional dependency, the ImportError should be raised first
The ValueError
doesn't happen at install time of dask
. It happens at import time.
If you haven't yet, see the description and diff at dask/dask#11007. This ValueError
comes from code that only runs when you try to import dask.dataframe
.
is the
pip
package for dask not configured correctly?
The dask
package on conda-forge lists dask-expr
as a strong runtime dependency (code link).
The dask
wheel on PyPI does not... but it does list dask-expr
as a dependency of dask[dataframe]
(code link).
Neither of those is "correct" or "incorrect"... they're just different.
Fixes multiple CI-blocking issues that have all come up in the last few days:
dask.dataframe
now requiringdask-expr
([ci] [python-package] [dask] some CI jobs broken, others skipping Dask tests #6365)clang-18
failing in ther-debian-clang-devel
CI jobs ([ci] [R-package] clang-18 jobs failing #6369)r-sanitizers
jobs failing with install-time segfaults ([ci] [R-package] r-sanitizers jobs segfaulting while installing packages #6367)