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

Split pandas pytests to prepare for GPU vs CPU metrics reporting #16743

Closed
wants to merge 36 commits into from

Conversation

galipremsagar
Copy link
Contributor

@galipremsagar galipremsagar commented Sep 4, 2024

Description

This PR splits the pandas tests job into 3 groups using pytest-split. The purpose of this PR is to reduce the runtime and avoid hangs being faced in running all the tests in one job along with profiling that will detect and extract metrics of GPU vs CPU execution in: #16754

As part of splitting the tests into separate groups we need .test_durations file which is 25 MB, hence I've added this file using git lfs.

Checklist

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

@github-actions github-actions bot added Python Affects Python cuDF API. cudf.pandas Issues specific to cudf.pandas labels Sep 4, 2024
@galipremsagar galipremsagar added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Sep 4, 2024
@galipremsagar galipremsagar marked this pull request as ready for review September 9, 2024 12:43
@galipremsagar galipremsagar requested review from a team as code owners September 9, 2024 12:43
Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

There's no PR description or linked issue so I can't tell what you aim to accomplish in this. We should find a solution that does not require git LFS.

@@ -0,0 +1 @@
python/cudf/cudf/pandas/scripts/.test_durations filter=lfs diff=lfs merge=lfs -text
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this change intentional? Why? I strongly oppose introducing LFS requirements to this repository.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Responded here: #16743 (comment)

@galipremsagar
Copy link
Contributor Author

galipremsagar commented Sep 9, 2024

There's no PR description or linked issue so I can't tell what you aim to accomplish in this. We should find a solution that does not require git LFS.

@bdice I just updated the PR title. My apologies for hitting ready for review when it isn't. Now it is :)

The reason for using git LFS is the file size. I can move the file to s3 but this file will require future updates if the test suite changes drastically and I wanted to avoid having to ask ops to upload a file manually. Do you see any downsides for using GIT LFS ? I followed instructions from ops as mentioned here: https://docs.rapids.ai/resources/git/#large-files-and-git

@galipremsagar galipremsagar changed the title Split pandas pytests Split pandas pytests to prepare for GPU vs CPU metrics reporting Sep 9, 2024
Copy link
Contributor

@bdice bdice left a comment

Choose a reason for hiding this comment

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

Please remove pytest-split if we are using pytest-shard instead.

Please run all shards in one worker (serially) to avoid paying for the job setup cost on multiple GPU workers (scarce resources). Then we don't need any LFS, S3, or .gitattributes changes.

@galipremsagar
Copy link
Contributor Author

@bdice This PR is not needed anymore as #16739 has all the necessary changes needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cudf.pandas Issues specific to cudf.pandas improvement Improvement / enhancement to an existing function non-breaking Non-breaking change Python Affects Python cuDF API.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants