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

Update determine runner workflow to use large runners if the branch name matches a certain format #769

Merged
merged 14 commits into from
Jun 20, 2024

Conversation

rashidnhm
Copy link
Collaborator

@rashidnhm rashidnhm commented Jun 18, 2024

Context:
This PR is being opened under sc-66351. It introduces a new change to CI ahead of release where a local branch (not forked) that matches the regex pattern vX.Y.Z-rcN (The N after rc being optional) is auto ported over to the large runners.

Though this may provide a minor bump to the runtimes of the workflows themselves, the bigger objective (right now at least) is to not wait long times to get a runner.

Description of the Change:
The change is a minor one within the determine-runner workflow. It now checks for both a label present on a pr OR if the pr branch names matches the rc branching format above.

Benefits:
Faster queue times for rc branches.

Possible Drawbacks:
Though the large runners and standard runners should have parity in theory. We do not live in a perfect world. There is a none-zero chance that workflows might break. But we can test that out as part of this PR.

Related GitHub Issues:
None.

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.

Copy link

codecov bot commented Jun 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 59.34%. Comparing base (73bfee7) to head (bb75ba6).
Report is 92 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (73bfee7) and HEAD (bb75ba6). Click for more details.

HEAD has 56 uploads less than BASE
Flag BASE (73bfee7) HEAD (bb75ba6)
61 5
Additional details and impacted files
@@             Coverage Diff             @@
##           master     #769       +/-   ##
===========================================
- Coverage   92.80%   59.34%   -33.47%     
===========================================
  Files         126       17      -109     
  Lines       18385     1904    -16481     
===========================================
- Hits        17063     1130    -15933     
+ Misses       1322      774      -548     

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

@rashidnhm rashidnhm added the urgent Mark a pull request as high priority label Jun 19, 2024
@rashidnhm rashidnhm requested a review from mlxd June 19, 2024 16:37
@rashidnhm rashidnhm marked this pull request as ready for review June 19, 2024 16:37
@AmintorDusko
Copy link
Contributor

Hey @rashidnhm, how are we testing this change?

@rashidnhm
Copy link
Collaborator Author

Hey @rashidnhm, how are we testing this change?

Hey @AmintorDusko, we can test the large runners themselves by adding the urgent label to this PR.

To test the auto on-board, I have tested the expression locally against the past rc branch names, we can also create an rc branch name and test it (but I didn't want to do that right now to cause confusion 😅 )

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.

Thanks @rashidnhm!

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.

Thanks @rashidnhm . I'm trusting this works. Please don't forget to update the CHANGELOG with a description and your name, and then I'll approve.

@AmintorDusko
Copy link
Contributor

Thank you, @rashidnhm!

@rashidnhm
Copy link
Collaborator Author

Thanks @rashidnhm . I'm trusting this works. Please don't forget to update the CHANGELOG with a description and your name, and then I'll approve.

Thank you for that @vincentmr. If a bug does arise in the logic, we can definitely fall forwards and fix the issue. We also still retain the ability to port any PR to using large runners with the urgent label (this includes rc branches).

@rashidnhm rashidnhm requested a review from vincentmr June 19, 2024 18:13
Copy link
Contributor

@AmintorDusko AmintorDusko left a comment

Choose a reason for hiding this comment

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

Nice job! Only one more suggestion. Thanks!

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.

LGTM, thanks @rashidnhm .

@AmintorDusko AmintorDusko merged commit d9da016 into master Jun 20, 2024
46 of 47 checks passed
@AmintorDusko AmintorDusko deleted the sc-66351-auto-onboard-large-runenrs branch June 20, 2024 13:26
rashidnhm added a commit that referenced this pull request Jun 21, 2024
**Context:**
This PR fixes an issue introduced as part of the previous pr #769 .

This issue was identified while trying to replicate an RC branch and
testing the workflows as part of #773.

**Description of the Change:**
- Added extra echo statements to see clearly the outcome of the workflow
if logic
- Removes quotes around the regex expression as that was making all
evaluations of the logic to go false.
- This issue did not arise in my previous testing but turned out to be a
mismatch in shell configuration between myself locally and github
actions. I was able to reproduce the bug locally and see that this
worked.

#773 worked as expected with the changes introduced in this PR.

Links:
-
https://github.com/PennyLaneAI/pennylane-lightning/actions/runs/9616650952/job/26526641408?pr=773#step:2:14
-
https://github.com/PennyLaneAI/pennylane-lightning/actions/runs/9616650967/job/26526641639?pr=773#step:2:14

**Benefits:**
Auto on-boarding tested and working with this fix.

**Possible Drawbacks:**

**Related GitHub Issues:**

---------

Co-authored-by: ringo-but-quantum <[email protected]>
Co-authored-by: Vincent Michaud-Rioux <[email protected]>
Co-authored-by: Ali Asadi <[email protected]>
rashidnhm added a commit to PennyLaneAI/catalyst that referenced this pull request Jun 21, 2024
…ame matches the rc branching format (#846)

**Context:**
This PR introduces a change to the CI, where a PR that matches the
naming format of the RC branches (eg: `vX.Y.Z-rcN`) is automatically
on-boarded to use large runner queue *without the usage of a label*

**Description of the Change:**
This change has been introduced within pennylane-lightning with the
following two PRs:
- PennyLaneAI/pennylane-lightning#769
- PennyLaneAI/pennylane-lightning#774

**Benefits:**
Lower wait times for rc branch builds

**Possible Drawbacks:**
As noted in the lightning PRs, though the runners have parity, it is
possible for a workflow to fail on the larger runners. The `urgent`
label will be added to this PR to ensure jobs pass.

**Related GitHub Issues:**
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
urgent Mark a pull request as high priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants