Skip to content

fix(semantic): pass convert_to_numpy explicitly so encode() types as ndarray#29

Merged
kschlt merged 1 commit into
mainfrom
fix/mypy-encode-overload
Jul 15, 2026
Merged

fix(semantic): pass convert_to_numpy explicitly so encode() types as ndarray#29
kschlt merged 1 commit into
mainfrom
fix/mypy-encode-overload

Conversation

@kschlt

@kschlt kschlt commented Jul 15, 2026

Copy link
Copy Markdown
Owner

This unblocks CI for every PR in the repo. The lint job gates all other jobs via needs: lint, and it has been failing on 3 pre-existing mypy errors in adr_kit/semantic/retriever.py. Nothing could merge until this landed — PR #28, a one-file docs change, is what surfaced it.

Why

mypy adr_kit/ fails in CI with:

  • L384 — Tensor has no attribute "astype" [attr-defined]
  • L384 — Returning Any from function declared to return ndarray [no-any-return]
  • L476 — Tensor has no attribute "astype" [attr-defined]

These are not caused by any recent change. Main's last CI run was green on 2026-04-04 and main has not moved since — subsequent work lived in gitignored paths. Three months of unpinned dependency drift accumulated with nothing running CI. PR #28 was the tripwire, not the cause.

Root cause: both call sites called self.model.encode(...) without passing convert_to_numpy, relying on its runtime default. sentence-transformers 5.6.0 overloads encode() such that it resolves to torch.Tensor unless convert_to_numpy=True is passed as an explicit literal. Tensor has .numpy(), not .astype() — hence the attr-defined errors. The resulting Any then trips the repo's warn_return_any, producing the second error on the same line.

Approach

Pass convert_to_numpy=True explicitly at both call sites. It is already the library's runtime default, so behavior is byte-identical. What it buys us: the overload pins to the ndarray branch on both old and new sentence-transformers, and the call sites now state an intent they always had — both immediately call .astype(np.float32) and feed np.dot / np.linalg.norm, so they genuinely require an ndarray.

Each call site carries a short comment recording that the argument is load-bearing rather than redundant. Without that note, a reasonable future cleanup could strip it as "obviously the default" and silently reintroduce the break — and local tooling would not catch it (see below).

Considered and rejected:

  • Pin sentence-transformers<5.6 — kicks the can. The call sites are under-specified regardless of version.
  • # type: ignore[attr-defined] — hides a real ambiguity, and would itself break under warn_unused_ignores on the local resolution, where the error does not occur.
  • .numpy() / cast()convert_to_numpy=True is the library's own supported way to request an ndarray.
  • Fixing it on PR docs: source release identity values from .agent/config.json seam #28's branch to turn it green — mixes an unrelated product fix into a docs-only PR.

What Was Tested

make quality cannot verify this, and that is worth understanding before reviewing. Local dev runs Python 3.13, for which sentence-transformers 5.6.0 / torch 2.13.0 publish no wheels; uv resolves down to 5.1.1 / 2.8.0, whose overloads don't trigger the error. Local mypy passes on the broken code. So the normal gate is structurally blind here.

Instead: built a throwaway Python 3.11 venv, installed -e .[dev], and confirmed against the CI install log that it resolved to CI's exact versions — mypy 1.17.1, sentence-transformers 5.6.0, torch 2.13.0, numpy 2.4.6.

In that env, as a controlled A/B with both runs cold (--no-incremental --cache-dir=/dev/null):

  • Without the fix (git stash) — reproduced CI's 3 errors exactly.
  • With the fixSuccess: no issues found in 74 source files.

Then ran every CI step in the same env: ruff check adr_kit/ tests/ clean, black --check 114 files unchanged, and the full pytest tests/ suite at 640 passed. Worth flagging: CI's test job has been skipped for months (gated behind the failing lint), so this is also the first confirmation that the suite passes on CI's interpreter and dependency set at all.

Risks

Low for the change itself. convert_to_numpy=True is the existing runtime default, so there is no behavior change; the 2 tests exercising the retriever pass, as does the full 640-test suite on CI's interpreter.

The systemic cause is deliberately not fixed here, and should be treated as follow-up:

  1. No lock file. All 15 runtime deps are floor-pinned (>=) with no upper bounds, so CI re-resolves the entire tree every run. It will drift again.
  2. Local dev cannot reproduce CI. Python 3.13 resolves a different dependency set than CI's 3.11, so make quality structurally cannot catch this class of bug on a 3.13 machine.

Both are captured in a backlog input for triage. Pinning or locking is a policy decision with real blast radius for downstream consumers installing adr-kit from PyPI — not a drive-by change to make inside a CI unblock.

…ndarray

CI's mypy job failed with 3 errors in this file (Tensor has no attribute
"astype" at lines 384 and 476, plus a Returning-Any-from-ndarray error
tripped by warn_return_any). The lint job gates every other job via
`needs: lint`, so this blocked every PR in the repo.

sentence-transformers 5.6.0 overloads encode() to resolve to torch.Tensor
unless convert_to_numpy is passed as an explicit literal. Tensor has
.numpy(), not .astype() — and both call sites here immediately call
.astype(np.float32) and feed np.dot / np.linalg.norm, so they genuinely
require an ndarray. Passing convert_to_numpy=True states that intent and
pins the overload to the ndarray branch. It is already the library's
runtime default, so behavior is byte-identical.

Considered and rejected: pinning sentence-transformers<5.6 (kicks the can;
the call sites are under-specified regardless of version), a
type: ignore[attr-defined] (hides a real ambiguity, and would itself break
under warn_unused_ignores on the local resolution where the error does not
occur), and .numpy()/cast() (convert_to_numpy=True is the library's own
supported way to ask for an ndarray).

These errors were pre-existing on main, not introduced by recent work.
main's last CI run was green on 2026-04-04 and main has not moved since;
subsequent work lived in gitignored paths, so three months of unpinned
dependency drift accumulated unobserved. They are invisible to local
`make quality` because local dev runs Python 3.13, for which
sentence-transformers 5.6.0 / torch 2.13.0 publish no wheels — uv resolves
down to 5.1.1 / 2.8.0, whose overloads do not trigger the error.

Verified in a throwaway Python 3.11 venv resolving to CI's exact versions
(mypy 1.17.1, sentence-transformers 5.6.0, torch 2.13.0, numpy 2.4.6) as a
cold A/B: without the fix, CI's 3 errors reproduce exactly; with it, mypy
reports no issues in 74 source files. ruff, black --check, and the full
640-test suite also pass there — the first confirmation the suite passes on
CI's dependency set at all, since the test job had been gated off for months.

The systemic cause is deliberately out of scope: there is no lock file and
all runtime deps are floor-pinned with no upper bounds, so CI re-resolves
the tree every run and will drift again. Pinning or locking is a policy
decision, tracked separately.
@kschlt
kschlt merged commit 4fa3d7f into main Jul 15, 2026
8 checks passed
@kschlt
kschlt deleted the fix/mypy-encode-overload branch July 15, 2026 10:39
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