MicroPython
Improving the GitHub Actions experience for pull request contributors #18844
Replies: 8 comments 29 replies
-
|
For context, the proposition that spawned my remarks was to add the following text to the pull request template, under the Testing section:
While this is an ideal I wholeheartedly endorse, adding it as just community rule in the current situation is also this diff: int getRandomNumber()
{
-return 4;
+return 4; // returns 5
}Normalization of DevianceThe actual problem that MicroPython has here is normalization of deviance, due to the high rate of spurious CI failures in our Github Actions workflow for PRs. Test failures, and the corresponding overall ❌ badge, are a normal outcome from a CI run on even a bug-free pull request. Leaving this unaddressed is the actual expectation, and merging PRs with failed CI tests is what is currently practiced.1 I've personally missed bona-fide platform-dependent corner-case issues, even when attempting to conscienciously review the the exact CI test run outputs that should have revealed them to me. When In the instances I'm aware of, of course, another contributor was able to spot and point out what I missed; the holes in the swiss cheese didn't line up. And, though I personally enjoy pretending that it's both reasonable and possible to try to keep MicroPython flawless against even human error, it's worth acknowledging that that too is just a fantasy. (Wouldn't that feel good though? To imagine that what we do here could one day be the heroic detail that makes the difference between a near-miss and a tragedy, when some life-safety-critical device ends up just working?) But, even acknowledged as fiction, that's no reason to be complacent about this either. This is also a problem that contributes to the larger workload problem the maintainers here have --- and there are also very reasonable things we can do about it! Empowerment
In this instance, our normalized deviance isn't an understanding problem, it's an empowerment problem. The peter parker principle applies here in its converse: power is a prerequisite for responsibility. As a volunteer community, the rules we publish must truthfully reflect what the community's actual emergent expectations are. "Aspirational truth" can be a useful part of a community's rules, but doubling down on wishful thinking just broadens the culture gap between new and veteran contributors, and alienates consciencious members in favor of careless one who won't read them anyway. In order to improve our community culture and adopt the expectatation that a PR submitter take responsibility for fixing their failing tests, we need to grant community members the technical capacity to do so, and where possible eliminate spurious failure indications altogether. CI's Stochastic Failure ProblemMost current CI failures are stochastic failures --- that is, the CI indication isn't being confounded by individual tests in The large number of tests also represents a time cost --- waiting for a complete CI run to finish is easily long enough to break development flow, so by the time the PR they believe is correct loses its coin-flip, they'll have already moved on. Even if they have the technical know-how to just One solution to this, for anyone who's played Yahtzee, and for the project maintainers who actually can do this, is to rerun just the individual failed CI jobs from a PR, one at a time. This is both fast, and using the toy numbers above also has a much better 99%6 ≈ 94% chance of working, each time it's applied. The underlying "trigger CI job" technical permission is not without its own risk potential (in how it could be used to game a CI pass for a PR that actually does introduce a change with a low-percent chance of passing some particular test) --- but in my opinion, that's a risk worth exposing MicroPython to in favor of the risks that a renewedly-meaningful CI-pass indicator would help relieve. The Broader ContextMicroPython's maintainers currently struggle to keep up with the flood of PRs that the project recieves. This flood is only going to get worse as AI tools become even more prevalent. Currently, the CI pass/fail indication is not a usefully-reliable indication of code quality in either direction. It may never be possible to make the ✔️ indication a positive endorsement of quality (though more on that later in the Solutions section). If we're able to resolve and make it a reliable negative indication, though, that can also help relieve the wider workload problem --- so maintainers at least don't have to spend energy rejecting PRs that are indicatively not ready yet. Empowerment is also a solution I'd advocate for independent of specifically the CI issue. The maintainer workload problem is also a problem of scale, and often the easiest solution to a scale problem is to just increase delegation and actually scale the number of people able to work on it. Note that the real problem isn't the flood of bad PRs per-se, either --- it's the low signal-to-nose ratio that lets bad PRs bleed the time, attention, and goodwill away from the good PRs and the contributors that the community needs to function and grow. At present, we've got quite the high desertion rate (i.e. people doing something partway and never bothering to finish) --- this is the consequence when contributors stop feeling like finishing their contributions would have value. For the sake of illustrating what I mean about 'desertion', and in the hope that the attention I'm able to call at least to these individual situations outweighs what might be an excessive beating of the proverbial dead horse:
SolutionsAs a set of solutions I'd offer: Create a New "Reviewer" RoleEven if for no technical value beyond pure social recognition, even shiny medals do help when keeping a community motivated and slow the desertion rate. This is one of the fundamental things that made Stack Overflow as much of a success as it was in its heyday --- even if there's also lessons to learn from its downfall about clique formation and making seniority distinctions too procedural and rote. Grant Github Actions Permissions to More ContributorsIf more contributors are empowered with the technical permissions needed to trigger reruns of individual CI jobs, this will reduce the time and energy it costs to achieve a CI-pass on a correct PR, and help improve the signal-to-nose ratio.
Grant Close Permissions to More ContributorsA large number of the PRs and issues here could just be closed, as an even bigger signal-to-nose improvement. The long queue is more of a scale issue than anything. This would scale up both the number of contributors able to work through it, improve the efficiency of reviewers in finding PRs worth doing detailed technical reviews for, and help reduce desertion by giving these trusted contributors a larger social stake in the project. Advice over RulesDirect power often needs to be kept behind some barrier, but that doesn't mean it can't still be made more accessible to the community broadly: Instead of:
What if we could instead offer:
This conveys the same expectation, but much more strongly empowers the user to actually do something about meeting it. As a type of community gateway to some of the permissions that I'm advocating for above, this kind of thing can also help direct more newcomers to participate in a dialogue with veteran contributors --- as that dialogue is the fundamental difference between novice PRs we actually want to review, and AI-slop drive-by PRs that should just be closed. Change How CI Failure is EvaluatedAs a pure technical change, I'd add a step to CI that, when running on PR, checks the set of current test failures against a historical set from the last several commits of the upstream branch, and only mark global failure if there are new failing tests. I'd also add to that, the idea that a test-only PR should actually also get a ✔️, especially when it adds a new failing test. This could, if done well, even allow us to narrow the distinction between cpydiffs and other types of tests, and share more abstractions between them. Demote Low-Reliability TestsSystematically demote all of the perennial low-reliability CI thorns to some sort of new not-required-to-pass "advisory test" status, based on an appropriate statistical analysis of the rate of spurious failures. This would be a major workload challenge to do manually --- I'd know, I tried to take it on at one point with this series of commits:
(I'd not be opposed to doing something like this manually as a paid project, but as a volunteer I quickly stopped finding the idea reasonable with the amount of work involved.) I've also taken a stab at automating this concept, with AJMansfield/confusionbot. I've not been able to figure out how to infer usable confusion-matrix classifications from git topology (i.e. classifying a test failure as a 'false positive' if the failure was on a test that got merged, didn't produce anything useable). For you LLM wizards, if someone wants to contribute, this is about as responsible a use-case for LLM technology as it gets. If someone can figure out how to prompt an LLM with the PR's diff and get it to classify associated test failures on a confusion matrix, that could be quite the useful tool. Enforce Red-Green PR StructureAs the thing I mentioned earlier about potentially improving the positive reliability of a ✔️ indication too, I'd suggest it might be appropriate to have CI enforce a rule similar to what it enforces about commit messages, that require any changes to tests be in separate commits from any other type of change. I'm not the biggest fan of fully-dogmatic absolutist test-first test-driven-design, but it's an excellent way to structure a pull request for review, specifically because of how it makes sensitivity testing so much easier. When I'm reviewing, the standard of review I try to check for isn't just that the tests pass --- I also check if the tests correctly fail, when cherry-picked out and their associated features discarded or intentionally broken in some way. Point being, to mechanistically establish confidence that the test results do mean something beyond "this doesn't break everything". Enforcing full red-green at the CI level would be overly-dogmatic --- after all, coverage and regression tests also have value. But I would like if at least the recommendation to keep tests in separate commits were added to the contribution guidelines. I'd be happy to do a writeup on the fixup/autosquash workflow I use that makes this type of commit structure trivial to achieve. If it's not too radical, having this enforced by the commit message checker CI could even be an option too, but I'd want to see more adoption from regular contributors and an end to the current era of normalized deviance before adding rules that would only serve to retroactively deviate our norms further. So as a first initial step to supporting the adoption of that kind of workflow, I'd propose to start by just relaxing the commit-message checker to issue a weaker warning and fail as a ∅ run rather than a ❌ run, when a PR branch includes a commit with a Custom Git Trailers for Manual CI Control@agatti suggested an idea for some sort of PR metadata that could give a submitter some level of manual control over the relevant test set, which led me to elaborate out a few ideas for some more technical specifics. The git-trailers mechanism is a standard way to attach extra metadata to a commit. I'd propose that we could define a suitable set of custom trailer for our own use, and enhance the CI test process with the addition of a
I'd worry about discoverability here, but this could be a major time-saver when iterating on a bug that only occurs in a specific test environment the developer doesn't have their own setup for. This would also be a tool to help a developer hoping to apply red-green more dogmatically to unambiguously express their intent for a commit to result in a test failure when tested for sensitivity --- by adding a This has potential for abuse, of course --- but that's also something reviewers can check for more-or-less mechanistically. Footnotes
|
Beta Was this translation helpful? Give feedback.
-
|
Just made several substantive edits to this write-up, in response to @Josverl's and @agatti's comments --- about the permissions situation and the need for some kind of gateway to grant contributors the appropriate level of Actions access; about the cultural details that would need to be addressed in stages to roll out the Red-Green PR thing, and the new Git Trailers proposal. |
Beta Was this translation helpful? Give feedback.
-
|
Excellent write-up, @AJMansfield! However, I'm not totally sold on forcing having tests in separate commits in a PR :) For example, for simple changes that happen to add/change a test, it's extra work to craft a separate commit and come up with a meaningful commit subject and message for not much benefit. It's extra cognitive load/attrition for the writer (especially for new committers) and, in a way, for the reviewer too depending on the tool(s) they use to view the changes. This can be worked around with scripting, but then I believe you're not really improving things as far as the developer's experience goes. For large PRs [1], when following your proposal you'd probably end up with three options: have a single test-only commit at the end, multiple test-only commits that may group series of tests spanning more than one commit each, or a matching test-only commit for each code-changing commit. In the first two cases you may lose the tests' temporal/positional reference (as in, which test matches which commit), in the last one you increase cognitive load and add commit churn. Trying to have tests being part of the commit they're related to whenever possible and/or convenient could be a reasonable middle ground, also helping when performing [1] With "large" I mean more than three or four commits. I did some analysis a few months ago and back then the average PR contained only one or two commits once you take out the obvious outliers. There are also a couple more things that I think would be nice to have: Being able to add metadata in the PR message that let some tests to be skipped could help. For example, if I make a change in the RISC-V emitter that doesn't change anything outside Finally, the Unix/Coverage workflow will always fail on a local branch if you don't have the necessary Codecov API keys set up. It'd be nice if the coverage metrics upload step would be executed only if CI is running on |
Beta Was this translation helpful? Give feedback.
-
That's an important technical limitation, then --- one way or another, we would need to implement our own gateway to grant less-trusted contributors access to this ability. It also doesn't necessarily need to be an automatic gateway, either. IMO I'd almost prefer trying it first with a manual "ping |
Beta Was this translation helpful? Give feedback.
-
True, but from what I see this seems to apply only on
That could work, but assuming the earlier issue cannot be addressed for local forks, local exploratory commits could become a bit too involved. Maybe once some more data is gathered on which files affect which ports/boards/etc. then some common domains could be established. In the meantime I don't have a solution that would be perfect for all cases - the metadata in the PR message works on the main repo but not on local forks, having metadata in the commit message itself may cause issues if you have more than one domain set unless you refactor those as well (and won't work on local forks), same applies to things like git notes. Rewriting local history to clean things up before a PR could also be prone of leaving trailer artefacts.
Much better indeed! |
Beta Was this translation helpful? Give feedback.
-
I've added a new section to my write-up describing an elaboration of the idea in broader strokes, also with the addition of a
Nice thing about git-trailers is that git hooks can also edit them; we'd just need to configure an appropriate hook to strip them some time before they're pushed to master. I'm also of two minds here on whether or not it might actually be better to leave these trailer artifacts in the commit history, for what they actually do communicate about a developer's intent for a commit.
That's because this kind of filtering only happens on PR jobs. |
Beta Was this translation helpful? Give feedback.
-
That is exactly what I am talking about. I don't mean 'gateway' as in specifically CGI webserver technology, I mean the principle of a piece of software or administrative and procedural 'gateway', as its meant in its analogy to the gate of a city or fortification whose gatekeepers grant entry based on the entry rules they are meant to enforce. A gh app or workflow that checks for group membership to allow the user to rerun failed jobs is a gateway --- and so is my own alternate recommendation of having a reviewer role that contributors could ping in discord. |
Beta Was this translation helpful? Give feedback.
-
Edit I should have refreshed the page before adding this comment :) The suggestion to start having control over with CI jobs rather than tests still stands though.
Right, then maybe the filtering should be extended to also run on push rather than just on PR, if possible. |
Beta Was this translation helpful? Give feedback.
-
Board definitionsPerhaps slightly off topic as note pure CI , but related. I think we should have more clarity on :
|
Beta Was this translation helpful? Give feedback.
-
|
I think there are several that do or are willing to provide hardware if asked. But lets start by clearly documenting
That would be the basis of a PR review. Edit:
another option is to create a specific PR template for boards with the appropriate questions etc in the template , |
Beta Was this translation helpful? Give feedback.
-
Many board PRs also come from users who want the hardware they paid for to work. I've given up even thinking about trying to submit them. Do not throw more obstacles in the way. |
Beta Was this translation helpful? Give feedback.
-
|
Earlier tonight I was about to point someone to https://github.com/micropython/micropython-example-boards as an alternative and to reduce the board definition influx. [inspired by @mattytrentini] Just to be sure I tried to build the esp32 board there 😱
I think we can bring that up to date ( an eager agent is working on the first board )
|
Beta Was this translation helpful? Give feedback.
-
|
❓ What if there was a micropython/community_boards repo, ( updated fork of
that may help :
|
Beta Was this translation helpful? Give feedback.
-
Hmm, I think what I'm really getting at is that inclusion in That's a standard that would be completely impossible to meet, though, on every board that we ought to be trying to make MicroPython accessible on --- and you're right that we definitely also need a lower-barrier way to accept contributed board definitions.
I quite like this idea, though I think that the delay between a board PR and the next community-boards release would still be a source of frustration here --- I'd hope we'd be able to still offer them for download from micropython.org for the sake of making them available more expediently. Worth also considering how this can fit in with the recently-adopted definitions for different Port Tiers: Even just for the sake of seeding a transition like this, too, it would also be useful to go through all of the extant board definitions and identify at least one active or unmerged board from each port that can be demoted from micropython/micropython to micropython/community_boards. Probably not exactly the correct division, but as an attempt to put ideas to paper that are at least usefully-wrong, I'd offer: Community Board
Official Board
|
Beta Was this translation helpful? Give feedback.
-
use of standard test frameworksMy experience is mostly with mpremote testing , and IIRC the current bash based tests cover less than 23% of the code. Personally I have to testing most of my mpremote submissions using pytest , and have just omitted these tests from my PRs do avoid even longer merge times due to the additional complexity and framework conversations. So lets open that conversation;
I know that Alternatives:
|
Beta Was this translation helpful? Give feedback.
-
Yes , now that I have been able to resurrect my repo I have been trying to cherry-pick all relevant commits into a branch. |
Beta Was this translation helpful? Give feedback.
-
To be clear, do want to require pytest/unittest on the MCU, ( not bash either ) One example: |
Beta Was this translation helpful? Give feedback.
-
Oh I see, I thought you meant using mpremote to replace |
Beta Was this translation helpful? Give feedback.
-
|
While there may be some edge cases where pytest could improve testing MicroPython on Windows, I do not think rewriting the main test suite is worth the effort now. |
Beta Was this translation helpful? Give feedback.
-
|
True, but maybe it'd be possible to write an adapter for Pytest that can handle MicroPython's own test framework :) |
Beta Was this translation helpful? Give feedback.
-
|
A thing that I forgot to mention in my first post - it would also be nice to have a precommit hook/CI job that checks non-test Python code to make sure it is still compatible with the minimum supported Python version (which I think it's 3.8 right now). Vermin is supposed to do just that. I've had a go at coming up with a workflow for it, but I ended up getting stuck trying to work out how to get a list of changed files in a PR (I guess I'm not really a devops person ;)). If anyone's interested in picking this up and making it work for real, my attempt is available here (precommit hook) and here (workflow). |
Beta Was this translation helpful? Give feedback.
-
|
It would make sense in CI , not so much in pre-commit as there is no Windows version - so it would leak getting the changed files in CI should work something like this: |
Beta Was this translation helpful? Give feedback.
-
|
Thanks for the pointer, but my issue is related to GitHub itself and not with the git command line. I'm trying to not rely on third party actions to get that list (and it's strange that one has to end up doing that, to be honest) so I've been focusing on getting it to work within the realm of a single workflow. With this: echo "changed=$(git diff --name-only --diff-filter=ACMRT ${{ github.event.pull_request.base.sha || github.event.before }}...${{ github.event.pull_request.head.sha || github.event.after || github.event.sha }} | xargs)" >> $GITHUB_OUTPUTeven though the SHA hashes being substituted do exist on my repository and the rendered command line works fine locally, it croaks saying that it cannot handle the revisions range. But with this: if ${{ github.event_name == 'pull_request' }}; then
echo "changed=$(git diff --name-only -r HEAD^1 HEAD | xargs)" >> $GITHUB_OUTPUT
else
echo "changed=$(git diff --name-only ${{ github.event.before }} ${{ github.event.after }} | xargs)" >> $GITHUB_OUTPUT
fiit fails saying that one of the commit hashes is invalid. The rendered command line, again, works fine locally. Making the checkout non-shallow or increasing the checkout depth doesn't seem to work either, so I'm tempted to leave it up for somebody more knowledgeable to make it work and claim all the glory :) |
Beta Was this translation helpful? Give feedback.
-
Autocorrect on commit message formatting.I think that some
Also perhaps use either Action notifications , or create a custom action that can be called from any workflow to send a comment to the PR conversions with clearer instructions for contributors, as I think they often do not know how to fix these things. - name: Notify PR (always)
if: ${{ always() }}
uses: actions/github-script@v8
with:
github-token: ${{ secrets.GITHUB_TOKEN }}
script: |
const prNumber = context.issue.number || (context.payload.pull_request && context.payload.pull_request.number);
if (prNumber) {
await github.rest.issues.createComment({
owner: context.repo.owner,
repo: context.repo.repo,
issue_number: prNumber,
body: `Your commit message or signature is not correct . Please see https://foo.bar/helpme.html`
});
} |
Beta Was this translation helpful? Give feedback.
-
|
I am working on a common action Built using GHA-YAML + Python scripts Im trying to integrate it in a few more workflows to see what functionality can/should be moved to shared code. 
WIP Fork PR : Josverl#1 Responsible AI usage : Copilot + Jos |
Beta Was this translation helpful? Give feedback.
-
Handle PRs that are ignoring the template.responding to Angus' comment
Should be possible to :
|
Beta Was this translation helpful? Give feedback.
-
|
Agree this would be useful, with an additional gate of not applying if the PR is less than (say) 5-10 lines changed so we're not putting a bunch of unnecessary work onto trivial fixes. |
Beta Was this translation helpful? Give feedback.
-
|
Having a configurable minimum change threshold makes sense |
Beta Was this translation helpful? Give feedback.
-
|
On the topic of flaky tests, I had Claude Code crawl through the GitHub API to analyze all The job-level data is pretty conclusive — just 4 root-cause tests are responsible for essentially all of it:
GitHub only retains action logs for ~90 days so the test-level data is limited to 20 recent runs, but the job-level statistics are strong across the full 14 months and the correlation between job and test is 100% consistent in every run where both are known. The aggregate model (multiplying per-job pass rates independently) predicts 80.4% pass rate vs 82.1% observed, so the failures are well-characterised as independent events. I've written this up as a report with methodology, raw data, and clearly separated estimates here: The practical takeaway is that excluding or fixing these 4 tests would take the per-run failure rate from ~18% to near 0%. I've put together a PR that adds a retry mechanism for known-flaky tests to If the retry approach works out, it should also reduce the urgency around custom rerun infrastructure — most spurious failures would be absorbed automatically within the run itself. Manual reruns might still be useful down the track for infrastructure flakes or iterating on platform-specific bugs, but the known-flaky tests are the main driver right now. |
Beta Was this translation helpful? Give feedback.
-
|
Thanks @andrewleech, this is really useful info - I was planning to write a script to do something similar this week, so thanks for getting to it already. The results also match my own intuition of which tests are flaky (some of which have been on the TODO list to revisit for a while, I'm ashamed to say). Retrying and/or ignoring them seems like some obvious low-hanging fruit, so thanks also for the PR. |
Beta Was this translation helpful? Give feedback.
Uh oh!
There was an error while loading. Please reload this page.
-
Everyone who has opened a PR for MicroPython: what improvements would you like to see in the GitHub Actions checks in the PR workflow?
This is a spin-off from this comment by AJMansfield, who (correctly) identified the experience of MicroPython's GitHub Actions checks as a normalisation of deviance. Basically: GitHub Actions checks fail a lot, so people ignore the failures.
I have a lot of my own ideas that I'll write in a reply, but I want to throw this open first.
My only request is to please make constructive criticism or suggestions for improvements, rather than venting.
Beta Was this translation helpful? Give feedback.
All reactions