Skip to content

🏷️ types(quantity): make Quantity(1, "m") type-check under pyright#761

Draft
nstarman wants to merge 5 commits into
GalacticDynamics:mainfrom
nstarman:fix-quantity-constructor-typing
Draft

🏷️ types(quantity): make Quantity(1, "m") type-check under pyright#761
nstarman wants to merge 5 commits into
GalacticDynamics:mainfrom
nstarman:fix-quantity-constructor-typing

Conversation

@nstarman

Copy link
Copy Markdown
Contributor

u.Quantity(1, "m") — the constructor call on the first line of the README
and throughout the docs — is a pyright error:

error: Argument of type "Literal['m']" cannot be assigned to parameter "unit"
  of type "AbstractUnit" in function "__init__" (reportArgumentType)

It works at runtime (a converter parses the string), but any user with pyright
enabled sees an error immediately.

Root cause

The unit field is unit: AbstractUnit = eqx.field(converter=<unit>). Pyright's
dataclass_transform support types the generated __init__ parameter from the
converter's signature — but unit is a plum dispatch pyright reads as an
opaque Function, so it falls back to the declared field type AbstractUnit,
which excludes str.

I confirmed the mechanism with a spike, which also disproved the tempting
one-line alternative: adding a @plum.dispatch.abstract entry to unit can't
help — unxt_api.unit is already abstract-only and Angle(2.0, "rad") still
errors against it, because plum's .abstract yields the same opaque Function.
The value field type-checks (Quantity(1, m) accepts an int) because its
converter has concrete overloads pyright can read — not because of any
abstract entry.

Fix

A thin typed wrapper in _src/units.py:

def parse_unit(obj: AbstractUnit | str, /) -> AbstractUnit:
    construct = cast("Callable[[AbstractUnit | str], AbstractUnit]", unit)
    return construct(obj)

used as the converter on all four unit fields — Quantity, StaticQuantity,
Angle, ParametricQuantity
. Pyright reads the union param and accepts the
string; q.unit still reads as AbstractUnit. Runtime is unchanged (it
delegates to unit); the cast restores the union signature the checker can't
see through the dispatch (verified: the wrapper adds zero new pyright errors
over the pre-existing plum-dispatch baseline).

Guard — nothing type-checked in CI before this

No type checker ran in CI or pre-commit, so this fix (and future regressions)
would be invisible. Added:

  • tests/typing/test_construction.py — an assert_type fixture (also a
    runtime smoke test).
  • A pinned pyright (==1.1.411) nox session, scoped via [tool.pyright]
    include = ["tests/typing"] — the tree isn't pyright-clean yet, so this guards
    the constructor typing without gating on the whole source.
  • A pre-commit hook and a CI Pyright job in the CI-Pass needs gate
    (not allowed-skips).

Verified the guard bites: 5 pyright errors on the fixture without the fix, 0
with it.

Scope

Pyright-only by design. mypy and astral's ty don't implement the converter
field-specifier extension, and mypy is already blind to unxt.*
(ignore_missing_imports). A mypy/ty regression is the documented trigger to
revisit a v2-correct .pyi stub (superseding the v1-stale #617). The public
u.unit() returning object to checkers is a related, separate gap noted in the
fixture — a follow-up.

Full CI scope: 3636 passed, 49 skipped, 20 xfailed. No runtime change.

Planned and verified before implementing; found in the pre-v2.0 release-gate
audit.

🤖 Generated with Claude Code

The constructor call on the README's first line was a pyright error:
`Argument of type "Literal['m']" cannot be assigned to parameter "unit" of
type "AbstractUnit"`. It works at runtime (a converter parses the string), but
any user with pyright on sees an error immediately.

Root cause: the `unit` field is `eqx.field(converter=<unit>)`. Pyright's
dataclass_transform support types the generated `__init__` param from the
converter's signature -- but `unit` is a plum dispatch pyright reads as opaque,
so it falls back to the declared field type `AbstractUnit`, which excludes
`str`. (An abstract dispatch entry can't help -- plum's `.abstract` yields the
same opaque Function; the `value` field type-checks because of its *concrete*
overloads.)

Fix: a thin typed wrapper `parse_unit(obj: AbstractUnit | str) -> AbstractUnit`
in _src/units.py, used as the converter on all four `unit` fields (Quantity,
StaticQuantity, Angle, ParametricQuantity). Pyright reads the union param and
accepts the string; `q.unit` still reads as `AbstractUnit`. Runtime is
unchanged (it delegates to `unit`); the `cast` restores the union signature the
checker can't see through the dispatch.

Guard (nothing type-checked in CI before): a `tests/typing` smoke fixture with
`assert_type`, a pinned `pyright` nox session scoped via `[tool.pyright]`, a
pre-commit hook, and a CI job in the CI-Pass gate. Verified the guard bites: 5
pyright errors without the fix, 0 with it.

Pyright-scoped by design -- mypy/ty don't implement the converter
field-specifier, and mypy is already blind to `unxt.*`. A mypy/ty regression is
the trigger to revisit a .pyi stub (superseding the v1-stale GalacticDynamics#617).

Full CI scope 3636 passed; no runtime change.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 21, 2026 18:19
@nstarman
nstarman requested a review from a team as a code owner July 21, 2026 18:19
@nstarman nstarman added this to the v2.0.0 milestone Jul 21, 2026
@github-actions github-actions Bot added 👷 Add / update CI build system Add or update CI build system. 🔧 Add / update configuration Add or update configuration files. ✅ Add / update / pass tests Add, update, or pass tests. 🧩 unxts-parametric Issues/PRs affecting the unxts.parametric namespace package 🏷️ Add / update types Add or update types. labels Jul 21, 2026
@codecov

codecov Bot commented Jul 21, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.10%. Comparing base (4066488) to head (3b54297).
⚠️ Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #761      +/-   ##
==========================================
+ Coverage   91.65%   95.10%   +3.44%     
==========================================
  Files          82       52      -30     
  Lines        3642     2695     -947     
  Branches      303      168     -135     
==========================================
- Hits         3338     2563     -775     
+ Misses        227       93     -134     
+ Partials       77       39      -38     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@nstarman
nstarman marked this pull request as draft July 21, 2026 18:27

Copilot AI 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.

Pull request overview

This PR addresses a pyright typing regression where u.Quantity(1, "m") (and similar constructors used throughout the README/docs) incorrectly fails type-checking because pyright infers the eqx.field(converter=...) parameter type from an opaque plum dispatch. It introduces a typed converter wrapper and adds a pyright-only “typing guard” to CI/pre-commit to prevent regressions without requiring the whole codebase to be pyright-clean.

Changes:

  • Add parse_unit(obj: AbstractUnit | str) -> AbstractUnit as a typed eqx.field converter and wire it into Quantity, StaticQuantity, Angle, and ParametricQuantity.
  • Add a focused typing smoke test (tests/typing) plus pyright configuration, a nox session, a pre-commit hook, and a CI job to enforce the guard.
  • Pin pyright (and its transitive nodeenv) in the lockfile and dependency groups to keep diagnostics stable.

Reviewed changes

Copilot reviewed 10 out of 11 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/unxt/_src/units.py Adds the typed parse_unit wrapper intended for eqx.field(converter=...) use.
src/unxt/_src/quantity/quantity.py Switches Quantity.unit converter to parse_unit.
src/unxt/_src/quantity/static_quantity.py Switches StaticQuantity.unit converter to parse_unit.
src/unxt/_src/quantity/angle.py Switches Angle.unit converter to parse_unit.
packages/unxts.parametric/src/unxts/parametric/_src/parametric.py Switches ParametricQuantity.unit converter to parse_unit.
tests/typing/test_construction.py Adds a pyright-scoped typing smoke test for string-unit constructors.
pyproject.toml Adds a pinned typecheck dependency group and [tool.pyright] config scoped to tests/typing.
noxfile.py Adds a pyright nox session for the typing fixture.
.pre-commit-config.yaml Adds a local pyright hook (via uv run) to run the typing guard on relevant changes.
.github/workflows/ci.yml Adds a dedicated Pyright CI job and includes it in the required “CI-Pass” gate.
uv.lock Locks pyright==1.1.411 and transitive deps (nodeenv), plus the new typecheck group.

Comment thread src/unxt/_src/units.py
Comment thread tests/typing/test_construction.py
@nstarman nstarman modified the milestones: v2.0.0, future Jul 21, 2026
The test relied only on static assert_type plus does-not-raise calls; add
runtime asserts that each string-unit constructor parses to the expected unit,
so it validates behaviour under pytest as well.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 22, 2026 04:45

Copilot AI 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.

Pull request overview

Copilot reviewed 10 out of 11 changed files in this pull request and generated 4 comments.

Comment thread packages/unxts.parametric/src/unxts/parametric/_src/parametric.py Outdated
Comment thread packages/unxts.parametric/src/unxts/parametric/_src/parametric.py Outdated
Comment thread tests/typing/test_construction.py
Comment thread tests/typing/test_construction.py
Review follow-ups on the converter PR:

- parse_unit is now re-exported from `unxt.units` (added to `__all__`), so the
  separate `unxts.parametric` package imports it from the public path instead
  of `unxt._src.units` -- removing the cross-package dependency on a private
  module.
- The typing smoke test now also `assert_type`s Angle. StaticQuantity is
  omitted deliberately: its *value* field is separately opaque to pyright
  (`value: StaticValue` rejects `Literal[1]`), unrelated to this unit-field
  fix; ParametricQuantity is guarded by its own package's suite.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 22, 2026 12:17

Copilot AI 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.

Pull request overview

Copilot reviewed 11 out of 12 changed files in this pull request and generated 1 comment.

Comment thread .github/workflows/ci.yml
@github-actions github-actions Bot added the ♻️ Refactor code Refactor code. label Jul 22, 2026
The pyright job jumped from 'Install uv' straight to 'uv run --frozen nox -s
pyright' with no dependency sync, so nox itself was never installed (nox lives
in the non-default 'nox' group) -- the job would fail with nox not found. Add
the Install step the other nox jobs use. The session installs its own
'typecheck' group + extras via nox-uv, so the job only needs 'nox'.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 22, 2026 12:31

Copilot AI 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.

Pull request overview

Copilot reviewed 11 out of 12 changed files in this pull request and generated no new comments.

Copilot AI 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.

Pull request overview

Copilot reviewed 11 out of 12 changed files in this pull request and generated 1 comment.

Comment thread src/unxt/_src/units.py Outdated
`parse_unit` is defined in this module and re-exported publicly via
`unxt.units`, but was missing from this module's `__all__`, making star-import
and export auditing inconsistent. Add it. Addresses PR review.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings July 22, 2026 14:26

Copilot AI 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.

Pull request overview

Copilot reviewed 11 out of 12 changed files in this pull request and generated no new comments.

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

Labels

👷 Add / update CI build system Add or update CI build system. 🔧 Add / update configuration Add or update configuration files. ✅ Add / update / pass tests Add, update, or pass tests. 🏷️ Add / update types Add or update types. ♻️ Refactor code Refactor code. 🧩 unxts-parametric Issues/PRs affecting the unxts.parametric namespace package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants