Skip to content

Commit uv.lock and install CI's test dependencies from it - #1807

Open
Dev-iL wants to merge 1 commit into
apache:mainfrom
SummitSG-LLC:2611/uv-lock
Open

Dev-iL wants to merge 1 commit into
apache:mainfrom
SummitSG-LLC:2611/uv-lock

Conversation

@Dev-iL

@Dev-iL Dev-iL commented Nov 8, 2026 •

Copy link
Copy Markdown
Collaborator

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 with UV_LOCKED=1, and installs uv 0.13.7 in both jobs on PR runs. Scheduled and manual test runs use the latest uv and run uv lock --upgrade before 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 lock and 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
  • test includes narwhals and the PostgreSQL driver used by openlineage; test-polars and test-pyspark cover their plugin installs.
  • The lock includes current main's Polars 2 and Python 3.15 dependency declarations, including the Python 3.15 ddtrace pre-release. User-facing dependency declarations match main.
  • The workflow preserves main's Python 3.11–3.15 matrix, redirect-package trigger paths and Polars 1.x reader checks.
  • Polars 1.x checks deliberately install an older major outside the default lock. Vaex also stays outside the lock because its pandas/dask constraints would downgrade other tests. Their workflow comments explain those exceptions.
  • PR trigger paths include the lock and the local package metadata it resolves.
  • The canary refresh is ephemeral; no step commits it back.
  • Dependabot uses 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.

uv lock --check --offline
Resolved 407 packages

uv sync --locked --group test --python 3.14
Installed the locked test environment

# Copy of project metadata, with tabulate added to test without refreshing the lock:
uv lock --check --offline
exit 1: The lockfile at uv.lock needs to be updated

actionlint .github/workflows/hamilton-main.yml
exit 0

UV_LOCKED=1 prek run --all-files --hook-stage pre-merge-commit
all hooks passed

# Same locked environment, plus Polars compatibility runtime for this CPU:
HAMILTON_TELEMETRY_ENABLED=false python -m pytest tests/ --cov=hamilton --ignore tests/integrations -q
1679 passed, 8 skipped

python -m pytest plugin_tests/h_polars plugin_tests/h_narwhals plugin_tests/h_pandas plugin_tests/h_dask tests/integrations -q
77 passed, 1 xpassed

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

  • Lockfile adoption remains the maintainers' project decision.
  • PR runs pin uv; canaries exercise latest uv. This makes a resolver update deliberate, at the cost of a manual pin bump.
  • Keep the project's existing strict: false merge 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.
  • Keep the lint job on the committed lock during canary runs; test jobs resolve latest packages.
  • The workflow removes unreachable OS-specific steps and bounds each test job to 60 minutes.

Debt left

  • Concurrent lock/dependency PRs can still leave a stale lock on main under strict: false. The daily committed-lock lint check detects it; uv lock repairs it. Stronger merge serialization is a separate maintainer policy decision.
  • The PR uv pin needs manual updates; scheduled/manual runs expose compatibility with newer uv.
  • Sequential test steps still stop at the first failure in each matrix leg.
  • After merge, observe one main canary run and Dependabot's first weekly lock-only PR. Neither can be proven by local checks.

@Dev-iL
Dev-iL requested review from cswartzvi, elijahbenizzy, jernejfrank, pjfanning, potiuk and skrawcz and removed request for skrawcz November 9, 2026 07:37

@skrawcz skrawcz left a comment

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.

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:

  1. main currently has required_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.
  2. 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 potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

@potiuk

potiuk commented Nov 9, 2026

Copy link
Copy Markdown
Member

Small comment from the mentor :) . Feel free to ignore it :)

@Dev-iL

Dev-iL commented Nov 10, 2026

Copy link
Copy Markdown
Collaborator Author

Addressing skrawcz's review and potiuk's mentor follow-up:

  1. uv is pinned for PR runs. In f4c4393, both Static Checks and Unit Tests install uv 0.13.7. Scheduled/manual runs still use latest uv, and the test canary refreshes the lock. CI on this commit passes on Python 3.11–3.15, with the canary-only re-resolution skipped on each PR leg. Local installer checks cover all three events and reject an inverted condition.

  2. The existing strict: false policy is retained. Concurrent dependency updates can still leave a stale lock on main. The daily canary's Static Checks job uses UV_LOCKED=1 on the committed lock without upgrading it, so it detects that state; it does not prevent the race. The description records the risk and the uv lock repair. Requiring every PR to update its base, or introducing a merge queue, changes repository-wide merge policy; I am leaving that decision to maintainers rather than including it in this proposal.

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.

@Dev-iL
Dev-iL requested a review from skrawcz November 10, 2026 08:16

@potiuk potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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"] repeats polars, which is already in the test group. --group test-polars in the polars and narwhals steps therefore installs nothing extra. The tests/plugins/test_polars_* tests need polars in test, so the simplest fix is to drop the group and the two --group test-polars flags.

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 branch has not been deployed

No deployments
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.

3 participants