-
Notifications
You must be signed in to change notification settings - Fork 912
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
Propagate failures in pandas integration tests and Skip failing tests #17521
Propagate failures in pandas integration tests and Skip failing tests #17521
Conversation
For the reviewer, I'll follow-up this PR to fix failing tests. xref #17490 |
python/cudf/cudf_pandas_tests/third_party_integration_tests/tests/test_xgboost.py
Outdated
Show resolved
Hide resolved
python/cudf/cudf_pandas_tests/third_party_integration_tests/tests/test_tensorflow.py
Show resolved
Hide resolved
python/cudf/cudf_pandas_tests/third_party_integration_tests/tests/test_seaborn.py
Outdated
Show resolved
Hide resolved
python/cudf/cudf_pandas_tests/third_party_integration_tests/tests/test_pytorch.py
Outdated
Show resolved
Hide resolved
python/cudf/cudf_pandas_tests/third_party_integration_tests/tests/test_numpy.py
Outdated
Show resolved
Hide resolved
python/cudf/cudf_pandas_tests/third_party_integration_tests/tests/test_matplotlib.py
Outdated
Show resolved
Hide resolved
python/cudf/cudf_pandas_tests/third_party_integration_tests/tests/test_holoviews.py
Outdated
Show resolved
Hide resolved
python/cudf/cudf_pandas_tests/third_party_integration_tests/tests/test_holoviews.py
Outdated
Show resolved
Hide resolved
…sts/test_xgboost.py
…sts/test_tensorflow.py
…sts/test_seaborn.py
…sts/test_pytorch.py
…sts/test_numpy.py
…sts/test_matplotlib.py
…sts/test_holoviews.py
…sts/test_holoviews.py
python/cudf/cudf_pandas_tests/third_party_integration_tests/tests/test_xgboost.py
Outdated
Show resolved
Hide resolved
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.
Is the plan to merge this after we add xfails? Sorry I think I've lost track of the various discussions we've had, I know I've asked this a couple of times already.
Yes that is still the plan. A few of the tests that are failing with |
python/cudf/cudf_pandas_tests/third_party_integration_tests/tests/test_catboost.py
Outdated
Show resolved
Hide resolved
…sts/test_catboost.py
python/cudf/cudf_pandas_tests/third_party_integration_tests/tests/test_catboost.py
Outdated
Show resolved
Hide resolved
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.
@@ -1,129 +0,0 @@ | |||
# Copyright (c) 2024, NVIDIA CORPORATION. |
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.
I'm getting strange errors in the catboost
test suite. Pytest fails to collect the tests with this error.
ValueError: numpy.dtype size changed, may indicate binary incompatibility. Expected 96 from C header, got 88 from PyObject
From a cursory investigation, it may be due to the version of NumPy installed that's causing problems. I tried skipping the tests but it still fails. I'm deleting the tests for now and will make it priority to add them back in #17490.
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.
This is probably due to ABI incompatibility between the version of NumPy that catboost is compiled against and the version of of NumPy installed in test CI job.
.github/workflows/pr.yaml
Outdated
@@ -40,6 +40,7 @@ jobs: | |||
- pandas-tests | |||
- pandas-tests-diff | |||
- telemetry-setup | |||
- third-party-integration-tests-cudf-pandas |
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.
I'll delete these changes to pr.yaml
once this PR is approved. I only added them to run the CI job in this PR.
Why delete catboost? It looks like the failure is a runtime issue and not like a seg fault, so can't you just mark the test as a skip? |
Discussed offline, this test still seems to make the suite fail even if it is skipped so we'll drop it for now and add back in a follow-up. |
/merge |
1 similar comment
/merge |
5baaf6d
into
rapidsai:branch-25.02
Description
This PR ensures that the integration tests fail in any one of the test modules fails. It also skips of xfails any tests that are not currently passing. Finally, it fixes one incorrect use of
rng.random
.Some of the change were originally made in #17489
Checklist