Repository navigation
Conversation
skrawcz
left a comment
There was a problem hiding this comment.
AI-assisted preliminary review — findings must be fixed or technically refuted before human review.
Committing the CI test lock is a sound policy now that the daily canary separately exercises newest dependency releases. The dependency groups, UV_LOCKED enforcement, lock-only Dependabot update, and trigger coverage are coherent; the lock validates and the locked focused tests plus exact-head CI are green.
Two integrity gaps should be addressed before this becomes the merge gate:
maincurrently hasrequired_status_checks.strict: false, so concurrent dependency PRs can each pass on an old base and then merge into a stale lock state that no branch tested. Please require branches to be up to date, add an equivalent merge queue, or technically demonstrate another mechanism that validates the exact merge result.- PR CI installs the latest unpinned uv, so a uv release can invalidate or reinterpret the unchanged lock and break every open PR—the same non-attributable instability this change is intended to remove. Please pin the uv version used for locked CI, or refute this with a concrete compatibility policy/mechanism.
The lock structure, dependency groups, canary split, and tests otherwise look sound.
potiuk
left a comment
There was a problem hiding this comment.
Weighing in on the two findings in #1807 (review). On (1) I think the bar is set too high; on (2) I agree, with a fix that matches the PR's own design.
1. strict: false and a stale lock on main. The window is narrower than it looks.
Two PRs that both change dependencies touch the same [package.metadata] requires-dist block of the root package in uv.lock, so git already flags them as conflicting.
The clean-merge case is a lock-only Dependabot PR plus a contributor's dependency change. UV_LOCKED checks that the lock still satisfies pyproject.toml, not that it matches a fresh resolve, so that merge usually still passes.
If it doesn't, the canary's static-checks job catches it within a day: it syncs from the committed lock with UV_LOCKED=1 and never runs the re-resolve step. uv lock fixes it.
Requiring every PR to be up to date with main makes all contributors pay for a rare, self-healing case. A cheaper option, if we want it caught within minutes instead of a day: add a push trigger on main for uv.lock / pyproject.toml that runs uv lock --check.
We have run a committed uv.lock in Airflow without up-to-date branch protection, at far higher PR volume, and stale-lock merges have been an occasional nuisance, not a reason to gate every PR.
2. Unpinned uv. Agreed that this reintroduces the "breaks for a reason the author didn't cause" problem this PR is meant to remove. Suggestion: pin uv for pull_request runs (e.g. curl -LsSf https://astral.sh/uv/0.13.7/install.sh | sh, or astral-sh/setup-uv with a version), and keep the latest uv on scheduled/manual runs. The canary then flags new uv releases the same way it flags new package releases, and bumping the pin is a one-line PR when it does. Dependabot won't bump it, so that bump stays manual, but at least it is visible and attributable.
Otherwise the dependency groups, the canary split and the lockfile-only Dependabot config look right to me.
This review was drafted by an AI-assisted tool and
confirmed by an Apache Hamilton maintainer. The findings
below are observations, not blockers; an Apache Hamilton
maintainer — a real person — will take the next look at the
PR. If you think a finding is mis-applied, please reply on
the PR and a maintainer will weigh in.
|
Small comment from the mentor :) . Feel free to ignore it :) |
|
Addressing skrawcz's review and potiuk's mentor follow-up:
The PR is one commit directly on current main, with its Polars 2 and Python 3.15 coverage preserved. Lock validation, stale-lock rejection, actionlint and repository lint pass. Adopting the lockfile and accepting the residual merge window remain maintainer decisions. |
potiuk
left a comment
There was a problem hiding this comment.
Re-reviewed f4c4393 against my earlier comment.
uv pin — resolved. Both Static Checks and Unit Tests now install from $UV_INSTALLER_URL. That is the versioned 0.13.7 installer on pull_request runs and the latest installer on scheduled and manual runs. PR results are reproducible, and the canary picks up new uv releases the same way it picks up new package releases.
strict: false — fine as is. The canary's Static Checks job syncs from the committed lock with UV_LOCKED=1 and never runs uv lock --upgrade (only the test job does). So a stale lock on main surfaces within a day, and uv lock repairs it. Tightening merge policy is a separate decision for the whole repo, and the description records the remaining window. If a day turns out to be too slow, a push-on-main trigger that runs uv lock --check is a cheap follow-up. It doesn't need to block this PR.
One non-blocking nit:
test-polars = ["polars"]repeatspolars, which is already in thetestgroup.--group test-polarsin the polars and narwhals steps therefore installs nothing extra. Thetests/plugins/test_polars_*tests need polars intest, so the simplest fix is to drop the group and the two--group test-polarsflags.
From my side this is ready. @skrawcz, your change request is the remaining gate. Both of its points are now addressed in the code or in the description.
This review was drafted by an AI-assisted tool and
confirmed by an Apache Hamilton maintainer. The findings
below are observations, not blockers; an Apache Hamilton
maintainer — a real person — will take the next look at the
PR. If you think a finding is mis-applied, please reply on
the PR and a maintainer will weigh in.
This puts a project decision to the maintainers: should Hamilton commit a lockfile for its CI? Maintainer review decides whether to adopt it.
PR CI currently resolves whatever dependencies and uv were released most recently. An upstream release can break an unchanged PR, and yesterday's result may be impossible to reproduce. The daily canary from #1800 provides a separate place to test new releases.
This proposal commits
uv.lock, enforces it withUV_LOCKED=1, and installs uv 0.13.7 in both jobs on PR runs. Scheduled and manual test runs use the latest uv and runuv lock --upgradebefore installing. Dependabot proposes one grouped, lock-only update each week after a four-day cooldown. Library dependency ranges stay unchanged.CI on the current commit passes Static Checks, Unit Tests and contrib tests on Python 3.11–3.15. Local proof includes 1,679 main-suite tests, lock checks and installer-event checks. Remaining risks are the concurrent-lock merge window, manual uv-pin bumps, and deliberately unlocked compatibility steps.
Contributor task: change a test dependency and reproduce a PR failure.
Before: CI resolves anew on each run; an unchanged PR can fail after an upstream release.
After: run
uv lockand commit its result with the dependency declaration. PR runs keep the committed package versions and uv version; a stale lock fails with instructions to refresh it.Configuration and compatibility coverage
testincludes narwhals and the PostgreSQL driver used by openlineage;test-polarsandtest-pysparkcover their plugin installs.versioning-strategy: lockfile-only, preserving the declared dependency ranges.Validation commands and observed results
Pinned uv: 0.13.7. Local runtime: Python 3.14.3.
A shell check runs both installer commands with a stub transport. PR events choose the versioned 0.13.7 URL; schedule and workflow_dispatch choose the latest installer URL. Inverting the event comparison fails the check. The canary-only upgrade condition and locked-sync setting are checked as well.
The host CPU lacks AVX2. Local Polars checks use the official compatibility runtime at the same locked Polars version, 2.1.0; this changes only the local test environment, not the committed dependency declarations or lock.
Unit Tests CI on f4c43930 passes Static Checks and every Python 3.11–3.15 leg. Each leg skips the canary-only re-resolution and passes the Polars 1.x reader step. Contrib CI on the same commit also passes all five Python versions.
Decisions
strict: falsemerge policy. Changing branch protection for all contributors exceeds this proposal. The daily canary's static-checks job validates the committed lock without refreshing it; that detects a stale merge but does not prevent the race.Debt left
strict: false. The daily committed-lock lint check detects it;uv lockrepairs it. Stronger merge serialization is a separate maintainer policy decision.