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

Run CI conditionally with paths #695

Merged
merged 38 commits into from
Apr 25, 2024
Merged

Conversation

vincentmr
Copy link
Contributor

@vincentmr vincentmr commented Apr 24, 2024

Before submitting

Please complete the following checklist when submitting a PR:

  • All new features must include a unit test.
    If you've fixed a bug or added code that should be tested, add a test to the
    tests directory!

  • All new functions and code must be clearly commented and documented.
    If you do make documentation changes, make sure that the docs build and
    render correctly by running make docs.

  • Ensure that the test suite passes, by running make test.

  • Add a new entry to the .github/CHANGELOG.md file, summarizing the
    change, and including a link back to the PR.

  • Ensure that code is properly formatted by running make format.

When all the above are checked, delete everything above the dashed
line and fill in the pull request template.


Context:
The CI is slow enough to be a real nuisance and productivity killer. This is making devs nervous, especially with freeze week coming.

Description of the Change:
Split tests into CPP and Python tests. CPP tests are run only when changes are made to the CPP source files. Python tests always run.

Test skipping can be seen in action in this PR, targeting the current one.

Benefits:
Run fewer tests which cannot be impacted by changes.
Faster CI runs in Lightning.
Increase GHA runners availability across the organization.

Possible Drawbacks:

Related GitHub Issues:
[sc-61920]

@vincentmr vincentmr added the ci:use-multi-gpu-runner Enable usage of Multi-GPU runner for this Pull Request label Apr 24, 2024
@vincentmr vincentmr requested a review from a team April 24, 2024 16:02
Copy link

codecov bot commented Apr 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.11%. Comparing base (6260d59) to head (cafc1d6).

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #695      +/-   ##
==========================================
+ Coverage   98.39%   99.11%   +0.72%     
==========================================
  Files         150      208      +58     
  Lines       18063    27589    +9526     
==========================================
+ Hits        17773    27346    +9573     
+ Misses        290      243      -47     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@vincentmr vincentmr marked this pull request as ready for review April 24, 2024 16:59
Copy link
Member

@mlxd mlxd left a comment

Choose a reason for hiding this comment

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

Thanks folks. No major concerns from me, but a few questions:

  • Will this require us to make the CPP tests as optional, rather than explicit for the CI checks?
  • How much better are the CI runs after this? From my experience, the CPP tests were never the bottleneck, but I could be wrong.
  • Do we have any any issues with coverage checks, since now we are cutting coverage on the overall ecosystem on a per-PR basis?
  • To follow no form the above, do we have issues uploading mutliple coverage files to codecov, or is the number the same as before?

.github/workflows/tests_lgpu_cpp.yml Outdated Show resolved Hide resolved
.github/workflows/tests_lkcuda_cpp.yml Show resolved Hide resolved
@vincentmr
Copy link
Contributor Author

vincentmr commented Apr 24, 2024

Thanks folks. No major concerns from me, but a few questions:

  • Will this require us to make the CPP tests as optional, rather than explicit for the CI checks?

I think so, unless we can also make this requirement dependent on the modifications.

  • How much better are the CI runs after this? From my experience, the CPP tests were never the bottleneck, but I could be wrong.

This is more true now that we have MCM tests, but GPU tests (and especially MPI tests) are still a bottleneck. This will increase availability so that Python tests can get started faster. This is also a first step to be followed by backend-filtered testing and the use of pytest-split. The latter change will especially benefit from skipping the C++ tests when possible.

  • Do we have any any issues with coverage checks, since now we are cutting coverage on the overall ecosystem on a per-PR basis?

Not clear from this PR, but I think nobody really trust CodeCov at this point. For instance, we rarely test MPI code so we have to look at the CodeCov report to really know which lines are meaningfully lacking coverage. Otherwise, I would expect the patch report to be alright.

  • To follow no form the above, do we have issues uploading mutliple coverage files to codecov, or is the number the same as before?

Same files as before.

@vincentmr
Copy link
Contributor Author

I can confirm CodeCov plays nice in 3c40f07.

@vincentmr vincentmr requested a review from mlxd April 24, 2024 20:08
Copy link
Member

@mlxd mlxd left a comment

Choose a reason for hiding this comment

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

No major blocker from me, thanks. I've a few quick comments left, but you can ignore these if no longer relevant.

.github/workflows/tests_without_binary.yml Outdated Show resolved Hide resolved
@vincentmr vincentmr added the ci:build_wheels Activate wheel building. label Apr 24, 2024
@mlxd
Copy link
Member

mlxd commented Apr 25, 2024

Since #664 will likely need an update or two to match this CI change, can that PR go in first?

@vincentmr
Copy link
Contributor Author

Since #664 will likely need an update or two to match this CI change, can that PR go in first?

Yes, it will be a good test. This might require a few adjustments.

Copy link
Member

@maliasadi maliasadi left a comment

Choose a reason for hiding this comment

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

Nice work @vincentmr 🥳

.github/workflows/tests_lkcuda_cpp.yml Outdated Show resolved Hide resolved
.github/workflows/tests_lkcuda_cpp.yml Show resolved Hide resolved
.github/workflows/tests_lkcuda_python.yml Outdated Show resolved Hide resolved
.github/workflows/tests_windows_cpp.yml Outdated Show resolved Hide resolved
Copy link
Member

@maliasadi maliasadi left a comment

Choose a reason for hiding this comment

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

🪂

@mlxd
Copy link
Member

mlxd commented Apr 25, 2024

I resolved the merge conflicts locally with the streaming ops. Everything now should adhere to this PR's design

@vincentmr vincentmr merged commit 8b131ca into master Apr 25, 2024
86 checks passed
@vincentmr vincentmr deleted the feature/ci_conditional_tests branch April 25, 2024 21:59
@vincentmr vincentmr mentioned this pull request Apr 26, 2024
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci:build_wheels Activate wheel building. ci:use-multi-gpu-runner Enable usage of Multi-GPU runner for this Pull Request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants