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.

Sign up for GitHub

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

Jump to bottom

[FEA] Add an environment variable to fail on fallback in cudf.pandas #16562

Merged
merged 7 commits into from
Sep 25, 2024
Merged

[FEA] Add an environment variable to fail on fallback in cudf.pandas #16562

merged 7 commits into from
Sep 25, 2024

Conversation

Matt711
Copy link
Contributor

@Matt711 Matt711 commented Aug 14, 2024 •
edited
Loading

Description

This PR makes more on #14975 by adding an environment variable that fails when fallback occurs in cudf.pandas. It also adds some tests that do not fallback.

Checklist

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

@Matt711 Matt711 added non-breaking Non-breaking change cudf.pandas Issues specific to cudf.pandas labels Aug 14, 2024
@Matt711 Matt711 self-assigned this Aug 14, 2024
@github-actions github-actions bot added the Python Affects Python cuDF API. label Aug 14, 2024
@Matt711 Matt711 added the feature request New feature or request label Aug 14, 2024
@Matt711 Matt711 changed the title [FEA] Warn on fallback in cudf.pandas [FEA] Add decorator to fail tests on fallback in cudf.pandas Aug 30, 2024
@copy-pr-bot copy-pr-bot
Copy link

copy-pr-bot bot commented Sep 3, 2024

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@Matt711 Matt711 marked this pull request as ready for review September 4, 2024 23:39
@Matt711 Matt711 requested a review from a team as a code owner September 4, 2024 23:39
Matt711
Matt711 commented Sep 5, 2024
View reviewed changes
Copy link
Contributor Author

@Matt711 Matt711 left a comment

Choose a reason for hiding this comment

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

For the reviewer: I can add some more tests, but I wanted to get some feedback on the approach.

python/cudf/cudf_pandas_tests/test_cudf_pandas_fallback.py Outdated Show resolved Hide resolved
python/cudf/cudf_pandas_tests/test_cudf_pandas_fallback.py Outdated Show resolved Hide resolved
@Matt711 Matt711 requested review from mroeschke and bdice September 5, 2024 00:46
@Matt711 Matt711 added the 2 - In Progress Currently a work in progress label Sep 5, 2024
@Matt711
Copy link
Contributor Author

Matt711 commented Sep 9, 2024

/ok to test

@mroeschke
Copy link
Contributor

Should the PR title be edited to say that this adds an environment variable instead of a decorator to warn on fallback?

Also, do we intend for this environment variable to be publicly usable? If we, we should probably add it in the docs somewhere

@Matt711 Matt711 changed the title [FEA] Add decorator to fail tests on fallback in cudf.pandas [FEA] Add an environment variable to fail on fallback in cudf.pandas Sep 11, 2024
@Matt711
Copy link
Contributor Author

Matt711 commented Sep 18, 2024

/ok to test

@Matt711 Matt711 force-pushed the assert-fallback-false branch from d37f525 to 84d7110 Compare September 18, 2024 17:49
@Matt711
Copy link
Contributor Author

Matt711 commented Sep 18, 2024

/ok to test

@Matt711
Copy link
Contributor Author

Matt711 commented Sep 19, 2024

/ok to test

@Matt711 Matt711 added 3 - Ready for Review Ready for review by team and removed 2 - In Progress Currently a work in progress labels Sep 20, 2024
@Matt711
Copy link
Contributor Author

Matt711 commented Sep 20, 2024

For the reviewer: CI takes a while, so I'd prefer a review before I address the merge conflict.

galipremsagar
galipremsagar reviewed Sep 20, 2024
View reviewed changes
python/cudf/cudf_pandas_tests/test_cudf_pandas_no_fallback.py

@pytest.fixture(autouse=True)
def fail_on_fallback(monkeypatch):
monkeypatch.setenv("CUDF_PANDAS_FAIL_ON_FALLBACK", "True")
Copy link
Contributor

Choose a reason for hiding this comment

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

Will this have to be unset and is the scope of the fixture guaranteed to be limited to this file when ran with pytest-xdist with worksteal algo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to the pytest docs, the default scope for the fixture is at the function level. So I think each test gets the environment variable set and unset on startup and teardown, respectively.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also, it doesn't look like we don't run worksteal for the cudf.pandas test suite. Should we?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup, can you add that as part of this pr

@Matt711 Matt711 requested a review from a team as a code owner September 20, 2024 16:37
@Matt711 Matt711 added 2 - In Progress Currently a work in progress and removed 3 - Ready for Review Ready for review by team labels Sep 23, 2024
@Matt711
Copy link
Contributor Author

Matt711 commented Sep 23, 2024

/ok to test

@Matt711
Copy link
Contributor Author

Matt711 commented Sep 23, 2024 •
edited
Loading

I'm looking into why CI failed. It may have been do to adding

    --numprocesses=8 \
    --dist=worksteal \

to the pytest command for cudf.pandas tests.

@Matt711
Copy link
Contributor Author

Matt711 commented Sep 25, 2024

I'm looking into why CI failed. It may have been do to adding

    --numprocesses=8 \
    --dist=worksteal \

to the pytest command for cudf.pandas tests.

I'll add this back in a follow-up PR.

@galipremsagar
Copy link
Contributor

/merge

@rapids-bot rapids-bot bot merged commit dbe5528 into rapidsai:branch-24.10 Sep 25, 2024
99 checks passed
@vyasr vyasr mentioned this pull request Nov 5, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mroeschke mroeschke mroeschke left review comments

@galipremsagar galipremsagar galipremsagar approved these changes

@wence- wence- Awaiting requested review from wence- wence- is a code owner automatically assigned from rapidsai/cudf-python-codeowners

@bdice bdice Awaiting requested review from bdice

Assignees

@Matt711 Matt711

Labels
2 - In Progress Currently a work in progress cudf.pandas Issues specific to cudf.pandas feature request New feature or request non-breaking Non-breaking change Python Affects Python cuDF API.
Projects
cuDF Python
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Matt711 @mroeschke @galipremsagar @bdice