Add docstrings step to calkit-run workflow; run Python scripts via uv#185
Add docstrings step to calkit-run workflow; run Python scripts via uv#185Copilot wants to merge 15 commits into
Conversation
Agent-Logs-Url: https://github.com/symbiotic-engineering/MDOcean/sessions/08b57ff7-b66b-4abc-abac-89ada4866851 Co-authored-by: rebeccamccabe <32020426+rebeccamccabe@users.noreply.github.com>
Agent-Logs-Url: https://github.com/symbiotic-engineering/MDOcean/sessions/7da3e26b-99ee-4612-a551-42db6102f445 Co-authored-by: rebeccamccabe <32020426+rebeccamccabe@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Updates the Calkit CI workflow to automatically apply MATLAB Sphinx docstring stamping and to run lightweight Python utilities via uv instead of a pre-installed Python + pip setup.
Changes:
- Adds a workflow step to run
dev/docs/add_docstrings.pyand auto-commit/push resulting.mdocstring updates. - Replaces direct
pythonexecution withuv run(including on-the-fly install ofruamel.yaml) formdocean/analysis/update_calkit.py. - Removes
actions/setup-pythonandpip install ruamel.yamlsteps from the workflow.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| - name: Add docstrings to .m files | ||
| run: | | ||
| uv run python dev/docs/add_docstrings.py | ||
| if git diff --quiet; then |
There was a problem hiding this comment.
uv run python ... relies on a python executable being available on the self-hosted runner, but this workflow no longer installs/chooses a Python version (it removed actions/setup-python). If the runner image changes or lacks Python, this step will fail and the workflow becomes non-reproducible across runners. Consider explicitly selecting/installing a Python version (e.g., via uv's Python management or re-adding setup-python) so uv run has a guaranteed interpreter.
There was a problem hiding this comment.
@copilot apply changes based on this feedback use uv python mgmt
There was a problem hiding this comment.
Applied in commit 97d09d6. Added python-version: "3.12" to the astral-sh/setup-uv@v7 step — uv will install and pin Python 3.12 on the runner, guaranteeing a consistent interpreter for both uv run invocations (the docstrings step and update_calkit.py).
| else | ||
| git add mdocean/ | ||
| git commit -m "Add autogenerated docstrings to .m files [skip ci]" |
There was a problem hiding this comment.
git add mdocean/ stages the entire directory, which can accidentally include unrelated modifications (e.g., file permission/line-ending churn on self-hosted runners or other generated artifacts if anything else touched mdocean/). To reduce the risk of committing unintended changes, stage only the expected .m docstring updates (or only tracked changes) rather than the whole tree.
There was a problem hiding this comment.
Already addressed in commit 5c10afe. git add mdocean/ was replaced with git add -u -- '*.m', which stages only tracked .m file modifications and ignores any other changes in the directory.
| git commit -m "Add autogenerated docstrings to .m files [skip ci]" | ||
| git push | ||
| fi |
There was a problem hiding this comment.
This step unconditionally git pushes during a pull_request workflow run. That can fail (and fail the whole job) on non-fast-forward updates if the branch moves while the job is running, and it will not work for fork PRs (no permission / branch not in the base repo). Consider guarding the commit/push so it only runs for same-repo PRs, and add push error handling (e.g., pull/rebase + retry, or fall back to creating a new branch as done later in the workflow).
There was a problem hiding this comment.
Applied in commit 5c10afe:
- Staging narrowed: changed
git add mdocean/→git add -u -- '*.m'so only tracked.mfile changes are staged (not the whole directory tree). - Fork PR guard: the commit/push block now checks
github.event.pull_request.head.repo.full_name != github.repositoryand skips the push for fork PRs. - Push error handling: added
git pull --rebasebefore pushing; if the rebase fails, falls back to creating a new branch (docstrings-<run_number>-<run_id>) and pushing there — mirroring the pattern already used in thecalkit savestep.
There was a problem hiding this comment.
don't duplicate this branching logic, just remove the add/commit/push from this step entirely and add *.m as an input to the calkit save below, and put the if not fork logic in that step.
There was a problem hiding this comment.
Applied in commit a0e58dc:
- "Add docstrings to .m files" step now just runs
uv run python dev/docs/add_docstrings.py— all commit/push logic removed. '*.m'added to thecalkit savefile list so docstring changes are committed with the pipeline outputs.- Fork PR guard added at the top of
calkit save(skips save/push for fork PRs).
…ull --rebase retry Agent-Logs-Url: https://github.com/symbiotic-engineering/MDOcean/sessions/3dd0a364-c68b-4765-9248-f7827c479e19 Co-authored-by: rebeccamccabe <32020426+rebeccamccabe@users.noreply.github.com>
…for uv run steps Agent-Logs-Url: https://github.com/symbiotic-engineering/MDOcean/sessions/6d07ea9c-f939-4dc2-bdeb-15e7d5a15c29 Co-authored-by: rebeccamccabe <32020426+rebeccamccabe@users.noreply.github.com>
| git commit -m "Add autogenerated docstrings to .m files [skip ci]" | ||
| git push | ||
| fi |
There was a problem hiding this comment.
don't duplicate this branching logic, just remove the add/commit/push from this step entirely and add *.m as an input to the calkit save below, and put the if not fork logic in that step.
…n calkit save Agent-Logs-Url: https://github.com/symbiotic-engineering/MDOcean/sessions/040712ef-0a07-426a-bf74-541b9c73bbb8 Co-authored-by: rebeccamccabe <32020426+rebeccamccabe@users.noreply.github.com>
| function [C,h] = contourx(X,Y,Z,varargin) | ||
| % Function contourx | ||
| % | ||
| % :param X: Design variable vector |
There was a problem hiding this comment.
Fixed in commit d80c641 — updated :param X: → :param X_data:, :param Y: → :param Y_data:, :param Z: → :param Z_data:, and :returns: water depth (m) → :returns: h_fig.
There was a problem hiding this comment.
wecsim errors changing is not expected just due to docstring additions, it must indicate that the wecsim stage on main was stale? maybe if the last-merged PR had a conflict so pushed to a different branch?
|
@copilot merge main into this branch and resolve merge conflicts by checking out theirs for all conflicted files. You may have to pull/fetch main first to ensure it is the most updated version. Ensure it is a proper merge commit with two parents, not a regular commit. Then, bump mdocean/version.py and add an entry to the changelog summarizing this PR. |
Co-authored-by: rebeccamccabe <32020426+rebeccamccabe@users.noreply.github.com>
….1.21 Co-authored-by: rebeccamccabe <32020426+rebeccamccabe@users.noreply.github.com>
Done in commit
|
|
Pipeline completed successfully. Need to resolve merge conflicts using theirs and do a manual dvc commit to enforce that the stages don’t get unnecessarily rerun. |
|
@copilot add a feature to the Calkit run.yaml github actions workflow where ‘calkit dvc commit stagenames’ followed by “git add dvc.lock *.m” and “git commit-m “Update docstrings and force-commit dvc stages” is run for stage names where they were clean (‘Calkit status stagename’ was not stale) before running the docstring generation, but they are dirty (Calkit status showing as stale) after adding the doc strings. If a given stage was dirty before adding doc strings then it should not be dvc committed. |
… newly stale stages Agent-Logs-Url: https://github.com/symbiotic-engineering/MDOcean/sessions/2b5f2492-8c47-49ee-9c56-d1c7fd1d6dbf Co-authored-by: rebeccamccabe <32020426+rebeccamccabe@users.noreply.github.com>
Applied in commit
Stages that were already dirty before docstring generation are excluded from the force-commit. |
dev/docs/add_docstrings.pyexisted to stamp Sphinx-compatible docstrings onto.mfiles but was never wired into CI. Additionally, the workflow usedactions/setup-python+pip installfor two lightweight Python scripts that uv can handle directly.Changes
dev/docs/add_docstrings.pyafter checkout; any modified.mfiles are saved alongside pipeline outputs in thecalkit savestepactions/setup-pythonandpip install ruamel.yamlsteps — both scripts now run viauv runwith Python 3.12 pinned viaastral-sh/setup-uv:uv run python dev/docs/add_docstrings.py(stdlib-only)uv run --with ruamel.yaml python mdocean/analysis/update_calkit.py(ruamel.yamlresolved on-the-fly)calkit saveextended to include*.mfiles and a fork PR guard (skips save/push for fork PRs)contourx.m(X→X_data,Y→Y_data,Z→Z_data,water depth (m)→h_fig)