Skip to content

Packaging + release CD: PyPI trusted publishing, GitHub Releases, version-coupled HF weights#133

Open
zachtheyek wants to merge 5 commits into
feature/huggingface-integrationfrom
feature/release-engineering
Open

Packaging + release CD: PyPI trusted publishing, GitHub Releases, version-coupled HF weights#133
zachtheyek wants to merge 5 commits into
feature/huggingface-integrationfrom
feature/release-engineering

Conversation

@zachtheyek

Copy link
Copy Markdown
Owner

Closes #132

Stacked PR: based on feature/huggingface-integration (#130) — it extends the revision-resolution chain that PR introduces. Only the commits past #130's tip belong to this PR; merge #130 first, then rebase this onto master.

Implements packaging + release CD per the release contract in docs/RELEASE.md (on #131's branch): one version string couples the git tag, the GitHub Release, the PyPI release, and the HF weights tag — with CD enforcing the coupling by verification, never by creating weights.

What's here

1. Build system (pyproject.toml)

  • Hatchling finalized ([build-system] uncommented, wheel target src/aetherscan); both stale TODO headers resolved.
  • [project].dependencies filled from requirements-container.txt's ranges plus the packages the NGC base image provides implicitly (tensorflow[and-cuda]==2.17.*, h5py>=3.11,<3.12, hdf5plugin, psutil>=6.0,<6.1 — ranges mirror environment.yml). requires-python = ">=3.10,<3.13" kept.
  • dev extra = ruff, pre-commit, pytest.
  • Development Status classifier: left at 2 - Pre-Alpha with a clearly-marked TODO knob for the v1 Beta-vs-Production decision (maintainer's call in the release PR).

2. Version single-sourcing

  • version = "0.9.0.dev0" in pyproject is now the only place the version is written (pre-release on purpose; the release PR bumps it to 1.0.0 per the runbook).
  • src/aetherscan/__init__.py reads it back via importlib.metadata.version("aetherscan"), falling back to "0.0.0.dev0" on PackageNotFoundError so source-tree/container runs work uninstalled.

3. Version-coupled weight resolution (hf_hub.py)
Resolution chain is now: explicit local paths → --hf-revisionv{__version__} when running as an installed release → latest semver v* tag → latest final_vX tag → error with guidance. The new step (version_default_revision) is what makes pip install aetherscan==1.0.0 + bare inference pull exactly the v1.0.0 weights. Two deliberate design points, documented in the docstring:

  • Guard: the default only activates when v{__version__} lands in the release-tag family (strict vX.Y.Z). The dev fallback (0.0.0.dev0), .dev pre-releases, and rc/post/local versions all fall through to latest-release resolution.
  • No existence check / no silent fallback: an installed release whose weights tag is missing fails the download loudly (the blessing step was skipped) rather than silently pulling some other version's weights.

--hf-revision help text updated accordingly → README inference CLI block regenerated with utils/print_cli_help.py (top/train blocks verified unchanged).

4. CD workflow (.github/workflows/release.yml)on: push: tags: ["v*"], gates in order:

  1. Version guard: pushed tag must equal v + pyproject version, else fail loudly with pointer to the runbook.
  2. Unit tests: calls tests.yml as a reusable workflow (it gained a workflow_call trigger), so the release gate runs the exact CI suite by construction instead of by copy-paste.
  3. HF weights verification: checks the matching tag exists on zachtheyek/aetherscan (public read, no token). Verify-never-create; the failure message names the fix (utils/hf_tag_release.py) and says not to touch the git tag.
  4. Build: python -m build (sdist + wheel), plus a wheel smoke — pip install --no-deps + __version__ must equal the guarded version.
  5. Publish: pypa/gh-action-pypi-publish@release/v1 with permissions: id-token: write and environment pypi (trusted publishing; no stored token).
  6. GitHub Release: gh release create --verify-tag --generate-notes with the dist files attached (curate the body from the claude-release-notes comments afterwards, per the runbook).

Dry run: workflow_dispatch with test_pypi: true publishes to test.pypi.org under environment testpypi (recommended before the first real release). A manual run with the box unchecked fails the guard — real releases only ever go through tag pushes.

5. utils/hf_tag_release.py — the runbook's weight-blessing step: python utils/hf_tag_release.py --save-tag final_v1 --release v1.0.0 creates the HF release tag pointing at the training upload's commit. Standalone (no PYTHONPATH/install needed), validates the tag shape, and gives targeted errors for missing token / missing save-tag / already-blessed release.

Maintainer prerequisites (one-time, before the first release)

Deliberate divergences / notes vs docs/RELEASE.md

  • Wheel smoke step in the build job is an extra gate the contract doesn't mention (cheap, catches packaging mistakes before publish).
  • Dry runs skip the HF weights gate — pre-release versions never have a blessed weights tag, so the gate would make the dry-run path permanently red; the skip is logged. The contract doesn't specify dry-run semantics.
  • TestPyPI prerequisites (testpypi environment + trusted publisher on test.pypi.org) aren't in RELEASE.md's one-time setup list — they're documented in the workflow header; RELEASE.md could gain a line when Documentation suite: one technical document per pipeline surface, indexed in docs/README.md #131 lands.
  • CUDA classifier: the pre-existing Environment :: GPU :: NVIDIA CUDA :: 12 :: 12.8 is not a valid trove classifier (the list tops out at 12 :: 12.6) and broke python -m build the moment validation turned on — replaced with the :: 12 parent (the 12.4 entry is valid and kept).
  • pip install aetherscan with deps is Linux-only: TF 2.17's and-cuda extra pins nvidia-* wheels unconditionally (no platform markers upstream), and those wheels only exist for Linux. This matches the pipeline's supported runtimes (the clusters); it's why local verification below uses --no-deps.
  • CITATION.cff still says version: 0.1.0 — it's a version-stamped artifact for the release PR to regenerate (runbook step 4, "regen anything version-stamped").

Verification

  • Build + install (macOS arm64, Python 3.12): python -m build succeeds (sdist + wheel); wheel installed into a fresh scratch venv with pip install --no-deps (see Linux-only note above), then:
    $ python -c "import aetherscan; print(aetherscan.__version__)"
    0.9.0.dev0
    
    Source-tree fallback verified too: PYTHONPATH=src python -c ...0.0.0.dev0 (uninstalled).
  • Unit suite: PYTHONPATH=src pytest -m "not gpu and not cluster" -q530 passed, 3 skipped, 2 deselected (arm64 venv, TF 2.17.1; run after rebasing onto HuggingFace Hub integration: opt-in weight upload after training, default weight download for inference, tag dedup guards #130's current tip 7cd19e1) — includes 11 new tests covering the full resolution chain: the dev-version guard (parametrized over dev/rc/post/local/partial versions), explicit-revision precedence, version-pin-without-listing, dev-fallthrough-to-latest, and the end-to-end installed-release download path.
  • Workflows: yaml.safe_load clean on release.yml + tests.yml; actionlint not available locally, so the workflow got a line-by-line manual review (documented gates, permissions per job, artifact handoff, environment-scoped OIDC).
  • pre-commit run --all-files green; ruff lint + format clean.

AI disclosure

Implemented end-to-end with Claude Code (model: Claude Fable 5): packaging, resolution-chain extension, CD workflow, helper script, tests, and this PR body. Human review requested per AI_POLICY.md.

🤖 Generated with Claude Code

zachtheyek and others added 4 commits July 12, 2026 15:58
Uncomment and finalize the hatchling build backend (wheel target
src/aetherscan) and resolve both stale TODO headers. Fill
[project].dependencies from requirements-container.txt's ranges plus the
packages the NGC base image provides implicitly (tensorflow[and-cuda]==2.17.*,
h5py, hdf5plugin, psutil) — a pip install has no base image to lean on.
dev extra gains pre-commit and pytest.

Version single-sourcing: pyproject's version (kept a pre-release,
0.9.0.dev0 — the release PR bumps it) is now the only place the version is
written; __init__.py reads it back via importlib.metadata with a
0.0.0.dev0 fallback so source-tree/container runs work uninstalled.

The pre-existing CUDA '12 :: 12.8' classifier is not in the trove list
(tops out at 12 :: 12.6) and fails hatchling validation — replaced with
the valid ':: 12' parent. Development Status keeps a TODO knob for the
v1 Beta-vs-Production call. gitignore gains python -m build artifacts.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Extend the inference revision-resolution chain with a version-pinned
default between --hf-revision and latest-release resolution: explicit
local paths > --hf-revision > v{__version__} > latest semver v* tag >
latest final_vX tag > error with guidance. version_default_revision()
makes 'pip install aetherscan==1.0.0' + bare inference pull exactly the
v1.0.0-blessed weights — the runtime half of docs/RELEASE.md's contract.

Guard: the default only activates when v{__version__} lands in the
release-tag family (strict vX.Y.Z via _SEMVER_TAG_PATTERN). The
importlib.metadata fallback (0.0.0.dev0), .dev pre-releases, and
rc/post/local versions fall through to latest-release resolution, so
source-tree and dev-install runs keep tracking the newest blessed
weights. Deliberately not existence-checked: an installed release whose
weights tag is missing fails the download loudly rather than silently
pulling another version's weights.

--hf-revision help text updated; README inference CLI block regenerated
(top/train verified unchanged). Tests cover the guard shapes, precedence,
pin-without-listing, dev fallthrough, and the end-to-end installed-release
download; existing no-revision tests now pin the dev version explicitly
so they never depend on whether aetherscan happens to be pip-installed.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
.github/workflows/release.yml on push of v* tags, gates ordered so a
half-released state is impossible:

1. Version guard — tag must equal v + pyproject version, else fail
   loudly (the release PR's bump didn't land on the tagged commit).
2. Unit tests — tests.yml called as a reusable workflow (it gains a
   workflow_call trigger), so the release gate runs the exact CI suite
   by construction rather than by copy-paste.
3. HF weights verification — the matching v* tag must exist on the
   public model repo (no token). Verify, never create: the failure
   message points at utils/hf_tag_release.py and docs/RELEASE.md step 3.
4. Build — python -m build (sdist+wheel) plus a wheel smoke: --no-deps
   install must report exactly the guarded version.
5. PyPI via trusted publishing — pypa/gh-action-pypi-publish, OIDC
   (id-token: write) scoped to the pypi environment; no stored token.
   One-time maintainer setup documented in the workflow header.
6. GitHub Release — gh release create --verify-tag --generate-notes
   with the dist files attached; body curated from claude-release-notes
   comments afterwards per the runbook.

Dry run: workflow_dispatch with test_pypi publishes to test.pypi.org
under the testpypi environment; manual runs refuse to target real PyPI.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The release runbook's step 3 (docs/RELEASE.md): create the HF release
tag vX.Y.Z pointing at a training upload's commit —

    python utils/hf_tag_release.py --save-tag final_v1 --release v1.0.0

A thin wrapper over HfApi.create_tag, standalone on purpose (no
PYTHONPATH or aetherscan install required). Validates the release-tag
shape, loads HF_TOKEN from the gitignored .env when python-dotenv is
available, and turns the common failure modes into targeted errors:
missing token, save-tag not on the repo (train with --hf-upload first),
and an already-existing release tag (immutable by convention). This is
the human blessing decision the release CD deliberately cannot make —
it only verifies the tag exists before publishing.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@claude

claude Bot commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Claude finished @zachtheyek's task in 2m 37s —— View job


PR Review: Packaging + Release CD

  • Gather context and read changed files
  • Review pyproject.toml changes
  • Review __init__.py version single-sourcing
  • Review hf_hub.py version-coupled resolution
  • Review release.yml CD workflow
  • Review hf_tag_release.py helper script
  • Review test coverage
  • Post review findings

Overall

Well-structured PR. The version-coupling design — one string flows from pyproject.toml through the git tag, PyPI release, and HF weights tag, with CD verifying rather than creating — is sound and well-documented. The "verify, never create" principle for HF weights is the right call for a pipeline where training runs take ~30h on cluster hardware.

Strengths

  • Version guard chain is airtight: tag push → pyproject version match → tests → HF weights verification → build → smoke → publish → GitHub Release. Each gate fails loudly with actionable guidance pointing at the runbook.
  • Test coverage is thorough: 11 new tests cover the full resolution chain including parametrized edge cases (dev, rc, post, local, partial versions). The _set_version helper ensures tests are hermetic regardless of install state. Existing tests correctly updated to pin __version__.
  • No-silent-fallback design: an installed release whose weights tag is missing fails the download rather than quietly pulling different weights. This is the safe default for a scientific pipeline.
  • Workflow parallelism: tests and verify-weights run concurrently after guard, with build fanning in — failures surface as early as possible.
  • Trusted publishing: OIDC-based PyPI publishing with no stored API token is the current best practice. Permissions are scoped per-job (id-token: write only on publish, contents: write only on github-release).

Issues and Suggestions

1. release.yml guard step — unclosed file handle (nit)
release.yml:60open('pyproject.toml','rb') in the one-liner relies on GC to close the handle. Fine for a CI step, but a with statement would be cleaner. See inline comment.

2. Consider pinning the actions/checkout and actions/setup-python digests
The workflow uses @v5 tags for actions/checkout and actions/setup-python. For a release pipeline that publishes to PyPI with OIDC trust, pinning to commit SHAs (with a Dependabot config or Renovate to update them) would harden against tag-hijacking supply chain attacks. Not blocking, but worth doing before the first real release.

3. Missing PYTHONPATH=src in tests.yml pytest invocation
tests.yml:60 runs pytest -m "not gpu and not cluster" -q. The pyproject.toml has pythonpath = ["src"] under [tool.pytest.ini_options], so this works — but only because pytest is run from the repo root. This is fine as-is; just noting the dependency on pythonpath config for anyone reading the workflow.

4. hf_tag_release.py — consider validating --save-tag format
The script validates --release against _RELEASE_TAG_PATTERN but accepts any string for --save-tag. A typo in the save-tag would produce a confusing RevisionNotFoundError from the Hub API. A lightweight check (e.g., non-empty, no whitespace) could give a better error message earlier. Low priority since the RevisionNotFoundError handler already gives targeted guidance.

5. pyproject.tomlsetigen is unpinned
All other dependencies have version constraints, but setigen has none. If setigen ships a breaking release, pip install aetherscan would pull it. Worth adding a floor pin (e.g., setigen>=X.Y) based on the version currently in use, consistent with the project's dependency hygiene.

6. Test gap — hf_tag_release.py has no tests
The helper script is standalone and relatively simple, but it's part of the release-critical path. A few unit tests (valid tag, invalid tag shape, missing HF_TOKEN, save-tag not found, already-blessed) would catch regressions. Not blocking since the script is thin and the CD workflow verifies the outcome, but worth noting.

Security

  • OIDC trusted publishing: Correct — no stored PyPI token, id-token: write scoped to the publish job only.
  • Per-job permissions: contents: read at workflow level, elevated only where needed (contents: write for GitHub Release creation). Good.
  • No secrets in logs: HF_TOKEN is read by the Hub client implicitly, never echoed. The verify-weights step uses public read (no token).
  • Dry-run guard: Manual dispatch without test_pypi: true is rejected — prevents accidental real publishes from the UI. Good.

Minor Observations

  • The CITATION.cff version (0.1.0) is noted as a release-PR task — correct, it's out of scope for this PR.
  • The CUDA 12.8 classifier fix (replacing invalid 12 :: 12.8 with the :: 12 parent) is the right call and well-documented.
  • The dev extras gaining pre-commit and pytest is a sensible addition.

Verdict: This is solid work. The architecture is well-thought-out, the code is clean, the tests are comprehensive, and the documentation (both in code and in the PR body) is excellent. The suggestions above are improvements, not blockers.

@claude claude Bot added the needs-issue PR is missing a linked issue label Jul 12, 2026
@claude

claude Bot commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

⚠️ Issue link not detected — This PR targets feature/huggingface-integration rather than the default branch, so the Closes #132 keyword won't create a formal closing reference (GitHub only honors closing keywords for PRs merged into the default branch). Once this PR is rebased onto master (as noted in the stacked-PR instructions), the link should activate automatically. Until then, I've added the needs-issue label as a reminder — it can be removed after the rebase.

Comment thread .github/workflows/release.yml Outdated
REF_NAME: ${{ github.ref_name }}
TEST_PYPI: ${{ inputs.test_pypi }}
run: |
VERSION=$(python3 -c "import tomllib; print(tomllib.load(open('pyproject.toml','rb'))['project']['version'])")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: open('pyproject.toml','rb') leaves the file handle to the garbage collector — fine in a short-lived CI step, but using a with statement would be more consistent with the rest of the codebase's style:

Suggested change
VERSION=$(python3 -c "import tomllib; print(tomllib.load(open('pyproject.toml','rb'))['project']['version'])")
VERSION=$(python3 -c "
import tomllib, pathlib
print(tomllib.loads(pathlib.Path('pyproject.toml').read_text()))
" | head -1)

Actually, the one-liner works because tomllib.load needs a binary stream — but a simpler cleanup would be:

VERSION=$(python3 -c "
import tomllib
with open('pyproject.toml', 'rb') as f:
    print(tomllib.load(f)['project']['version'])
")

Low priority — the step exits immediately anyway.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in 57089c8 — the guard now reads pyproject.toml inside a with block (kept tomllib.load on the binary handle rather than the loads/read_text variant, per your second suggestion).

RELEASE_TAG: v${{ needs.guard.outputs.version }}
run: |
# Range mirrors requirements-container.txt
pip install "huggingface_hub>=1.21,<1.22"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The huggingface_hub version installed here (>=1.21,<1.22) mirrors requirements-container.txt — good. Worth noting that this inline pip install doesn't benefit from setup-python's pip cache (no cache-dependency-path references this workflow for this job). Not a real problem since this step is fast, but if the install ever grows, a pinned requirements file or a cache key would help.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed it's negligible today: one pure-Python wheel installed once per release run. Left uncached deliberately — a cache key would add maintenance for no measurable win at this size. If this step ever grows a real dependency set, moving it to a pinned requirements file + setup-python cache is the right shape.

Comment on lines +87 to +89
tests:
needs: guard
uses: ./.github/workflows/tests.yml

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good design: tests and verify-weights both depend only on guard, so they run in parallel. The build job then fans-in on all three. This means a failing test suite doesn't block the fast HF tag check and vice versa — the failure surfaces as early as possible.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes — that fan-out/fan-in is deliberate: the tag check is seconds while the suite is minutes, so a skipped blessing step surfaces almost immediately instead of hiding behind a full test run.

Comment thread utils/hf_tag_release.py

api = HfApi()
try:
api.create_tag(args.repo_id, tag=args.release, revision=args.save_tag)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The revision=args.save_tag parameter is key — it tells create_tag to point the new release tag at the same commit as the training tag. This is the correct semantic for "blessing" weights. Clear and correct.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right — revision=<save_tag> resolves the training tag to its commit on the Hub side, so the release tag and the training tag end up pointing at the same upload. That's the whole blessing semantic.

Comment thread src/aetherscan/hf_hub.py
runbook's blessing step was skipped) rather than silently pull some other version's
weights.
"""
import aetherscan # noqa: PLC0415 (late import so tests can monkeypatch __version__)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The late import aetherscan to enable monkeypatch in tests is a pragmatic pattern. The alternative (injecting __version__ as a parameter) would ripple through the call chain. This is clean as-is — the noqa + comment make the intent clear.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Exactly the tradeoff considered — threading __version__ through as a parameter would touch every caller of the resolution chain for a value that's conceptually ambient. The late import keeps the seam in one place.

Comment thread tests/unit/test_hf_hub.py
Comment on lines +178 to +181
def test_dev_version_falls_through_to_latest_release(self, monkeypatch):
_set_version(monkeypatch, "0.9.0.dev0")
monkeypatch.setattr(hf_hub, "list_hf_tags", lambda repo_id: ["final_v1", "v0.2.0"])
assert resolve_hf_revision("ns/repo", None) == "v0.2.0"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good: existing tests that previously didn't pin __version__ now correctly pin it to "0.0.0.dev0" so they don't accidentally pass/fail depending on whether the package is installed in the test environment. This is the right fix for test hermeticity.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks — this bit — the tests must not care whether the suite happens to run in a venv with aetherscan pip-installed. Pinning __version__ per-test was the only way to make the no-revision paths hermetic.

…in setigen

- release.yml version guard reads pyproject.toml via a with statement
  instead of leaving the handle to the GC (reviewer nit).
- hf_tag_release.py rejects an empty/whitespace --save-tag up front with
  a clear message instead of surfacing it as a Hub 404 later.
- setigen gains a >=2.6,<3 range (proven 2.6/2.7 line, 2.7.0 in current
  use) in pyproject.toml, requirements-container.txt, and environment.yml
  in lockstep — it was the only unpinned dependency.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@zachtheyek

Copy link
Copy Markdown
Owner Author

Thanks for the review — all six numbered items addressed. Fixes landed in 57089c8; per-item disposition:

  1. Guard file handle — fixed: the version parse now uses a with statement.
  2. SHA-pinning actions/* — agreed on the principle, deliberately not done here: every workflow in this repo pins by major tag (checkout@v5, setup-python@v5, and the claude-* suite), so SHA-pinning only release.yml would give partial protection while diverging from repo convention without the Dependabot/Renovate config that makes SHA pins maintainable. This wants a repo-wide hardening pass (all workflows + digest-update automation) as its own small PR before v1 — happy to include it in the pre-release checklist.
  3. PYTHONPATH in tests.yml — correct reading: the bare pytest invocation leans on pythonpath = ["src"] in [tool.pytest.ini_options] and on running from the repo root, which actions/checkout guarantees. Pre-existing behavior in tests.yml, unchanged by this PR (it only gained the workflow_call trigger).
  4. --save-tag validation — fixed: empty/whitespace save-tags now fail up front with a clear message instead of surfacing as a Hub 404.
  5. setigen unpinned — fixed, and per this repo's lockstep rule the range (>=2.6,<3, covering the proven 2.6/2.7 line with 2.7.0 in current use) went into requirements-container.txt and environment.yml as well as pyproject.toml, so the three dependency surfaces stay in sync.
  6. Tests for hf_tag_release.py — acknowledged, left as-is for now: the suite currently covers src/aetherscan only (no utils/ module has tests), the script is a thin wrapper whose only logic is arg validation + error mapping, and its outcome is independently verified by the CD's HF-weights gate. The validation paths were exercised manually (bad tag shape, bad save-tag, missing token). If utils/ grows a test convention, this script is an easy first target.

Re-verified after the fixes: tests/unit/test_hf_hub.py green (43 passed), python -m build + fresh-venv wheel install still reports 0.9.0.dev0, the guard snippet parses the version correctly under Python 3.12, and pre-commit run --all-files passes.

@zachtheyek

Copy link
Copy Markdown
Owner Author

Re the issue-link note: expected for a stacked PR — Closes #132 is in the body, but GitHub only binds closing keywords for PRs whose base is the default branch. Once #130 merges and this PR is retargeted/rebased onto master, the keyword activates automatically and the needs-issue label can come off. If a formal link is wanted sooner, the manual option is attaching #132 via the Development sidebar on this PR.

@zachtheyek

Copy link
Copy Markdown
Owner Author

@claude please perform a second-pass code review of this PR. Re-review the current diff for any remaining correctness bugs, security issues, or significant quality problems, and post your findings. If the diff is clean, please say so explicitly. (Final verification round before merge.)

@claude

claude Bot commented Jul 12, 2026

Copy link
Copy Markdown
Contributor

Second-pass code review — final verification

I've re-reviewed the full diff (release.yml, tests.yml, pyproject.toml, environment.yml, requirements-container.txt, init.py, cli.py, hf_hub.py, hf_tag_release.py, test_hf_hub.py, .gitignore, README.md) for correctness bugs, security issues, and significant quality problems.

The diff is clean. No remaining correctness bugs, security issues, or significant quality problems found.

What I verified

Workflow (release.yml)

  • Gate ordering is correct: guard → tests + verify-weights (parallel-safe via separate needs: guard) → build → publish → github-release. The dependency chain (build needs all three prior gates) prevents any half-released state.
  • Permissions are properly scoped: top-level contents: read, per-job id-token: write only on publish, contents: write only on github-release. No over-permissioning.
  • The workflow_dispatch guard correctly rejects test_pypi: false manual runs — real releases are tag-push-only.
  • Version extraction via tomllib is safe (no injection — python3 -c with a hardcoded script reading a local file, version value flows through $GITHUB_OUTPUT).
  • HF weights verification uses HfApi().list_repo_refs() on a public repo — no token needed, no secret exposure.
  • The dry-run path correctly skips the HF weights gate and the GitHub Release job.
  • Trusted publishing (OIDC) with environment-scoped id-token: write — no stored API tokens anywhere. Correct.

Reusable workflow (tests.yml)

  • workflow_call added correctly alongside the existing triggers. No parameters needed (the suite is self-contained). Clean.

Packaging (pyproject.toml)

  • Dependency ranges are in lockstep with requirements-container.txt and environment.yml — I cross-checked all three. The extra deps (tensorflow[and-cuda], h5py, hdf5plugin, psutil) correctly mirror environment.yml's pins and are documented as NGC-base-image substitutes.
  • setigen>=2.6,<3 is now consistent across all three files (was unpinned in requirements-container.txt and environment.yml before).
  • The invalid 12 :: 12.8 trove classifier fix to :: 12 is correct.
  • Hatchling build config targets src/aetherscan — matches the source layout.

Version single-sourcing (__init__.py)

  • importlib.metadata.version("aetherscan") with PackageNotFoundError"0.0.0.dev0" fallback is the standard pattern. The fallback keeps version_default_revision() inactive for source-tree runs (the _SEMVER_TAG_PATTERN rejects .dev0 suffixes). Correct.

Version-coupled resolution (hf_hub.py)

  • version_default_revision() correctly guards against non-release versions (dev, rc, post, local, partial) via the existing _SEMVER_TAG_PATTERN regex (^v(\d+)\.(\d+)\.(\d+)$). The late import aetherscan enables monkeypatching in tests — verified the tests exercise this.
  • Resolution chain in resolve_hf_revision(): explicit → version-pinned → tag-list-based. The version-pinned path deliberately skips the tag listing (no existence check) so a missing weights tag fails loudly at download time. This is the correct design per the release contract.
  • No silent fallback paths — an installed release with missing weights will error, not pull wrong weights.

Helper script (utils/hf_tag_release.py)

  • Standalone (no PYTHONPATH/install needed) — only imports huggingface_hub and optionally dotenv. Correct.
  • Input validation: rejects malformed --release (not vX.Y.Z), rejects empty/whitespace --save-tag, checks HF_TOKEN presence before making API calls.
  • Error handling covers the two expected failure modes: RevisionNotFoundError (save-tag doesn't exist) and HTTP 409 (release tag already exists). Unknown errors propagate via raise.
  • No secrets logged — the HF_TOKEN is only checked for existence, never printed.

Tests (test_hf_hub.py)

  • All existing resolve_hf_revision tests now pin __version__ via _set_version(monkeypatch, "0.0.0.dev0") so they don't depend on install state. Correct — prevents flaky behavior when the test suite runs in a pip-installed vs. source-tree environment.
  • New tests cover: installed release → version tag, dev fallthrough → latest release, explicit revision outranks installed release, version pin without tag listing, and the parametrized guard for all non-release version shapes. Good coverage of the resolution chain.

No security issues found. No secrets in code, no injection vectors, no over-permissioned workflows, no unsafe input handling.

LGTM — ready to merge (after #130 lands and this rebases onto master).

🤖 Generated with Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-issue PR is missing a linked issue

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant