Create the GitHub release automatically, not just the tag - #331
Conversation
Zenodo archives on the GitHub release, not on the tag, so a tag alone leaves the DOI pointing at whatever was last released by hand. v1.5.5 was created manually for the JOSS submission; without this, the next archived version would be whenever someone remembered to click the button, while PyPI moved ahead. The tag step now creates the release too, skipping if one already exists, so reruns and the existing v1.5.5 are safe. The job already declares contents: write, which is what gh release create needs; it just needs GH_TOKEN in the environment.
juaristi22
left a comment
juaristi22
left a comment
There was a problem hiding this comment.
Checked the preconditions and simulated the script; one structural fix needed.
What holds up. The workflow parses; the step carries GH_TOKEN, the job already has contents: write (what creating a release needs), and gh ships on the runner image (GitHub CLI 2.100.0 on ubuntu-latest). The step order from #326 is in place, PyPI upload first, and the v1.5.5 and v1.5.6 publish logs both show Tagging and [new tag], so tagging is proven in CI. A release created with the default token does not start workflows, but webhooks still fire and Zenodo listens on a webhook; the v1.5.5 record shows the integration is enabled. The fresh-tag path works: against a bare remote with a stub gh that records calls, a new tag produced the push, release view, then release create v1.5.6 --title v1.5.6 --generate-notes, and a failing release create turned the job red.
Blocking: the release step sits after the two early exits, so it only runs when the script also pushes a fresh tag. Whenever the tag already exists the script exits 0 before reaching it. Simulated:
- Tag on the remote, release missing:
Tag v1.5.6 already exists on the remote; nothing to do., exit 0, noghcall at all. - Rerun after a failed
release create: the tag is now on the remote, so the rerun exits 0 and the job goes green with no release. That is the silent failure #326 removed for tags, reintroduced for releases. - The three tags that already lack releases, v1.5.3, v1.5.4 and v1.5.6, can never be backfilled by this script.
The fix is structural rather than a flag: make the tag section an if/else that skips tagging when the tag exists instead of exiting, and always fall through to the release check, so the release check is reached on every run. I would also add --verify-tag to gh release create, so that if the tag is ever absent from the remote, gh aborts rather than creating a tag itself at the default branch head.
Optional. --generate-notes writes a commit list; the towncrier entry in CHANGELOG.md is the curated one, and --notes-file with that section would make the release body match it. Zenodo reads .zenodo.json either way. Worth stating in the PR body: with this in place every merge mints a release and therefore a Zenodo version, docs-only ones included, and backfilling the three missing releases by hand would mint three more.
Per review: the release step sat after the two early exits, so it ran only when the script also pushed a new tag. Whenever the tag already existed the script exited 0 before reaching it - which meant a rerun after a failed release create went green having done nothing, the same silent failure #326 removed for tags, reintroduced for releases. The tag section is now an if/else that skips tagging rather than exiting, so the release check is always reached. Simulated against a bare remote with a stub gh: a fresh tag pushes then creates; an existing tag with no release still creates; both present exits 0 after one release view; a failing create turns the job red. Adds --verify-tag, so gh aborts if the tag is absent from the remote rather than creating one itself at the default branch head. Release notes now come from the towncrier CHANGELOG section for the version, falling back to --generate-notes when that section is absent.
|
Fixed in The tag section is now an if/elif/else that skips tagging rather than exiting, so the release check is reached on every run. Re-simulated all four paths against a bare remote with a stub
Took the optional suggestion too: notes now come from the towncrier Both of your practical points are now in the PR body rather than only in the thread: every fragment-carrying merge will mint a release and so a Zenodo version, docs-only included, and the three tags without releases — v1.5.3, v1.5.4, v1.5.6 — will not be backfilled by this script. |
juaristi22
left a comment
juaristi22
left a comment
There was a problem hiding this comment.
Second pass on 5197d64. The restructure does what was asked, and I re-simulated every path against a bare remote with a stub gh recording its calls:
- Fresh tag: push,
release view, thenrelease create --verify-tag --notes-file -with the curated changelog section piped in. Exit 0. - Tag already present, locally or on the remote, release missing: skips tagging and creates the release. This is the path that produced no
ghcall at all before. - Both present: a single
release view, exit 0. - A failing
release createexits 1 on both the curated-notes and the--generate-notesbranches, so a rerun after a partial failure finishes the job rather than going green with nothing done. - Changelog section absent: falls back to
--generate-notes.
release_notes.py returns the right section for 1.5.6 on the real CHANGELOG.md, handles the last section in the file, and prints nothing for a missing version. gh accepts --notes-file - from stdin. The body now states the two practical consequences. CI is green.
One optional nit, not blocking: the "exists locally" branch skips the push, so a local-only tag would make --verify-tag abort. That state cannot occur in CI with fetch-depth: 0; checking the remote alone would remove it.
Approving.
gh release create --verify-tag reads the tag from the remote, so a tag that exists only locally must still be pushed; the old local-first branch skipped the push and made the release step abort.
|
Took the optional nit too, in Re-simulated all six paths against a throwaway bare remote with a stub Thank you for the two rounds on this one — the structural point in the first was the bug, and this second one would have bitten on any rerun where the tag had been created but not pushed. Merging. |
#330 merged, so MicroDataFrame.cov and corr now use the weights. Drops the warning box and folds both back into the weighted aggregation table, with the descriptions restored to frequency-weighted, and flips the assertion in test_documented_weight_behaviour_holds to the replicated sample. It also now asserts each frame cell equals the corresponding MicroSeries value, which is the property #330 introduced. The CI Lint failure was docformatter rather than ruff - make lint runs both, and the new _docs.py needed its docstrings rewrapped. Merges main in, so the branch carries #330 and #331.
* Add an API reference to the documentation The documentation had no API page, which is the largest gap a JOSS reviewer assessing documentation would find. Lists every weighted estimator and weight-handling method on both classes with its signature and a one-line description, taken from the docstrings, plus the estimator conventions that distinguish these from naive weighted equivalents. Registered in both docs/myst.yml and docs/_toc.yml, which are kept in step until the duplicate configs are reconciled. * Document the weighted estimators, and check coverage both ways Per review, the page listed the named estimators and the weight helpers but omitted the weighted aggregation methods and the weight-preserving overrides - sum, mean, median, quantile, var, std, cov, corr, rank, groupby, merge, drop and the rest. Those are the core of both things the page says it covers, and the README's feature list promises them. The coverage check only ran one way, from the page to the package, which is why it passed. It is now a test that runs in both directions, so a new public method that is not documented fails. Also adds docstrings to quintile_rank, quartile_rank and percentile_rank rather than hand-writing their description cells, and corrects the set_weights annotation from np.array, a function, to np.ndarray. The changelog fragment becomes .changed so this releases as a patch. * Correct three wrong descriptions on the API page MicroDataFrame.cov and corr do not use the weights - they call pandas on the plain frame - but the rows described them as frequency-weighted and sat under Weighted aggregation. Verified: with w=[1,1,1,5] the frame gives 3.1667, the unweighted pandas value, while the replicated sample and MicroSeries.cov both give 3.1786. They now carry a warning pointing at #327 and say plainly that they are unweighted. equals compares weights on both classes - both return equal_values and equal_weights - and the rows claimed the opposite. The set_weights row still rendered '<built-in function array>' because the page was not regenerated after the annotation fix, and quantile had the same artifact from an unfixed 'q: np.array'. Both annotations are now np.ndarray in source, in microseries.py and microdataframe.py, and the rows regenerated. Also marks values as an attribute rather than showing a call signature, skips the coverage test when docs/ is absent so the packaged tests pass, and drops an unused import and parameter. * Correct cumsum, and pin the hand-written claims in tests Audited every description on the page that was written by hand rather than taken from a docstring, since that is where all three errors found in review were. cumsum was the remaining one. It returns a plain pandas Series and warns that the weights have been applied and cannot be reused, so it is the one method in the weight-preserving section that does not preserve them. The page now says so. The rest hold: clip, round, sqrt, copy and astype carry weights through; repeat repeats them alongside the values; merge, reset_index and drop keep them aligned, and drop(index=) drops the matching weights; groupby returns the MicroSeriesGroupBy and MicroDataFrameGroupBy wrappers. Those checks are now a test rather than something I ran once, covering the claims no docstring enforces: the unweighted frame cov and corr, the weighted MicroSeries cov, equals comparing weights, cumsum dropping them, and repeat carrying them. * Regenerate the rows the source outran, and test that it cannot recur The three rank rows were still blank: they got docstrings in the first round but the page was never regenerated, the same miss as set_weights in the second. Patching individual rows by hand is what kept producing this, so the rows are now regenerated from the live signatures and docstrings, and two tests hold the page to the code. test_no_row_is_missing_its_description fails on any blank description cell. test_signatures_match_the_live_ones compares every rendered signature against inspect.signature, attributing each row to the class whose heading it falls under, since several names exist on both. The second test immediately found a real error: the MicroDataFrame set_weights row carried the MicroSeries signature, because the earlier regex fix matched both rows. Regenerated. * Generate the API page from a committed script, version-stably The script that produced docs/api.md was run by hand and lived outside the repo, which is why the source outran the page four times in review. It is now docs/build_api.py: the prose is held verbatim, the tables are generated from the live classes, and the hand-written descriptions that are not docstrings (cumsum, merge, reset_index, drop, equals, copy, groupby, clip, round, repeat, sqrt, and the two unweighted cov/corr rows) are an explicit override table so regeneration cannot lose them. str(inspect.signature(...)) was never safe to compare as text: pandas 2 renders a Series annotation as pandas.core.series.Series and pandas 3 renders it as pandas.Series, so a page generated under one fails CI on the other's jobs, and from Python 3.14 Optional[X] reprs as X | None. microdf/_docs.py renders signatures from parameter names, kinds and defaults, strips module qualifiers and puts every union into one form. The generator and the test both import it, so the page cannot disagree with the test, and the same committed page passes under pandas 2.3 and 3.0 and on Python 3.9 through 3.14. * Follow #330: cov and corr are weighted, and satisfy docformatter #330 merged, so MicroDataFrame.cov and corr now use the weights. Drops the warning box and folds both back into the weighted aggregation table, with the descriptions restored to frequency-weighted, and flips the assertion in test_documented_weight_behaviour_holds to the replicated sample. It also now asserts each frame cell equals the corresponding MicroSeries value, which is the property #330 introduced. The CI Lint failure was docformatter rather than ruff - make lint runs both, and the new _docs.py needed its docstrings rewrapped. Merges main in, so the branch carries #330 and #331.
2 participants
Follow-up to @juaristi22's observation on #329: the versioning workflow pushes a tag but never creates a GitHub release, and Zenodo archives on the release, not the tag.
v1.5.5 exists only because I created it by hand for the JOSS submission. Without this change, the next archived version is whenever someone remembers to click the button — so the DOI and the README badge silently go stale while PyPI moves ahead.
Change
publish-git-tag.shcreates the release after pushing the tag:gh release view v1.5.5succeeds against this repository today, so the existing manual release is skipped rather than conflicting.contents: write, which is whatgh release createneeds. It only neededGH_TOKENin the step environment.--generate-notesgives the commit-derived changelog; the repo's ownCHANGELOG.mdis built separately by towncrier.Worth a second pair of eyes
This is release infrastructure and the one step that cannot be tested before it runs, which is exactly the class of thing your CI-preconditions check caught on #326. The specific assumption I have not proved: that the default
GITHUB_TOKENcan create a release here.contents: writeshould cover it, but a release created byGITHUB_TOKENalso does not trigger further workflows, which is fine here — Zenodo listens on a webhook, not on a workflow.Update after review
The release step sat after the two early exits, so it only ran when the script also pushed a fresh tag. @juaristi22 is right that this reintroduced for releases exactly the silent failure #326 removed for tags: a rerun after a failed
release createwould find the tag already on the remote, exit 0, and go green having done nothing.The tag section is now an
if/elif/elsethat skips tagging rather than exiting, so the release check is reached on every run. Simulated against a bare remote with a stubghthat records its calls:ghcall at allrelease createfailsAlso added
--verify-tag, soghaborts if the tag is missing from the remote rather than creating one itself at the default branch head, and release notes now come from the towncrierCHANGELOG.mdsection for the version, falling back to--generate-noteswhen that section is absent.What this means in practice, which is worth stating plainly
CITATION.cffalways resolves to the newest, so the citation stays correct, but the version list will grow quickly.v1.5.3,v1.5.4andv1.5.6. This script only ever acts on the version inpyproject.toml, so it will not backfill them. Doing so by hand would mint three more Zenodo versions; leaving them is harmless, since the archived v1.5.5 is the one the JOSS submission cites.