Skip to content

fix: package GROMACS run schedule templates - #56

Closed
maxscheurer wants to merge 2 commits into
developfrom
feature/issue-55-fix-wheel-force-include
Closed

maxscheurer wants to merge 2 commits into
developfrom
feature/issue-55-fix-wheel-force-include

Conversation

@maxscheurer

Copy link
Copy Markdown
Collaborator

Closes #55

Fix wheel packaging for the GROMACS run-schedule templates across direct-checkout and isolated VCS/source builds, with artifact-level regression coverage.

Implementation plan posted as a comment below.

@maxscheurer

Copy link
Copy Markdown
Collaborator Author

Implementation Plan

Problem analysis

Hatchling selects the eight tracked GROMACS .mdp templates differently depending on source context:

  • In a direct Git checkout, the root *.mdp ignore rule prevents normal wheel discovery from including the templates, so the current directory-level force-include supplies them.
  • In an isolated VCS/source build, normal discovery already includes those tracked templates and the same force-include adds them again, causing the duplicate-file exception.

Deleting the force-include alone fixes the reported git dependency build but produces incomplete wheels from the repository checkout. The fix must make the package-owned templates discoverable without broadly exposing generated simulation .mdp files.

Deliverables

  1. Scope the ignore policy for package templates

    • Keep the general *.mdp rule for generated simulation files.
    • Explicitly unignore only .mdp files under mdfactory/run_schedules/gromacs/.
  2. Remove the conflicting Hatch configuration

    • Delete the directory-level mdfactory/run_schedules/gromacs force-include.
    • Retain the existing single-file entries for spc216.gro and cgenff_output.itp.
    • Update the nearby comment so it accurately describes the remaining entries.
  3. Add artifact-level packaging regression coverage

    • Build a wheel from the repository checkout.
    • Build a second wheel from a VCS-like exported source tree without repository metadata, covering the source context that previously produced duplicate entries.
    • Inspect each artifact and require exactly the expected eight bilayer/mixedbox MDP paths.
    • Extract the wheel into an isolated location and smoke-test RunScheduleManager there so the test cannot pass by reading resources from the source checkout.
  4. Run packaging coverage in CI

    • Execute the focused packaging test in the build job, where Hatch is already available.
    • Preserve the fast default test command, which intentionally excludes tests whose names contain build.
  5. Keep unrelated packaging policy out of scope

    • Do not add a force-include for the rescue fixture: it is already included on develop, and no missing-artifact behavior was demonstrated.
    • Do not change the broader policy of including tests/fixtures in production wheels as part of this bug fix.

Acceptance criteria

  • A wheel built directly from a Git checkout succeeds.
  • A wheel built from a metadata-free exported source tree succeeds without duplicate-file errors.
  • Each wheel contains exactly these eight package resources, once each:
    • gromacs/bilayer/{em,md,npt,nvt}.mdp
    • gromacs/mixedbox/{em,md,npt,nvt}.mdp
  • RunScheduleManager loaded from extracted wheel contents resolves all four files for both packaged system types.
  • An isolated git-revision dependency build no longer raises Hatchling's duplicate-file exception.
  • Generated .mdp files outside mdfactory/run_schedules/gromacs/ remain ignored by Git.
  • Existing short tests, lint, formatting, and wheel build checks pass.

Files to modify

  • .gitignore — add a narrowly scoped negation for packaged GROMACS MDP templates.
  • pyproject.toml — remove the conflicting directory force-include and correct its explanatory comment.
  • mdfactory/tests/test_packaging.py — add helpers and tests for both source contexts, exact wheel contents, and isolated runtime resource resolution.
  • .github/workflows/ci.yml — invoke the focused packaging regression in the build job.

No production Python module or documentation change is expected because runtime lookup behavior and user-facing behavior remain unchanged.

Testing approach

  • Use pytest's tmp_path for build outputs, source export, and wheel extraction.
  • Invoke the repository's configured Hatch build tooling through a checked subprocess and surface captured output on failure.
  • Use the standard-library ZIP APIs to compare wheel members against an explicit expected path set, catching both omissions and duplicate entries.
  • Launch the runtime smoke check from outside the repository with the extracted wheel tree first on the import path, preventing accidental source-tree imports.
  • Include a focused assertion that the scoped ignore exception applies to package templates while a representative external .mdp path remains ignored.
  • Run:
    • the focused packaging test,
    • pytest -k "not build" mdfactory,
    • formatting/lint hooks,
    • and the existing Hatch build command.

