Test narwhals in CI

[bdice]

Description

Contributes to #17662.

@MarcoGorelli provided very helpful instructions for running the narwhals test suite. We should examine and correct the failing tests. We are down to 39 failures, shown in this Kaggle notebook.

Checklist

• I am familiar with the Contributing Guidelines.
• New or existing tests cover these changes.
• The documentation is up to date with these changes.

[copy-pr-bot]

Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually.

Contributors can view more details about this message here.

[bdice]

/ok to test

[bdice]

/ok to test

[MarcoGorelli]

thanks @bdice !

import polars as pl

E ModuleNotFoundError: No module named 'polars'

there's a PR in progress to allow the test suite to be run without Polars installed, once that's in this issue should resolve itself narwhals-dev/narwhals#1896

Alternatively, you may want to install Polars so you can also run

NARWHALS_POLARS_GPU=1 pytest --constructors=polars[lazy]

?

[vyasr]

@bdice depending on how you want to scope this PR you can either address all of #17662 or just one of the different runs since in principle we can run the narwhals test suite either directly with cudf, with cudf.pandas, or with cudf-polars. I expect that we'll find different issues from all three.

[bdice]

Sounds good! I have spent very little time on this so far - just a skeleton at the moment. I will try to come up with some reasonable way to present these tests. Perhaps as three separate jobs? How do we want to handle known failures, with some kind of skip list?

[bdice]

/ok to test

[vyasr]

I think three separate tests makes sense, yes.

For how to handle failures, I would follow the model that we use for running the polars test suite of explicitly listing out tests that we deselect and why. If the list gets too long we can revisit, but I'm hopeful that the narwhals test suite is still small enough that we won't have problems at the scale that we do with the pandas test suite (and getting the narwhals test suite passing should help us with the long tail of pandas test failures).

[Matt711]

I think three separate tests makes sense, yes.

For how to handle failures, I would follow the model that we use for running the polars test suite of explicitly listing out tests that we deselect and why. If the list gets too long we can revisit, but I'm hopeful that the narwhals test suite is still small enough that we won't have problems at the scale that we do with the pandas test suite (and getting the narwhals test suite passing should help us with the long tail of pandas test failures).

Are these failures flaky? If so, skipping them makes sense. If they fail deterministically, we should xfail them, right?

[vyasr]

Are these failures flaky? If so, skipping them makes sense. If they fail deterministically, we should xfail them, right?

Ideally yes. I don't think that is so easy to do, though. There is no way to inject per-test fails via the pytest CLI, is there? I think you would have to implement a minimal pytest plugin to programmatically add an xfail marker to the relevant tests. It's not impossible, but it seems like more work than it's worth. I could be wrong though, maybe there's an easier way.

[gforsyth]

Ideally yes. I don't think that is so easy to do, though. There is no way to inject per-test fails via the pytest CLI, is there? I think you would have to implement a minimal pytest plugin to programmatically add an xfail marker to the relevant tests. It's not impossible, but it seems like more work than it's worth. I could be wrong though, maybe there's an easier way.

There is no easier way to do this if the tests are defined upstream. I have a skeleton of what is required in a (very non-production-ready) repo here: https://github.com/gforsyth/pytest-salmon that I'll work into a full-fledged pytest plugin eventually, but it always ends up feeling like a bit of a hack.

[MarcoGorelli]

Hey all

The tests you're currently running shouldn't be flaky, we run them regularly (but not in CI due to GPU requirements) to check that they're green. We did have an error in the test suite on the main branch when CI for this PR ran, but it's since been fixed, see see https://www.kaggle.com/code/marcogorelli/testing-cudf-in-narwhals

It looks like the polars-gpu CI failures are due to the missing polars-gpu dependency?

>           raise ModuleNotFoundError(err_message) from None
E           ModuleNotFoundError: GPU engine requested, but required package 'cudf_polars' not found.
E           Please install using the command `pip install cudf-polars-cu12` (or `pip install --extra-index-url=https://pypi.nvidia.com/ cudf-polars-cu11` if your system has a CUDA 11 driver).

If you include that and re-run, my expectation is that the CI from this PR should be green 🥦

FWIW, my suggestion would be:

• start running the tests for cudf and polars[gpu] in CI
• for the cudf.pandas, there's still a lot of bugs related to null value handling (e.g. AssertionError: Mismatch at index 4: 6.0 != None), once the number of failures is down to say single-digits as opposed to 40, I'd say we can add some xfails for the remaining ones in Narwhals, and then consider running those in CI too?

[bdice]

/ok to test

[bdice]

@MarcoGorelli Thanks! I've been slow on this PR, just making gradual progress when I circle around to it. I think I fixed the cudf-polars installation issues and CI is passing now.

How do you envision this going forward? Obviously we want to know if there are failures that are specific to cuDF / cudf.pandas, but it may be hard to track those down if they are xfailed in the upstream. One possible solution would be to have only a minimal number of xfails for cudf in the upstream narwhals repo, for things we don't intend to fix (like iter_rows). We could manually skip any other failing tests in our narwhals CI, so that we have a short and hardcoded list of tests that we need to fix.

A few xfails I see that are specific to cudf -- just trying to sample through what I see, to get a sense of what we might want to do:

• Pivot issue here should be fixed now that Support pivot with index or column arguments as lists #17373 is in: https://github.com/narwhals-dev/narwhals/blob/20d52d7ac554fe7d903394939818f162184d2a0c/tests/frame/pivot_test.py#L158-L161
• iter_rows is expected to fail, so we should leave this xfail in place: https://github.com/narwhals-dev/narwhals/blob/20d52d7ac554fe7d903394939818f162184d2a0c/tests/frame/rows_test.py#L38-L39
• Join duplicate columns xfail here seems like Raise NotImplementedError if .merge(suffixes=) introduces duplicate labels #17905 but maybe it's different? https://github.com/narwhals-dev/narwhals/blob/20d52d7ac554fe7d903394939818f162184d2a0c/tests/frame/join_test.py#L675-L677
• There are several more but I'll stop here for now.

[MarcoGorelli]

Great!

We've got an issue about unifying test failures narwhals-dev/narwhals#1893, and would like to only use xfail for "it currently fails but in theory should really be passing"
In addition to that, for every xfail, there should be an associated open issue in the relevant repo

So, as we move forwards, we can open more issues, report them here, and gradually remove the xfails as they get addressed (or turn them into pytest.raises contexts for ones aren't intended to be supported)

[bdice]

In addition to that, for every xfail, there should be an associated open issue in the relevant repo

This is great! Filing issues to cudf is a good strategy for tracking problems going forward -- and we appreciate your willingness to do that.

So, as we move forwards, we can open more issues, report them here, and gradually remove the xfails as they get addressed (or turn them into pytest.raises contexts for ones aren't intended to be supported)

I agree that pytest.raises is the right answer for some of these, like iter_rows. Thanks for your help!

[Matt711]

Thanks @bdice! I'll focus on running the test with cudf.pandas in a follow-up PR, but this is a great starting point.