Prototype memory check

[tekktrik]

Here's my prototype for a import memory usage analysis GitHub Actions workflow.

I'm trying to put all my thoughts down on here, so apologies for the wall of text that follows:

Important Considerations Upfront

Dependency Risk

Perhaps technically the most "impactful" is that I currently manage this action and the few critical ones it depends on. I am happy to continue owning them or transfer them over to the adafruit organization. I intend to support this like I do tekktrik/issue-labeled-ping but I also recognize that it is technically a dependency risk and would take a patch to convert the libraries after the fact to an Adafruit fork at that time. My vote would be to continue owning and supporting them personally for ease of development and simplicity in initial deployment, and if there comes a time to fork or transfer them then that's all good.

Artifacts vs. Comments

This action does not post a comment to the PR but rather uses workflow artifacts. This is done because PRs from public forks do not have write access permissions and therefore the workflow cannot post a comment. Instead, important files are uploaded to the workflow as artifacts (and in many cases are reused by a dependent workflow anyway). The downside is that it is up to reviewers to check the artifacts if they are concerned about memory, but the upside is that it will not clutter the discussion. If comment posting is desired/required, the way I would recommend would be to have a secondary, dependent workflow in the repositories that has elevated permissions to post the results of the analysis. This separates the running code and the posting of a simple text comment into appropriate levels of permissions, reducing the likelihood of malicious activity.

New Workflow File