Risks and open questions

  • Source-context fidelity: the exported-tree test must omit VCS metadata so it exercises the same Hatchling selection path as an isolated dependency build.
  • Import isolation: the smoke test must not inherit the checkout as its import source.
  • Test cost: two artifact builds are intentionally kept out of the short suite and run in the CI build job.
  • Ignore scope: the negation must remain limited to package-owned templates; broad .mdp unignoring would expose simulation outputs.
  • Future system types: the expected set intentionally reflects the eight resources currently shipped. Adding another packaged schedule directory should require updating the packaging expectation, making artifact changes explicit.

Plan created by mach6

@maxscheurer

Copy link
Copy Markdown
Collaborator Author

Progress Update

Implemented the wheel-packaging fix for GROMACS run-schedule templates:

  • Added a narrowly scoped Git ignore exception for package-owned MDP templates while retaining the broad ignore rule for generated simulation files.
  • Removed the conflicting directory-level Hatch force-include and retained the unrelated single-file inclusions.
  • Added packaging regression coverage for Git-checkout and metadata-free exported-source builds.
  • Verified that each wheel contains exactly the eight expected templates once and that RunScheduleManager resolves them from extracted wheel contents.
  • Added the focused packaging regression to the CI build job.

Verification completed:

  • Packaging tests: 3 passed.
  • Hatch sdist and wheel build: passed; wheel contains all eight templates exactly once.
  • Isolated local git-revision install with uv: passed; installed package contains all eight templates.
  • Changed-file Ruff and pre-commit checks: passed.
  • Short suite: 2,950 passed, 4 skipped, with one unrelated existing Python 3.14 multiprocessing pickling failure in test_lock_folder_processes.

Commit: bbcd743


Progress tracked by mach6

@maxscheurer
maxscheurer marked this pull request as ready for review September 14, 2026 07:26
@maxscheurer

Copy link
Copy Markdown
Collaborator Author

Unverified Review Candidates — Pending Assessment

Review round: 1
Reviewed commit: bbcd743

These are unverified candidates. Severity reflects reviewer confidence; do not treat any item as a merge blocker until the assessment comment is posted.

Critical

None.

Important

None.

Suggestions

Finding 1 — Collapse redundant wheel-member assertions (confidence: 88)

mdfactory/tests/test_packaging.py:83-85 first compares the member set and then uses Counter to reject duplicates. Comparing sorted(run_schedule_members) with sorted(EXPECTED_RUN_SCHEDULES) would express both exact membership and multiplicity in one assertion and remove the Counter import without changing behavior.

Strengths

  • The core fix combines removal of the conflicting directory force-include with a narrowly scoped Git-ignore exception, covering both metadata-free duplicate entries and direct-checkout omissions.
  • Packaging tests exercise both source contexts and verify exact wheel contents, duplicate absence, and runtime resolution from extracted wheel contents rather than the checkout.
  • The runtime smoke test explicitly verifies that imports originate from the extracted artifact.
  • CI runs the build-heavy packaging regression separately while preserving the fast default suite.
  • The change remains within the approved scope and avoids unrelated fixture-packaging policy changes.

Agents run: code-reviewer, error-auditor, test-reviewer, completeness-checker, simplifier


Reviewed by mach6

@maxscheurer

Copy link
Copy Markdown
Collaborator Author

Review Assessment

Unverified review candidates

Classifications

Finding Classification Reasoning
Finding 1 — Collapse redundant wheel-member assertions nitpick Factual: The existing assertions are correct. A sorted-list comparison is pass/fail equivalent, but the separate Counter assertion explicitly diagnoses duplicate members—the regression under test. Scope: The issue and approved plan do not require this cleanup; the current form directly expresses the “once each” acceptance criterion. Practical: Shipping the current code causes no meaningful harm. The proposed change offers no material benefit and would mildly weaken diagnostic clarity.

Action Plan

No merge blockers. No changes are required before merge.


Assessment by mach6

@maxscheurer

Copy link
Copy Markdown
Collaborator Author

Superseded by stack #67

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant