Skip to content
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

Merged
merged 8 commits into from
Mar 18, 2024

Conversation

borchero
Copy link
Collaborator

@borchero borchero commented Mar 11, 2024

Fixes multiple CI-blocking issues that have all come up in the last few days:

@borchero
Copy link
Collaborator Author

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 ubuntu-latest 🤔

@jameslamb
Copy link
Collaborator

Thanks! I've added label maintenace on this. In the future, can you please add one of the labels from this list to every PR open?

categories:
- title: '💡 New Features'
label: 'feature'
- title: '🔨 Breaking'
label: 'breaking'
- title: '🚀 Efficiency Improvement'
label: 'efficiency'
- title: '🐛 Bug Fixes'
label: 'fix'
- title: '📖 Documentation'
label: 'doc'
- title: '🧰 Maintenance'
label: 'maintenance'

That's used by that release-drafter bot to automatically create the sections in release notes you can see at https://github.com/microsoft/LightGBM/releases.


I'm a bit surprised that it uses the Debian setup instructions on ubuntu-latest 🤔

Those debian jobs do get scheduled onto an ubuntu-latest GitHub Actions virtual machine, but the main building and testing logic of those jobs all runs in a container using the rhub/debian-clang-devel image from DockerHub.

container: rhub/debian-clang-devel


there's a new failure, unfortunately, when setting up clang-18 (https://github.com/microsoft/LightGBM/actions/runs/8231989739/job/22510837736?pr=6357)

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:

The following packages have unmet dependencies:
 clangd-18 : Depends: libabsl20220623t64 (>= 0~20220623.0-1) but it is not installable
             Depends: libgrpc++1.51t64 (>= 1.51.1) but it is not installable
             Depends: libgrpc29t64 (>= 1.51.1) but it is not installable
             Depends: libprotobuf32t64 (>= 3.21.12) but it is not installable
 liblldb-18 : Depends: libcurl4t64 (>= 7.16.2) but it is not installable
              Depends: libpython3.11t64 (>= 3.11.5) but it is not installable
 libomp-18-dev : Depends: libpython3.11t64 (>= 3.11.5) but it is not installable
 llvm-18 : Depends: libcurl4t64 (>= 7.16.2) but it is not installable
E: Unable to correct problems, you have held broken packages.

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 clang-18 from that test matrix and write up an issue documenting the need to re-enable the job, and I could look into it more next week.

@jameslamb
Copy link
Collaborator

I just retried this, and the clang-18 job failed again: https://github.com/microsoft/LightGBM/actions/runs/8231989739/job/22740962055?pr=6357

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.

@jameslamb
Copy link
Collaborator

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 dask.dataframe

Documented in #6365

Issue 2: basictex homebrew formula was updated and TeX mirrors don't have the old files

Documented in #6366

@jameslamb jameslamb changed the title [ci] Fix tests for R 3.6 [ci] Fix R 3.6 tests, dask tests Mar 17, 2024
@jameslamb
Copy link
Collaborator

jameslamb commented Mar 17, 2024

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 dask: #6365 (comment)


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 packages

Documented in #6367


I've updated the PR description and title to reflect that this is addressing all of these issues.

@jameslamb
Copy link
Collaborator

With the changes in f1aa403, the Dask tests are passing 🎉

But the latest dask release caused a different error in the "can lightgbm be imported without its oprtional dependencies" check, documented at #6365 (comment).

I just pushed e789b15 to try to fix that.

@jameslamb
Copy link
Collaborator

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 clang-18 or sanitizers. It just stops them from blocking CI. I'm proposing not closing the related issues until those test jobs have been restored.

@jameslamb jameslamb changed the title [ci] Fix R 3.6 tests, dask tests [ci] Fix R 3.6 tests, dask tests, compatibility with dask>=2024.3.1 Mar 18, 2024
Copy link
Collaborator Author

@borchero borchero left a 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):
Copy link
Collaborator Author

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...

Copy link
Collaborator Author

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? 🤔

Copy link
Collaborator

@jameslamb jameslamb Mar 18, 2024

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.

@jameslamb
Copy link
Collaborator

No problem, thanks for helping with it @borchero !

I'm going to merge this based on both of us approving each others' changes, and to unblock development on other PRs.

Any of these things could be changed or reverted easily if @jmoralez has suggested changes whenever he's able to review this.

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