Because this action only looks at the difference in memory used between some point and the latest commit in the main branch, it only makes sense for it to be triggered on the pull_request event. This means it requires its own workflow flow, and not added to the typical build workflow (which runs on both pull_request and push.

Artifacts are Temporary

Artifacts are only stored for 90 days. Logs are not indefinite either, but it does mean that results may be lost if a PR is stale. I personally didn't see this as a major blocker, but worth mentioning. Re-pushing the commit will re-trigger a workflow anyway.

Pinning Action Versions

Similar to the other workflows, actions here are pinned to main so that they receive updates automatically. Workflows used in tekktrik/circuitpython-memory-check are, however, pinned to specific versions to ensure the all work together as intended. This matches how the other composite actions work.

Dummy Changes

I added dummy changes to the actual library as part of this PR in order to trigger some example changes.

---

I'll add another comment for how the workflow actual works next.

[tekktrik]

General Order of Operations

1. Build the simulator firmware
   2a. Check the import memory usage for the NEW changes
   2b. Check the import memory usage for the PREVIOUS codebase
2. Perform the memory comparison analysis

Building Simulator Firmware

Steps

• Setup Python and system dependencies for the simulator (tekktrik/simulate-circuitpython-nativesim/prepare-system)
• Build the latest CircuitPython firmware either from scratch or retrieve from cache to improve speed (tekktrik/simulate-circuitpython-nativesim/build-firmware)
• Upload the firmware as a workflow artifact

Notes

• Building the firmware can take 10-15 minutes, so it is cached after being built so that it can be retrieved in a matter of seconds in later runs. This saves an immense amount of time, but means that caching is dependent on how frequently a repository actually uses this action, and most repositories will end up rebuilding the CircuitPython firmware for most runs.
• The build-firmware action defaults to building/using the latest CircuitPython version internally. This means that PRs addressing breaking changes are likely to have issues because the next steps will use a mismatched version. I deemed this to be low risk because these sorts of changes don't often happen, these PRs are not where this action is most useful, and those changes almost certainly need to occur regardless of memory imports.

Checking Memory Usage

Steps

• Setup Python and system dependencies (tekktrik/simulate-circuitpython-nativesim/prepare-system)
• Checkout the repository (here, adafruit/Adafruit_CircuitPython_ImageLoad)
• Download the firmware artifact
• Find all imports for the library
• Prepare a mock CIRCUITPY drive by copying this library and and installing dependencies using circup
• Install the simulator library and run it via scripts/check_memory.py
• Upload the memory usage (JSON file) as a workflow artifact

Notes

• The import performed and analyzed is a simple import (e.g., import adafruit_imageload). This should import all of the items as opposed to a more targeted import (e.g., from adafruit_imageload import XYZ). This should cast the widest net, but I'm also sure that imports are not necessarily additive (e.g., import A; import A.B; is often not as much as the addition of the analyses for import A and import A.B)
• This action runs twice: once for the new changes to the library, and the other for the main branch at time of run. That means simultaneous PRs are not accounted for (e.g., PR A and PR B are both open at the same time, but PR A is merged first - the results gathered for PR B are now out of date). This didn't strike me as a huge issue, and re-pushing the commit or re-running the workflow will fix this issue if needed.
• These steps also occur in parallel since they only depend on using the built CircuitPython simulator firmware.

Memory Comparison

Steps

• Setup Python
• Download both memory checks from the previous steps
• Run the analysis script (scripts/compare_memory.py)
• Post the analysis (memory_analysis.txt) as a workflow artifact

Notes

• The analysis attempts to track memory changes for added and modified imports, restating import sizes for unchanged imports, and announcing imports ignored due to errors (e.g., the import relies on an unavailable dependency or prints other warning text to the console like it does in adafruit/Adafruit_CircuitPython_ImageLoad).
• For modified imports, it additionally displays the growth of the library as the delta as well as the percentage.
• Currently, because the script does not post to comments, I decided not to implement thresholds for warnings (such as a memory threshold for small boards). This can still be done however, and I would just want some pointers for what warnings would be good to add (e.g., 24KB import size would trigger a SMALL_BOARD_WARNING).

[tekktrik]

You can check out the workflow artifacts in the actions below!

[tekktrik]

If it's helpful, here are all the dependent workflows:

Main Action

• tekktrik/circuitpython-memory-check: Main action that handles building the simulator firmware, perform the import analyses, and comparing changes

Dependencies

• tekktrik/simulate-circuitpython-nativesim: Handles preparing the runner for the CircuitPython simulator and actually building it (or restoring from cache)
• simulate-circuitpython-nativesim: A PyPI package from that allows the flash preparation and simulations steps to be performed via Python OR shell scripts - mostly used to enable better automation. There are actually actions for these steps as well, but the automation is easier as a Python library for the memory check
• tekktrik/latest-circuitpython-version: Uses gh to determine the latest CircuitPython release

[tekktrik]

One change that would be incredible here is if nativesim was stored in the AWS bucket along with the rest of the firmware each time a new release is created in the circuitpython repository - that would free up the cache space, since each firmware.exe is about 2.5 MB, so many runs over many repositories will add up and possibly push close to the 10 TB cache limit the Adafruit org has.

[tannewt]

One change that would be incredible here is if nativesim was stored in the AWS bucket along with the rest of the firmware each time a new release is created in the circuitpython repository - that would free up the cache space, since each firmware.exe is about 2.5 MB, so many runs over many repositories will add up and possibly push close to the 10 TB cache limit the Adafruit org has.

It is, except not the exe. We'd need to add that as an extension. https://adafruit-circuit-python.s3.amazonaws.com/index.html?prefix=bin/native_native_sim/en_US/

[tannewt]

@jwcooper Would it be possible to set up IO or grafana to log library size over time? (I don't know what IO's history policy is.)

[jwcooper]

@tannewt IO will store data for 30 or 60 days depending on usage. I'm not sure what kind of timeline would be required for this.

We have an internal grafana, but I'm not sure if we can open it up for this.

I only skimmed, but could you just log to a file locally, and periodically generate charts with python?

[tekktrik]

It is, except not the exe. We'd need to add that as an extension. https://adafruit-circuit-python.s3.amazonaws.com/index.html?prefix=bin/native_native_sim/en_US/

@tannewt Okay, that makes sense. Happy to help with that effort as far as CI work goes.

I only skimmed, but could you just log to a file locally, and periodically generate charts with python?

@jwcooper I could set up a central GitHub repository, or a file in the bundle repo for this, but that might make historical graphing slightly convoluted. I have a couple ideas bouncing around my head though in this same vein that I think might work, at least in the immediate term, like keeping a sqllite database in another repo updated via CI and some Pythons scripts for generating charts like you say.

[tannewt]

@tannewt IO will store data for 30 or 60 days depending on usage. I'm not sure what kind of timeline would be required for this.

I'd love to have all historical data for library size.

We have an internal grafana, but I'm not sure if we can open it up for this.

I definitely wouldn't open an internal one up.

I only skimmed, but could you just log to a file locally, and periodically generate charts with python?

How costly are small files on S3? We already upload artifacts there. We could have a one off batch process merge them together from time to time.

[tekktrik]

Oh! I forgot to mention - because I own simulate-circuitpython-nativesim on PyPI, it is complete fair (and my intention) to use an associated table in pyproject.toml to house settings for any actions relying on it. As an arbitrary example, we could do something like the following:

[tool.simulate-circuitpyton.circuitpython-memory-check]
skip-action = false
ignore-libraries = [
    "adafruit_xyz.some_submodule",
    "adafruit_xyz.other_submodule",
]

The action is then free to dynamically react to each repository when the action runs.

[jwcooper]

How costly are small files on S3? We already upload artifacts there. We could have a one off batch process merge them together from time to time.

Pretty negligible costs for something like this, I'd think. Would work pretty well, especially since there is already infrastructure setup for this.

[tekktrik]

Note to self (and others): I made some small modifications to the underlying actions, so I force pushed the same commit to re-run the actions as another check

[tannewt]

@tekktrik what do you think about uploading results to S3? (next to the CP builds).

[tekktrik]

@tannewt that would be amazing - I can create a new action called something like download-firmware that downloads it instead of rebuilding it here (though that functionality is nice to have if someone wanted to work off of a fork).

[tekktrik]

I can PR the work needed for this within the coming days to the CircuitPython repo.

[tannewt]

Sounds good. I think you'll need to change the toml file to list the exe extension to get it uploaded.

[tekktrik]

Alright, I'll get started on using the cached versions of the executable. Using "latest" defaults to the latest released tag, so the I'll need to wait for 10.1.5 to be cut before there's an available firmware. :)