Automation to bump rocm-libraries submodule daily #2401
Conversation
|
We had some good results enabling auto merge (via the gh tool) on the bump PRs, so they would merge after CI checks are green. |
In which repository? Here in TheRock the only required check right now is pre-commit, so auto-merge won't wait for non-required CI checks. I would certainly like for auto-merge to be something we can safely enable if we can get our build/test/etc. times down to 30-60 minutes. |
|
#2448 Sample PR created by new changes |
| run: | | ||
| python3 ./build_tools/bump_submodules.py --push-branch \ | ||
| --branch-name ${{ steps.set-branch.outputs.branch-name }} \ | ||
| --components rocm-libraries" |
There was a problem hiding this comment.
Trailing " on the last line here? Remove?
There was a problem hiding this comment.
I was waiting on the final go ahead to fix all the pre-commit issues.
There was a problem hiding this comment.
The final go will be the approval but we cannot approve before this wasn't addressed thus not sure what you're looking for.
There was a problem hiding this comment.
There is a syntax error here. Did you test the workflow (on your fork)?
pre-commit is for formatting issues. You should install the pre-commit git hook or otherwise run pre-commit manually yourself. We should never see pre-commit failures on PRs if you have that properly configured.
See https://github.com/ROCm/TheRock/blob/main/CONTRIBUTING.md#pre-commit-checks
|
ammallya#2 New sample PR with commit message fixed to match title |
| run: | | ||
| python3 ./build_tools/bump_submodules.py --push-branch \ | ||
| --branch-name ${{ steps.set-branch.outputs.branch-name }} \ | ||
| --components rocm-libraries" |
There was a problem hiding this comment.
The final go will be the approval but we cannot approve before this wasn't addressed thus not sure what you're looking for.
|
|
||
| if __name__ == "__main__": | ||
| main(sys.argv[1:]) | ||
| main(sys.argv[1:]) |
| ["git", "checkout", "-b", args.branch_name], | ||
| cwd=THEROCK_DIR, | ||
| ) | ||
| component = args.components[0] |
There was a problem hiding this comment.
I am not confident this works. This only takes the first component but ignores if a user passes a list of components. While this is acceptable for the intended use case, it breaks the script when using it to bump more than one component as far as I see. Either this needs to be addressed properly or the changes for automation must go to it's own script.
| if not shutil.which("gh"): | ||
| raise SystemExit("ERROR: --create-pr requires the `gh` CLI.\n") |
There was a problem hiding this comment.
Eventually check earlier and before creating the commit and pushing the branch?
There was a problem hiding this comment.
Yes move that to argument parsing, like here:
TheRock/build_tools/github_actions/post_build_upload.py
Lines 346 to 357 in 83ec4f0
We can add an example like that to the style guide if it would help: https://github.com/ROCm/TheRock/blob/main/docs/development/style_guide.md#use-argparse-for-cli-flags
ScottTodd
left a comment
ScottTodd
left a comment
There was a problem hiding this comment.
Marking as "reviewed" while there are still open review threads (see the recent comments from Marius)
Motivation
Automate bumping of rocm-libraries PR daily
Progress on #223