ci: update python versions in ci workflows#410
Conversation
| examples/envs/generalized_tmaze_demo.ipynb | ||
| examples/envs/graph_worlds_demo.ipynb | ||
| examples/envs/knapsack_demo.ipynb | ||
| examples/envs/tmaze_demo.ipynb |
There was a problem hiding this comment.
Moved to nightly because it seems to use a lot of memory, which means when we run it with both python versions concurrently the runner gets a sigterm kill signal from github 🙃
conorheins
left a comment
There was a problem hiding this comment.
nice, supporting 3.13 and 3.14 is a smart move
two things before we land this
1. can we not drop 3.11 and 3.12 entirely? right now the matrix goes from ["3.11", "3.12"] to ["3.13", "3.14"], so we'd only be testing the two newest versions. our requires-python is still >=3.10, and realistically most users are on 3.11/3.12 today given how fresh 3.14 is, so we'd have zero coverage for the bulk of the userbase. can we keep a baseline in the matrix, eg ["3.11", "3.13", "3.14"] (or at least ["3.11", "3.14"])? happy to eat the extra few mins of CI for that.
2. lets not move tmaze to nightly. i'd really like to keep the tmaze demo in PR-tier CI since its one of the canonical pymdp demos and catching regressions in it before merge (rather than next morning) is worth a lot. you've actually already diagnosed the real problem in the PR description: its the animation/rendering, not the active inference computations, that slows it down.
I think the cleanest fix is nbval's cell-skip tag. if you add nbval-skip to just the heavy rendering cells (the frames = [] / env.render(mode="rgb_array", ...) loop + mediapy.show_video(...) ones, ie cells ~11/19/27/31), nbval skips executing them in CI but they stay in the notebook for anyone reading it on the docs or github. the active inference cells still run and still get output-validated under nbval-lax, so the part we actually care about is still gated.
to add the tag, just edit the cell metadata directly:
"metadata": { "tags": ["nbval-skip"] }couple notes on nbval skip:
- doesnt interfere with the sanitize hook, skipped cells keep their saved outputs locally and the execution-count standardization step only touches output-bearing cells
- nothing in the repo uses
nbval-skipyet so this'd be the first, would be good to add a one-liner to the "Authoring Notes" intest/notebooks/README.mddocumenting the convention (something like "use nbval-skip for cells that are heavy to execute under CI but belong in the tutorial, eg animation loops") so the next developer or coding agent knows about it
then we can revert the ci_notebooks.txt / nightly_notebooks.txt change and tmaze stays in the non-nightly CI. the longer-term animation refactor you mentioned still sounds great for theh future as well
|
this all looks great. nbval-skip approach came out clean and i like that you documented it in the README in two spots, that'll save the next person some confusion. one totally optional nit, not blocking: you bumped the matrix in approving 👍 |
This change bumps our CI pipelines to support tests on python 3.13 and 3.14 as well
longer term I'd like to revisit the animation/visualisation parts of the notebook as this is the resource heavy part, rather than the active inference bit. for now we skip the relevant cells in CI only