Mpy size actions #197
Mpy size actions #197
Conversation
|
Nice! I have a couple of comments:
|
| @@ -54,6 +54,17 @@ jobs: | |||
| pre-commit run --all-files | |||
| - name: Build assets | |||
| run: circuitpython-build-bundles --filename_prefix {% raw %}${{ steps.repo-name.outputs.repo-name }}{% endraw -%} {% if cookiecutter.target_bundle == 'Community' %} --package_folder_prefix {% if cookiecutter.library_prefix -%} {{ cookiecutter.library_prefix | lower | replace(' ', '_') }}_ {%- else %}""{%- endif -%}{%- endif %} --library_location . | |||
| - name: Check Sizes | |||
| run: | | |||
| git clone https://github.com/FoamyGuy/CircuitPython_Memory_Tools.git | |||
There was a problem hiding this comment.
Should that move to https://github.com/circuitpython ?
There was a problem hiding this comment.
I do like that idea. I'll cookiecut a new one to get the relavent boilerplate stuff and move this code into it, then put it in the circuitpython org. Probably sometime tomorrow I can work on that.
There was a problem hiding this comment.
Repo now lives here: https://github.com/circuitpython/CircuitPython_Org_size_tools
There was a problem hiding this comment.
Still need to update this PR to point to the new one. Will make a new commit with that once I've got the conditions mentioned in the other comment worked out.
The comment task does have a condition that makes it run only for the "pull request" event. So a direct commit that hasn't been made into a PR yet won't trigger it. Individual commits within the PR do trigger it to comment for each of them if they are all pushed individually. What other conditions could we use to limit it? I think we want to see the changes caused by new commits so re-commenting each time allows that, although it could make the threads quite long if there are lots of commits.
We discussed a few options during weeds discussion in the meeting and consensus arrived to at that time was to have it comment instead of only output into the actions log. |
Gotchya, that makes sense. In that case I would love to see it comment only when a few things happen:
That would keep it from printing too much but doing it's job when needed. Those numbers above are just examples, we'd need to baseline them across different repos if we went that route. I also think have a small tool that people can run locally (similar to |
|
The latest commits move this to use the size_tools repo in the circuitpython org as well as add the conditions mentioned. The initial values were a bit of a guess and we may want to tweak them some before making it active. They're defined here: I have done the majority of my testing on a single library (SI1145) it's a bit over the 1500 byte size that I set the baseline value to, so it does get commented on that repo. Here is the latest comment: FoamyGuy/Adafruit_CircuitPython_SI1145#1 (comment) The comment format is unchanged, but there is now logic to create the output file based on the value of the conditions, and if no file is output then it won't make the comment. |
|
This is fantastic!!! |
| if: github.event_name == 'pull_request' && steps.check_sizes_file.outputs.files_exists == 'true' | ||
| uses: machine-learning-apps/pr-comment@master | ||
| env: | ||
| GITHUB_TOKEN: {% raw %}${{ secrets.GITHUB_TOKEN }}{% endraw %} |
There was a problem hiding this comment.
Latest commit removes the extra space here. This was copied from another similar usage of the token which has now been removed by #202
|
I really, really like this idea, I think it's worth rolling out and patching to the libraries. I can help with generating an adabot patch when it gets to that point! |
|
Feedback from discussion during "in the weeds" of the meeting on 9/19/22 raised the idea to make the "current size" values get pulled from This would make it print information relevant to each specific PR in cases where there are multiple PRs included within a single release cycle. I.e in this scenario:
With current logic the comment on PR 2 would indicate that size changed by 1.08k since it's referencing the released mpy file rather than the previous version of main which would have included PR 1. With proposed new logic, this would output +1k for PR 1 and would only try to comment +8 bytes on PR 2 which more accurately reflects the change specifically, based on the other existing logic it will decide if the change is "too small" to comment so may end up not posting depending on what those thresholds are configured to. I will add further commits with this change. |
|
The latest commits change this to use the methodology mentioned in the prior comment. It will now clone and build main branch to get the values to compare changed branch to, instead of using the published mpy files from the bundle. I've also added a "summary" section to the comment that shows differences for the measured values to make the size impact easier to see at a glance. An example of the latest version of the comment is available here: FoamyGuy/Adafruit_CircuitPython_SI1145#1 (comment) |
|
Do we think this is ready, or do we want to try this out on a few PRs? I think it's good and approve but also think it's worth additional reviewers because it'll affect the PR process for everyone passively moving forward. |
|
Looks like this should be opened as a new PR to https://github.com/adafruit/workflows-circuitpython-libs/blob/main/build/action.yml |
So should this be closed and opened as an issue on the repo linked above? |
Yes! I can transfer it. |
This change adds two actions task inside the generated library repo that will post a comment on PRs with information about mpy file sizes.
This is an example of the comments that it posts:
FoamyGuy/Adafruit_CircuitPython_SI1145#1 (comment)
Some outstanding things to consider:
uses:instead of launched via command line as it is now.stringsoutput into the actions output? Currently there are minimal prints that make it to actions, most things are in the comment rather than the actions output.measure_size.pyagainst more libraries. I've done one of each: package and module, but not much else. There may be library structures that confuse it.