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

maintain stable versions of dependencies #5158

Merged
merged 38 commits into from
Feb 13, 2024
Merged

maintain stable versions of dependencies #5158

merged 38 commits into from
Feb 13, 2024

Conversation

timmysilv
Copy link
Contributor

@timmysilv timmysilv commented Feb 5, 2024

Context:
There are many difficulties with dependency management, and this PR tries to address two of them:

  • reproducible versions of PL+deps from a past version
  • ability to view which deps have been auto-updated (eg. a new version of an unbound dep is released) and when

Description of the Change:

  • Factor out the python environment setup to its own file. All default package versions are here now
  • Update interface-unit-tests to trigger the upload of pip freeze results as artifacts for later use. happens in the core, jax, tf, torch, all-interfaces and external test suites today (ensured that it only occurs on one worker using strategy.job-index)
  • Add the upload-stable-deps workflow to run after tests complete. The action only runs on merge commits to master, so users shouldn't see it. This is the real new feature, and here's how it works:
    1. create the branch bot/stable-deps-update (or rebases it on master if it already existed)
    2. delete the existing requirement files in .github/stable/ to avoid merge conflicts
    3. download the previously-uploaded requirement files generated in this flow to that same folder and check if there's a diff
    4. if no diff, do nothing!
    5. if there's a diff and no PR exists, it opens one and tags me (and hopefully the team because of CODEOWNERS)
    6. if there's a diff and a PR already exists, it just pushes to the existing PR branch (confirmed this is the case despite the open-pr step running - it just does nothing in that case)

Benefits:
Helps with both difficulties mentioned in the context section above

Possible Drawbacks:

  • the team will have to review this PR (if it becomes too noisy, we can turn this into a cronjob)
  • commits made by bots (eg. actions that use ${{ secrets.GITHUB_TOKEN }}) will not trigger CI, so the automated PRs need to have it triggered manually. A hack to fix this is proposed here if we want to consider that, but this is a feature of GitHub (not a bug), intended to avoid infinite CI loops

I will do something similar for the docs builds after this PR is merged, it's complex enough for now.

[sc-56376]

Copy link
Contributor

github-actions bot commented Feb 5, 2024

Hello. You may have forgotten to update the changelog!
Please edit doc/releases/changelog-dev.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.

@codecov-commenter
Copy link

codecov-commenter commented Feb 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (d376a7f) 99.68% compared to head (1ac2c55) 99.68%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5158      +/-   ##
==========================================
- Coverage   99.68%   99.68%   -0.01%     
==========================================
  Files         396      396              
  Lines       36463    36191     -272     
==========================================
- Hits        36349    36076     -273     
- Misses        114      115       +1     

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

@mudit2812 mudit2812 self-requested a review February 7, 2024 16:42
@timmysilv timmysilv closed this Feb 7, 2024
@timmysilv timmysilv reopened this Feb 7, 2024
@timmysilv timmysilv marked this pull request as ready for review February 7, 2024 21:49
@timmysilv timmysilv mentioned this pull request Feb 7, 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.

Nice, looks good overall! 🎉

One thing to clarify is who is ultimately responsible for running the CI and merging in these PRs.

.github/CODEOWNERS Show resolved Hide resolved
.github/workflows/install_deps/action.yml Show resolved Hide resolved
.github/workflows/install_deps/action.yml Show resolved Hide resolved
.github/workflows/install_deps/action.yml Show resolved Hide resolved
.github/workflows/install_deps/action.yml Show resolved Hide resolved
.github/workflows/install_deps/action.yml Show resolved Hide resolved
.github/workflows/interface-unit-tests.yml Show resolved Hide resolved
.github/workflows/tests.yml Show resolved Hide resolved
.github/workflows/tests.yml Show resolved Hide resolved
.github/workflows/tests.yml Outdated Show resolved Hide resolved
Copy link
Contributor

@mudit2812 mudit2812 left a comment

Choose a reason for hiding this comment

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

Looks good to me! I left one comment that's blocking an approval from me. I've also been thinking about the frequency with which we merge to master, and whether we might be opening requirements update PRs too frequently? I guess we always have the option of cherry-picking the PRs and merging only a subset of them. But maybe we could also consider running this action on a schedule instead of being triggered by pushes to master. Not really a blocker from me, but worth discussing imo.

.github/workflows/install_deps/action.yml Show resolved Hide resolved
.github/workflows/interface-unit-tests.yml Show resolved Hide resolved
.github/workflows/tests.yml Show resolved Hide resolved
@timmysilv
Copy link
Contributor Author

@mudit2812 was your blocking question on the frequency with which it runs? If so, I have discussed it with both @albi3ro and @rashidnhm so I will sum up the main point from each of those chats:

  • if it happens frequently enough that it bothers us, we can reduce the frequency. that said, it'd be nice to start here and change it if necessary
  • it now pushes to the same branch every time, and if the PR is already open, it won't make a new PR. this means that if we only merge it in once a week, we'll only see at most one new PR per week, so it puts the PR frequency in our hands as reviewers

I have 2 main problems with the cron job:

  1. we'd have to manually update the sets of deps being installed for the cron job, separate from the tests. I can start re-writing the workflow files so this isn't the case, but as of today it will be an additional burden and potential source of inconsistency with what tests actually run
  2. the current way only pushes to the deps branch at the end of a successful CI run, so we know that any file pushed to that branch passed CI at one point. if it's on a cronjob, we lose that certainty

These problems are not as severe as us being swamped with notifications, so I would still be happy to switch to that if it becomes burdensome.

Copy link
Contributor

@mudit2812 mudit2812 left a comment

Choose a reason for hiding this comment

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

Thanks for providing the info. I'm happy with the conclusions. Nice work!

@timmysilv timmysilv enabled auto-merge (squash) February 13, 2024 21:08
@timmysilv timmysilv merged commit 1660959 into master Feb 13, 2024
36 checks passed
@timmysilv timmysilv deleted the pip-freeze-stable branch February 13, 2024 21:19
Gabriel-Bottrill pushed a commit to QSAR-UBC/pennylane_qutrit_mixed that referenced this pull request Feb 14, 2024
**Context:**
There are many difficulties with dependency management, and this PR
tries to address two of them:
- reproducible versions of PL+deps from a past version
- ability to view which deps have been auto-updated (eg. a new version
of an unbound dep is released) and when

**Description of the Change:**
- Factor out the python environment setup to its own file. All default
package versions are here now
- Update `interface-unit-tests` to trigger the upload of `pip freeze`
results as artifacts for later use. happens in the `core`, `jax`, `tf`,
`torch`, `all-interfaces` and `external` test suites today (ensured that
it only occurs on one worker using `strategy.job-index`)
- Add the `upload-stable-deps` workflow to run after tests complete. The
action only runs on merge commits to master, so users shouldn't see it.
This is the real new feature, and here's how it works:
1. create the branch `bot/stable-deps-update` (or rebases it on master
if it already existed)
2. delete the existing requirement files in `.github/stable/` to avoid
merge conflicts
3. download the previously-uploaded requirement files generated in this
flow to that same folder and check if there's a diff
  4. if no diff, do nothing!
5. if there's a diff and no PR exists, it opens one and tags me (and
hopefully the team because of CODEOWNERS)
6. if there's a diff and a PR already exists, it just pushes to the
existing PR branch (confirmed this is the case despite the open-pr step
running - it just does nothing in that case)

**Benefits:**
Helps with both difficulties mentioned in the context section above

**Possible Drawbacks:**
- the team will have to review this PR (if it becomes too noisy, we can
turn this into a cronjob)
- commits made by bots (eg. actions that use `${{ secrets.GITHUB_TOKEN
}}`) will not trigger CI, so the automated PRs need to have it triggered
manually. A hack to fix this is proposed
[here](peter-evans/create-pull-request#48 (comment))
if we want to consider that, but this is a feature of GitHub (not a
bug), intended to avoid infinite CI loops

I will do something similar for the docs builds after this PR is merged,
it's complex enough for now.

[sc-56376]
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