[GitHub] Add basic CI for libclang Python binding unit tests #76784
Conversation
|
@llvm/pr-subscribers-github-workflow Author: Craig Hesling (linux4life798) ChangesThis is important to aid development on Python Type Annotations - #76664.
Issue #76601. Full diff: https://github.com/llvm/llvm-project/pull/76784.diff 1 Files Affected:
diff --git a/.github/workflows/libclang-python-tests.yml b/.github/workflows/libclang-python-tests.yml
new file mode 100644
index 00000000000000..da5bd696063c24
--- /dev/null
+++ b/.github/workflows/libclang-python-tests.yml
@@ -0,0 +1,41 @@
+name: Libclang Python Binding Tests
+
+permissions:
+ contents: read
+
+on:
+ workflow_dispatch:
+ push:
+ paths:
+ - 'clang/bindings/python/**'
+ - 'clang/tools/libclang/**'
+ - 'clang/CMakeList.txt'
+ - 'cmake/**'
+ - '.github/workflows/libclang-python-tests.yml'
+ - '.github/workflows/llvm-project-tests.yml'
+ pull_request:
+ paths:
+ - 'clang/bindings/python/**'
+ - 'clang/tools/libclang/**'
+ - 'clang/CMakeList.txt'
+ - 'cmake/**'
+ - '.github/workflows/libclang-python-tests.yml'
+ - '.github/workflows/llvm-project-tests.yml'
+
+concurrency:
+ # Skip intermediate builds: always.
+ # Cancel intermediate builds: only if it is a pull request build.
+ group: ${{ github.workflow }}-${{ github.ref }}
+ cancel-in-progress: ${{ startsWith(github.ref, 'refs/pull/') }}
+
+jobs:
+ check-clang-python:
+ # Build libclang and then run the libclang Python binding's unit tests.
+ name: Build and run Python unit tests
+ uses: ./.github/workflows/llvm-project-tests.yml
+ with:
+ build_target: check-clang-python
+ projects: clang
+ # There is an issue running on "windows-2019".
+ # See https://github.com/llvm/llvm-project/issues/76601#issuecomment-1873049082.
+ os_list: '["ubuntu-latest", "macOS-11"]'
|
|
A sample run of this action with a warm ccache can be seen at https://github.com/linux4life798/llvm-project/actions/runs/7393279920. |
|
@tbaederr and @AaronBallman, no rush, but I wanted to bring this to your attention. |
linux4life798
changed the title
linux4life798
changed the title
linux4life798
changed the title
There was a problem hiding this comment.
I'm not totally opposed to this change right now, especially since it will probably trigger pretty rarely, but we're building a lot of llvm/clang here. Caching will help, but Github's default caching rules are fairly restrictive on reuse.
We definitely want to have coverage for this, but we might not be ready yet. Ideally we'd be able to reuse all the clang build artifacts across multiple tests, but most of the CI is still stuck in buildkite. There also aren't any self hosted runners that have been migrated over yet.
linux4life798
force-pushed
the
add-python-workflow-checks-minimal
branch
from
98b6572
to
2bc9282
Compare
* Run on all pull requests and direct pushes.
* This makes use of the existing llvm-project-tests.yml
recipe, which will preload ccache from previous runs.
* Building libclang currently takes about 9mins when
ccache is warm and about an 1hr 20mins if it is cold
using the standard GitHub ubuntu runner.
* This could be broken into the following two steps:
- # Build libclang dependency.
ninja -C build libclang
- # Run Python unit tests.
ninja -C build check-clang-python
Issue llvm#76601.
Remove the check that only allows the action to run if it is running from the parent llvm organization repo. This is meant to protect contributors that fork the project from automatically launching actions with their account quota. This shouldn't be necessary, since GitHub Actions are disabled automatically on forks. The following docs says "When a public repository is forked, scheduled workflows are disabled by default.". https://docs.github.com/en/actions/using-workflows/disabling-and-enabling-a-workflow Issue llvm#76601.
linux4life798
force-pushed
the
add-python-workflow-checks-minimal
branch
from
2bc9282
to
8fd0cac
Compare
|
Sorry all, I'm still a little new to the PR review mechanics on GitHub. |
There was a problem hiding this comment.
No further issues on my end. We'll have to carefully monitor the cache usage, but this should be pretty similar to the SPIR-V precommit testing.
@tstellar should take a look at this before landing though.
|
|
||
| on: | ||
| workflow_dispatch: | ||
| push: |
There was a problem hiding this comment.
You forgot to add branches: - main here. This action now runs whenever I push a branch to my fork.
There was a problem hiding this comment.
Ah. That's why I was seeing the job run in places it shouldn't be. Fixed in c41472d. Thanks for pointing this out!
There was a problem hiding this comment.
Hmmm. What's the issue with the applicable test running in forks?
I intentionally didn't add these restrictions to allow for easier testing of changes in forks. I figured if you didn't like a particular workflow, then you just disable the specific workflow using the GitHub UI. No code modified. Plus, I believe all workflows are disabled by default for new forks (using a different mechanism), as I called out in 8fd0cac.
The alternative, where we disable them from within the yaml file, doesn't seem great to me. This means that you have to maintain additional commits/changes in every branch of your fork to re-enable this behavior. When you submit a PR, you have to remove these temporary commits. This discourages local CI testing.
Also, I think having pre-commit/PR testing is pretty important to ensure that the person making the change sees/corrects the failure, rather than maintainers seeing the failure after commit. The main branch tests are important to maintain a global cache (and of course to verify of the tree).
There was a problem hiding this comment.
@linux4life798 How do you disable specific workflows using the GitHub UI?
There was a problem hiding this comment.
@linux4life798 How do you disable specific workflows using the GitHub UI?
On the particular action/workflow you want to disable, you click the three dots in the top right and click Disable workflow:
After that, it will show a yellow banner warning that the workflow is disabled:
There was a problem hiding this comment.
I think for now we need to continue to restrict our workflows to the llvm organization or the llvm/llvm-project repository. I agree with you, though, that doing this makes testing harder and is inconvenient for forks that actually do want to test. On the other hand asking all the thousands of forks to manually disable the workflows doesn't seem like it scales well.
I don't know which solution is best, but we have had people complain in the past about unwanted workflows running in their forks. I'm not sure if they were aware of this option, though.
If we want to change our policy and remove the llvm org and llvm/llvm-project repo restrictions from our workflows, then I think we need an RFC to see how the rest of the community and the fork owners feel about this.
There was a problem hiding this comment.
Being automatically disabled is referring to scheduled workflows. This is not a scheduled workflow.
When the workflow is setup to also work on pushes anywhere, it runs on PRs it is not supposed to (at least from what I can tell).
It is more difficult to test in forks with these settings, but there are practical issues as mentioned above, and it's not really expected behavior. It's easy enough if you're working on the workflows to push another commit disabling some of the checks to make testing easier. I don't think making the tests opt-out by default on forks is a good idea.
There was a problem hiding this comment.
I don't know which solution is best, but we have had people complain in the past about unwanted workflows running in their forks. I'm not sure if they were aware of this option, though.
If we want to change our policy and remove the llvm org and llvm/llvm-project repo restrictions from our workflows, then I think we need an RFC to see how the rest of the community and the fork owners feel about this.
Fair. I have certainly been annoyed by failing action results in the past.
Being automatically disabled is referring to scheduled workflows. This is not a scheduled workflow.
Ohhh, I see that in the documentation, but that wasn't what I was referring to. I guess that is yet another automatic action disablement feature.
What I'm talking about seems to be weirdly undocumented. When you fork a repo with actions, you see the following message that forces you to opt-in to running actions/workflows:
Am I the only one who sees this? Try yourself by forking https://github.com/repo-sync/github-sync, which only has one on-push-tags action.
When the workflow is setup to also work on pushes anywhere, it runs on PRs it is not supposed to (at least from what I can tell).
Hmm. I didn't see any spurious Action runs in llvm proper. There are some confusing runs, like https://github.com/llvm/llvm-project/actions/runs/7445527409. When you dig into it, it looks like the rationale is that a parent commit did touch one of the filtered paths. If you push a bunch of commits, it won't run the action on each individual commit. It just runs the action on the last commit, but takes into account all paths touch since the last run. This is good.
It is more difficult to test in forks with these settings, but there are practical issues as mentioned above, and it's not really expected behavior. It's easy enough if you're working on the workflows to push another commit disabling some of the checks to make testing easier. I don't think making the tests opt-out by default on forks is a good idea.
For new users, you need to opt-in to enable actions. For existing users, you can disable all actions if you think they are annoying. Otherwise, it seems like a bonus add to be able to have you code automatically checked, assuming the rules for them to run are configured properly.
To be totally honest, I still have to manually disable all of the Actions I'm not interested in, since if: github.repository_owner == 'llvm' just results in the action being canceled. Seeing all the Canceled run item in the All workflows is still too annoying.
There was a problem hiding this comment.
What I'm talking about seems to be weirdly undocumented. When you fork a repo with actions, you see the following message that forces you to opt-in to running actions/workflows:
I do see that. I didn't realize that was a thing (must be pretty recent). Most LLVM forks are old however (at least for active contributors). We could get people to enable them and I guess active contributors would be the only ones seeing it and should be able to do so relatively easily, but it is another step.
Hmm. I didn't see any spurious Action runs in llvm proper. There are some confusing runs, like https://github.com/llvm/llvm-project/actions/runs/7445527409. When you dig into it, it looks like the rationale is that a parent commit did touch one of the filtered paths. If you push a bunch of commits, it won't run the action on each individual commit. It just runs the action on the last commit, but takes into account all paths touch since the last run. This is good.
The run that you mentioned is a PR (#77112) that has absolutely nothing to do with the libclang python bindings. I think this is normally an issue with merge commits, but there wasn't even one there in that case (probably due to the force-push). Sure, it just runs on the last commit, but it never should be running on that PR in the first place because it isn't relevant and potential noise to whoever is working on it. It's also just duplicate coverage as the last time main changed with the path-based filtering, the action should have already run. From what I can tell spurious runs like this stopped after I pushed the minor fix commit. We also need to be conservative with what we do run as we don't have that much compute.
To be totally honest, I still have to manually disable all of the Actions I'm not interested in, since if: github.repository_owner == 'llvm' just results in the action being canceled. Seeing all the Canceled run item in the All workflows is still too annoying.
Sure, but I think the main issue is when actions fail and then an automatic email notification is sent out. If the actions just cancelled, nothing gets sent and no compute is spent where someone probably doesn't want/need it.
This is also probably much better discussed on Discourse rather than buried in this merged PR.
) This is important to aid development of Python type annotations in the libclang binding. See llvm#76664 for more details. * Run on all pull requests and direct pushes. * This makes use of the existing llvm-project-tests.yml recipe, which will preload ccache from previous runs. * Building libclang currently takes about 9mins when ccache is warm and about an 1hr 20mins if it is cold using the standard GitHub ubuntu runner. * In the future, this could be broken into the following discrete steps for clarity: 1. Build libclang dependency. ninja -C build libclang 2. Run Python unit tests. ninja -C build check-clang-python * Followup changes will bring testing on older python versions and static type checking. Issue llvm#76601.
This is important to aid development of Python type annotations in the libclang binding.
See #76664 for more details.
ninja -C build libclang
ninja -C build check-clang-python
Issue #76601.