-
Notifications
You must be signed in to change notification settings - Fork 133
Daily submodule bump automation #2252
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
base: main
Are you sure you want to change the base?
Conversation
ScottTodd
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's split this into two pull requests.
- Dependabot for most submodules
- The new script just for rocm-libraries
| allow: | ||
| - dependency-name: "half" | ||
| - dependency-name: "HIPIFY" | ||
| - dependency-name: "rccl" | ||
| - dependency-name: "rccl-tests" | ||
| - dependency-name: "rocprof-trace-decoder" | ||
| - dependency-name: "rocm-cmake" | ||
| - dependency-name: "rocm-systems" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should split these into separate groups per component, if dependabot allows it.
Specifically:
| component | projects | notes |
|---|---|---|
| comm-libs | rccl, rccl-tests | |
| systems (core, etc.) | rocm-systems | |
| base | half, rocm-cmake | |
| compiler | hipify | Remove from dependabot, updates are managed by the compiler team together with amd-llvm |
| profiler | rocprof-trace-decoder |
Please test if that is possible, perhaps with code like
# comm-libs
- package-ecosystem: "gitsubmodule"
directory: "/"
schedule:
interval: "daily"
target-branch: "main"
allow:
- dependency-name: "rccl"
- dependency-name: "rccl-tests"
# systems
- package-ecosystem: "gitsubmodule"
directory: "/"
schedule:
interval: "daily"
target-branch: "main"
allow:
- dependency-name: "rocm-systems"We could also start with just rocm-systems on its own, as that is the highest priority submodule in this list to keep continuously up to date.
| reviewers: | ||
| - "ScottTodd" | ||
| - "marbre" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fine to start as you have this now, but we should add a github team or some labels to help with review, rather than tag individual maintainers like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
//testing PR review// Agree on having this reviewers centralized. we can have two approach:
- move reviewers field outside to some CODEOWNERS.yaml and maintain it there in code.
- low hanging would be to have teams setup in github as suggested by @ScottTodd
| patterns: | ||
| - "*" | ||
| - package-ecosystem: "gitsubmodule" | ||
| directory: "/" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pre-commit reported trailing whitespace here: https://github.com/ROCm/TheRock/actions/runs/19575710699/job/56060310982?pr=2252#step:4:168
pre-commit hook(s) made changes.
If you are seeing this message in CI, reproduce locally with: `pre-commit run --all-files`.
To run `pre-commit` as part of git workflow, use `pre-commit install`.
All changes made by hooks:
diff --git a/.github/dependabot.yml b/.github/dependabot.yml
index dcef9f2..18354f0 100644
--- a/.github/dependabot.yml
+++ b/.github/dependabot.yml
@@ -11,7 +11,7 @@ updates:
patterns:
- "*"
- package-ecosystem: "gitsubmodule"
- directory: "/"
+ directory: "/"
schedule:
interval: "daily"
target-branch: "main"
Error: Process completed with exit code 1.
You can install pre-commit to fix such issues for you on each commit: https://github.com/ROCm/TheRock/blob/main/CONTRIBUTING.md#pre-commit-checks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per my other comment, can you move the changes here into a second PR?
| app-id: ${{ secrets.PULL_REQUEST_APP_ID }} | ||
| private-key: ${{ secrets.PULL_REQUEST_APP_KEY }} | ||
|
|
||
| - name: Create pull request |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you share a PR this created so we can inspect it? Did test here or on your fork somewhere? I don't see any notes in the PR description to that effect.
| DATE=$(LC_ALL=C date +%Y%m%d) | ||
| BRANCH_NAME=github-actions/bump-rocm-libs-$DATE | ||
| TITLE="rocm-libraries Submodule Bump ${DATE}" | ||
| BODY="- Updating rocm-libraries" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(For future work) We may want some extra text in the body, like
Update to https://github.com/ROCm/rocm-libraries/commit/d76850c823f237e689d9c22b0181c5c1d51099c0
We could have the https://github.com/ROCm/TheRock/blob/main/build_tools/bump_submodules.py script generate a commit message and then use that commit message as the PR title. See the code here:
TheRock/build_tools/bump_submodules.py
Lines 154 to 157 in 57e417d
| exec( | |
| ["git", "commit", "-a", "-m", "Bump submodules " + date], | |
| cwd=THEROCK_DIR, | |
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it will fix and create a PR today for all the notes mentioned
Motivation
This pr brings in dependabot automation to bump submodules on a daily cadence and github action to bump rocm-libraries on a daily cadence
Progress on #223