Relax dependency bounds and enable Dependabot#93
Conversation
bernalde
left a comment
There was a problem hiding this comment.
Maintainer review — PR #93 @ 0f6b8bc (COMMENT)
I authored this PR and it is a draft, so this is a COMMENT (GitHub rejects author APPROVE/REQUEST_CHANGES). Since this branch was cut, #92 and #96 have merged to master, and this PR now conflicts.
Verified locally in a clean worktree at the head, with assimulo not installed:
pip install -e ".[test]"resolved numpy 2.5.0 / scipy 1.17.1 — the relaxed bounds work against the modern stack.pytest tests/test_optional_dependencies.py tests/ -m "not assimulo"-> 2 passed, 6 deselected.simps->simpson(row, x=time)is the correct replacement and the optionalTerminateSimulationimport guard letsPharmaPy.Commonsimport without assimulo.
Blocking — conflicts with master; branch cut from a stale base
mergeStateStatus is DIRTY / CONFLICTING. This branch descends from 6d27b40, before #92's follow-up df4eebd and #96 landed on master. A test merge into master conflicts in:
pyproject.toml([project.dependencies]block) — resolve by keeping this PR's new bounds + theassimuloextra..github/workflows/ci.yml(install step) — resolve by keeping this PR'spip install -e ".[test]".
Update the branch from master (append-only merge or rebase) and resolve both. Watch one thing: this branch still declares build-system.requires = ["setuptools>=61", "wheel"], but master already moved to setuptools>=77 (from #92). The 3-way merge keeps >=77 automatically because this PR doesn't touch that line — just confirm >=77 survives after resolving. Expected for a draft, but it blocks merge.
Summary
- Blocking: merge conflict vs
master(stale base) — must update and resolvepyproject.toml+ci.yml. - Nonblocking: core-dependency list is duplicated across
requirements.txt/requirements-assimulo.txt/pyproject.toml(inline); Dependabot has noignorefor the intentionally-pinnedassimulo/cython(inline). - Questions: none.
- Tests: relaxed bounds verified with numpy 2.5.0 / scipy 1.17.1; optional-deps + not-assimulo suite pass (2 passed, 6 deselected) with no assimulo. GitHub
Core testspass on the PR head — but that is head-only; the merge result currently conflicts and has not been CI-tested. - Linked issue:
Closes #9is appropriate — #9's acceptance criteria (documented bounds in DEPENDENCIES.md, grouped Dependabot, suite green under relaxed bounds) are met and the base ismaster, so it will auto-close. #10 (full solver interface / scipy fallback) remains open and is correctly not claimed. - Merge-readiness: draft + author -> COMMENT only; this review does not satisfy branch-protection approval. Resolve the conflict and mark ready; if approval is required an eligible maintainer's APPROVE is needed (no gate currently reported).
I would not merge this until the blocking conflict above is resolved.
| matplotlib | ||
| pandas | ||
| # Core runtime dependencies. See DEPENDENCIES.md for the remaining caps. | ||
| numpy>=1.22 |
There was a problem hiding this comment.
Nonblocking: core dependencies are now declared in three places — requirements.txt, requirements-assimulo.txt, and pyproject.toml ([project.dependencies] + the assimulo extra). That is three lists to keep in sync, and bounds can silently drift. Consider making pyproject.toml the single source of truth and noting in DEPENDENCIES.md that requirements*.txt are convenience mirrors (or generate them from the project metadata).
There was a problem hiding this comment.
Addressed in 5d188f0: DEPENDENCIES.md now states that pyproject.toml is the install-metadata source of truth and that requirements.txt / requirements-assimulo.txt are convenience mirrors. I also verified the mirror lists match the project metadata.
| @@ -0,0 +1,25 @@ | |||
| version: 2 | |||
| updates: | |||
| - package-ecosystem: "pip" | |||
There was a problem hiding this comment.
Nonblocking: assimulo==3.4.3 (exact pin) and cython>=0.29,<3 (capped) are intentionally constrained — per DEPENDENCIES.md, until the Assimulo job proves a broader stack — but there is no ignore here, so Dependabot will open PRs bumping assimulo (which cannot build without SUNDIALS) and raising the cython cap. Add an ignore block for assimulo (and cython major bumps), or group and document them, to avoid failing/no-op update PRs.
There was a problem hiding this comment.
Addressed in 5d188f0: .github/dependabot.yml now ignores assimulo updates and Cython major-version updates, and DEPENDENCIES.md documents why those solver-stack constraints stay in place.
|
Addressed the review feedback on the current head Commits pushed:
Main changes:
Verification:
Comments intentionally not addressed: none. Remaining gates: the PR is still draft, and the author COMMENT review does not provide an eligible maintainer approval if branch protection later requires one. Current merge state is |
Summary
Traceability
Closes #9.
Depends on #92 / #8 and is stacked on packaging/pyproject-metadata.
Refs #1, #2, CryPTSys#106, and CryPTSys#107.
Verification
Actions
GitHub Actions should run on this PR through the stacked CI workflow from #91.