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

Disable CI for draft PRs #847

Merged
merged 20 commits into from
Aug 23, 2024
Merged

Conversation

anthayes92
Copy link
Contributor

@anthayes92 anthayes92 commented Aug 12, 2024

Context:
Tooling Team has requested that all CI checks for draft PRs across all PennyLane repos are to be disabled to free up GH runner resources. SC Story.

Description of the Change:
Conditions for checking whether a PR is in draft state have been added to existing workflows.

Benefits:
Reduces GH runner usage.

Possible Drawbacks:
Some development steps are deferred to PRs in "ready for review" state only.

Related GitHub Issues:
N/A

Verification:

Created this PR as draft, only lightweight CI checks use GitHub runners (formatting, changelog reminder). When marked as "ready for review" the CI checks ran (excluding wheel builds and multi-gpu runners). When labels added for wheel builds and multi-gpu runners all CI checks ran.

Screenshot from 2024-08-12 15-26-32

Copy link
Contributor

Hello. You may have forgotten to update the changelog!
Please edit .github/CHANGELOG.md with:

  • A one-to-two sentence description of the change. You may include a small working example for new features.
  • A link back to this PR.
  • Your name (or GitHub username) in the contributors section.

@AmintorDusko
Copy link
Contributor

Can we have a label that could re-enable CIs for draft PRs?
We rely a lot on our CI/CD workflows.

@anthayes92 anthayes92 marked this pull request as ready for review August 12, 2024 19:27
@mlxd
Copy link
Member

mlxd commented Aug 12, 2024

Can we have a label that could re-enable CIs for draft PRs? We rely a lot on our CI/CD workflows.

This was a long-time coming request. We can enable the CI by going out of draft mode.
Given the burden the CI has right now, tests are to run in non-draft mode across the org by default.

@anthayes92 anthayes92 added ci:build_wheels Activate wheel building. and removed ci:build_wheels Activate wheel building. labels Aug 12, 2024
…nnyLaneAI/pennylane-lightning into disable-ci-for-pl-lightning-draft-prs
Copy link

codecov bot commented Aug 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.34%. Comparing base (dd5dca1) to head (b75cd06).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #847      +/-   ##
==========================================
+ Coverage   97.59%   98.34%   +0.75%     
==========================================
  Files         117      151      +34     
  Lines       18642    23636    +4994     
==========================================
+ Hits        18193    23244    +5051     
+ Misses        449      392      -57     

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

@anthayes92 anthayes92 added ci:build_wheels Activate wheel building. ci:use-multi-gpu-runner Enable usage of Multi-GPU runner for this Pull Request labels Aug 12, 2024
Copy link
Contributor

@Mandrenkov Mandrenkov left a comment

Choose a reason for hiding this comment

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

I think there might be a couple of cases where we need careful about operator precedence but otherwise this looks good. 👍

.github/workflows/wheel_macos_x86_64.yml Outdated Show resolved Hide resolved
.github/workflows/wheel_macos_arm64.yml Outdated Show resolved Hide resolved
.github/workflows/wheel_linux_x86_64_cuda.yml Outdated Show resolved Hide resolved
.github/workflows/wheel_linux_x86_64.yml Outdated Show resolved Hide resolved
.github/workflows/wheel_linux_ppc64le.yml Outdated Show resolved Hide resolved
.github/workflows/wheel_linux_aarch64.yml Outdated Show resolved Hide resolved
.github/workflows/tests_lgpumpi_python.yml Outdated Show resolved Hide resolved
.github/workflows/tests_lgpumpi_cpp.yml Outdated Show resolved Hide resolved
@anthayes92 anthayes92 marked this pull request as draft August 14, 2024 13:15
@anthayes92 anthayes92 marked this pull request as ready for review August 14, 2024 13:51
@anthayes92
Copy link
Contributor Author

Reran CI in "draft" and "ready for review" after adding @Mandrenkov suggestions on operator ordering, all works as expected.

Copy link
Contributor

@Mandrenkov Mandrenkov left a comment

Choose a reason for hiding this comment

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

Thanks @anthayes92, just want to confirm the new if conditions work correctly with push and workflow_call events before approving!

.github/workflows/tests_lgpumpi_cpp.yml Show resolved Hide resolved
.github/workflows/wheel_linux_x86_64_cuda.yml Outdated Show resolved Hide resolved
@anthayes92 anthayes92 marked this pull request as draft August 14, 2024 15:43
@anthayes92 anthayes92 marked this pull request as ready for review August 14, 2024 15:48
@AmintorDusko AmintorDusko requested a review from a team August 16, 2024 11:32
Copy link
Contributor

@vincentmr vincentmr left a comment

Choose a reason for hiding this comment

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

Just a style suggestion, but looks good to me. Thanks @anthayes92 !

.github/workflows/tests_lgpu_cpp.yml Show resolved Hide resolved
@mlxd
Copy link
Member

mlxd commented Aug 23, 2024

Hey @anthayes92
Is this good to be merged?

@mlxd mlxd requested a review from maliasadi August 23, 2024 13:57
@anthayes92
Copy link
Contributor Author

Hey @anthayes92 Is this good to be merged?

Similar reason as mentioned here, though since you have a higher stake in the lightning repo perhaps you'd like to make the call?

@Alex-Preciado
Copy link

Let's go ahead and merge this one and also the PennyLane PR (PennyLaneAI/pennylane#6093). We can add a label to run tests in draft mode in a follow-up PR.

@anthayes92 anthayes92 removed ci:use-multi-gpu-runner Enable usage of Multi-GPU runner for this Pull Request ci:build_wheels Activate wheel building. labels Aug 23, 2024
@anthayes92 anthayes92 marked this pull request as draft August 23, 2024 19:00
@anthayes92 anthayes92 marked this pull request as ready for review August 23, 2024 19:34
@anthayes92 anthayes92 merged commit 9755440 into master Aug 23, 2024
230 of 233 checks passed
@anthayes92 anthayes92 deleted the disable-ci-for-pl-lightning-draft-prs branch August 23, 2024 20:24
multiphaseCFD pushed a commit that referenced this pull request Sep 8, 2024
**Context:**
Tooling Team has requested that all CI checks for draft PRs across all
PennyLane repos are to be disabled to free up GH runner resources. [SC
Story](https://app.shortcut.com/xanaduai/story/66346/disable-ci-for-pl-draft-prs).

**Description of the Change:**
Conditions for checking whether a PR is in draft state have been added
to existing workflows.

**Benefits:**
Reduces GH runner usage.

**Possible Drawbacks:**
Some development steps are deferred to PRs in "ready for review" state
only.

**Related GitHub Issues:**
N/A

### Verification:
Created this PR as draft, only lightweight CI checks use GitHub runners
(formatting, changelog reminder). When marked as "ready for review" the
CI checks ran (excluding wheel builds and multi-gpu runners). When
labels added for wheel builds and multi-gpu runners all CI checks ran.

![Screenshot from 2024-08-12
15-26-32](https://github.com/user-attachments/assets/e78c24f8-8f31-46a0-91f6-eb2d1802694b)

---------

Co-authored-by: ringo-but-quantum <[email protected]>
Co-authored-by: Lee James O'Riordan <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants