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

sc-66351 auto onboard large runenrs #774

Merged
merged 21 commits into from
Jun 21, 2024

Conversation

rashidnhm
Copy link
Collaborator

@rashidnhm rashidnhm commented 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:

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

Possible Drawbacks:

Related GitHub Issues:

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 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 58.71%. Comparing base (d9da016) to head (0efacfe).
Report is 93 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (d9da016) and HEAD (0efacfe). Click for more details.

HEAD has 3 uploads less than BASE
Flag BASE (d9da016) HEAD (0efacfe)
8 5
Additional details and impacted files
@@             Coverage Diff             @@
##           master     #774       +/-   ##
===========================================
- Coverage   92.73%   58.71%   -34.02%     
===========================================
  Files          92       17       -75     
  Lines       13777     1904    -11873     
===========================================
- Hits        12776     1118    -11658     
+ Misses       1001      786      -215     

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

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.

Could you please re-open your test PR and rebase it to this PR's branch?

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.

Thank you very much!

Fix issue with regex check and added additional echo statements to show progress
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.

LGTM! Don't forget to update the changelog:

* Release candidate branches are automatically switched over to using the new large GitHub runner pool.
  [(#769)](https://github.com/PennyLaneAI/pennylane-lightning/pull/769)
  [(#774)](https://github.com/PennyLaneAI/pennylane-lightning/pull/774)

.github/workflows/determine-workflow-runner.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.

Thanks @rashidnhm! 🙌

.github/CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: Ali Asadi <10773383+maliasadi@users.noreply.github.com>
@rashidnhm rashidnhm merged commit f097557 into master Jun 21, 2024
45 of 47 checks passed
@rashidnhm rashidnhm deleted the sc-66351-auto-onboard-large-runenrs branch June 21, 2024 19:59
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:**
albi3ro pushed a commit to PennyLaneAI/pennylane that referenced this pull request Oct 18, 2024
**Context:**

Currently the CI gets congested when large amounts of pull requests are
being updated simultaneously. This pull request gives PRs an escape
hatch and use large runners and use different queue to have CI jobs be
picked up.

**Description of the Change:**

This pull request adds two new features:
- Ability to add the `urgent` label to any pull request and switch it
over to large runners
- Automatic swap of rc branch to large runner
    - This assumes the rc branch is of the format `vX.Y.Z-rcN`

Large runners, albeit slightly more powerful than standard runners, can
be spawned at a much higher volume than standard runners ... this is
because we pay per minute for these runners vs being included on our
GitHub Plan.

If a PR needs CI run without waiting for a runner, **add the `urgent`
label to the pull request**.

Important Note:
- This only affect jobs that run on `pull_request` and use `ubuntu`
runners.
- This change is already in-place in lightning and catalyst.
    - PennyLaneAI/pennylane-lightning#774
    - PennyLaneAI/catalyst#846


**Benefits:**
Ability to leverage large runner to have quick time for a runner to pick
up a job.

**Possible Drawbacks:**
Though we dictate the pool size of large runners, it is possible to
still saturate it.

**Related GitHub Issues:**
None.
[sc-73711](https://app.shortcut.com/xanaduai/story/73711/update-pennylane-ci-to-use-large-runner-group)

---------

Co-authored-by: Mudit Pandey <mudit.pandey@xanadu.ai>
austingmhuang pushed a commit to PennyLaneAI/pennylane that referenced this pull request Oct 23, 2024
**Context:**

Currently the CI gets congested when large amounts of pull requests are
being updated simultaneously. This pull request gives PRs an escape
hatch and use large runners and use different queue to have CI jobs be
picked up.

**Description of the Change:**

This pull request adds two new features:
- Ability to add the `urgent` label to any pull request and switch it
over to large runners
- Automatic swap of rc branch to large runner
    - This assumes the rc branch is of the format `vX.Y.Z-rcN`

Large runners, albeit slightly more powerful than standard runners, can
be spawned at a much higher volume than standard runners ... this is
because we pay per minute for these runners vs being included on our
GitHub Plan.

If a PR needs CI run without waiting for a runner, **add the `urgent`
label to the pull request**.

Important Note:
- This only affect jobs that run on `pull_request` and use `ubuntu`
runners.
- This change is already in-place in lightning and catalyst.
    - PennyLaneAI/pennylane-lightning#774
    - PennyLaneAI/catalyst#846


**Benefits:**
Ability to leverage large runner to have quick time for a runner to pick
up a job.

**Possible Drawbacks:**
Though we dictate the pool size of large runners, it is possible to
still saturate it.

**Related GitHub Issues:**
None.
[sc-73711](https://app.shortcut.com/xanaduai/story/73711/update-pennylane-ci-to-use-large-runner-group)

---------

Co-authored-by: Mudit Pandey <mudit.pandey@xanadu.ai>
mudit2812 added a commit to PennyLaneAI/pennylane that referenced this pull request Nov 11, 2024
**Context:**

Currently the CI gets congested when large amounts of pull requests are
being updated simultaneously. This pull request gives PRs an escape
hatch and use large runners and use different queue to have CI jobs be
picked up.

**Description of the Change:**

This pull request adds two new features:
- Ability to add the `urgent` label to any pull request and switch it
over to large runners
- Automatic swap of rc branch to large runner
    - This assumes the rc branch is of the format `vX.Y.Z-rcN`

Large runners, albeit slightly more powerful than standard runners, can
be spawned at a much higher volume than standard runners ... this is
because we pay per minute for these runners vs being included on our
GitHub Plan.

If a PR needs CI run without waiting for a runner, **add the `urgent`
label to the pull request**.

Important Note:
- This only affect jobs that run on `pull_request` and use `ubuntu`
runners.
- This change is already in-place in lightning and catalyst.
    - PennyLaneAI/pennylane-lightning#774
    - PennyLaneAI/catalyst#846


**Benefits:**
Ability to leverage large runner to have quick time for a runner to pick
up a job.

**Possible Drawbacks:**
Though we dictate the pool size of large runners, it is possible to
still saturate it.

**Related GitHub Issues:**
None.
[sc-73711](https://app.shortcut.com/xanaduai/story/73711/update-pennylane-ci-to-use-large-runner-group)

---------

Co-authored-by: Mudit Pandey <mudit.pandey@xanadu.ai>
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.

5 participants