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

Prune workflows based on changed files #16642

Merged
merged 86 commits into from
Aug 27, 2024

Conversation

KyleFromNVIDIA
Copy link
Contributor

Description

Only run tests based on things that have actually changed. For example, if only Python files have changed, we don't need to run the C++ tests.

Contributes to rapidsai/build-planning#94

Checklist

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

Only run tests based on things that have actually changed. For
example, if only Python files have changed, we don't need to run
the C++ tests.

Contributes to rapidsai/build-planning#94
@KyleFromNVIDIA KyleFromNVIDIA requested a review from a team as a code owner August 22, 2024 14:46
@KyleFromNVIDIA KyleFromNVIDIA added improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Aug 22, 2024
@vyasr
Copy link
Contributor

vyasr commented Aug 22, 2024

@KyleFromNVIDIA do you want to make this a draft until you're done editing? I assume you have some more iterating to do.

@KyleFromNVIDIA KyleFromNVIDIA marked this pull request as draft August 22, 2024 15:58
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.

Almost any of the other directories (.devcontainer, .github, ci, conda, notebooks) or top-level files (codecov.yml, dependencies.yaml, pyproject.toml) can affect the behavior of C++ and/or Python tests. We also have a lot of possible cases where new files could affect the behavior of C++/Python tests.

I would propose a different strategy that is based on skipping tests when we know things are safe rather than allowing tests to run on only certain changes.

I'd propose this:

  • Run Python tests always unless changes were confined to the following paths (conjuction is OR):
    • docs/
    • java/
  • Run C++ tests always unless changes were confined to the following paths (conjunction is OR):
    • docs/
    • java/
    • notebooks/
    • python/

.github/workflows/pr.yaml Outdated Show resolved Hide resolved
@KyleFromNVIDIA KyleFromNVIDIA marked this pull request as ready for review August 23, 2024 14:23
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.

A few notes:

  • I think pandas-test-diff will be safely skipped if its dependency on pandas-tests is skipped. That's the only case where we have added an if changed condition to a job that has downstream dependencies, so just wanted to make sure that seems safe to you too.
  • We could be more aggressive in the future with Python job dependencies. For example, changes in cudf or dask-cudf shouldn't trigger cudf-polars tests, and changes in cudf-polars shouldn't trigger cudf or pandas tests. However I think we are doing enough in this PR to make a dent in CI usage and I don't want to add significantly more complication at once.

.github/workflows/pr.yaml Outdated Show resolved Hide resolved
@KyleFromNVIDIA
Copy link
Contributor Author

https://github.com/rapidsai/cudf/actions/runs/10527602668?pr=16642 has passed with all of the tests jobs being skipped. pr-builder has run successfully. I'm now going to try deliberately inducing a failing test.

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@KyleFromNVIDIA
Copy link
Contributor Author

The notebook test didn't fail because I happened to pick the one notebook that wasn't actually being run.

However, devcontainers happened to fail due to the recent merge of rapidsai/devcontainers#271 (and this PR not having #15483), so we were able to get a failed pr-builder anyway.

I'm happy with the state of this PR. I'll remove the temporary modifications and test it one more time.

Copy link
Contributor

@vyasr vyasr left a comment

Choose a reason for hiding this comment

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

This looks great. I agree with using negative rather than positive filtering. We should be conservative to make sure we don't lose test coverage. We can always filter more aggressively over time.

.github/workflows/pr.yaml Show resolved Hide resolved
.github/workflows/pr.yaml Show resolved Hide resolved
@KyleFromNVIDIA
Copy link
Contributor Author

/merge

@rapids-bot rapids-bot bot merged commit dd585e8 into rapidsai:branch-24.10 Aug 27, 2024
86 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team improvement Improvement / enhancement to an existing function non-breaking Non-breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants