fix: stop silently dropping a failed FAISS reconstruction, and bound the linter - #8
Merged
Merged
Conversation
…the linter `ruff>=0.1.0` with no explicit `[tool.ruff.lint] select` means `ruff check` here answers differently depending on the day it runs. ruff 0.16.0 grew its DEFAULT rule set from 59 rules to 413 and this project selected no rules, so it silently inherited 354 of them. Verified against this tree: ruff 0.14.x All checks passed! ruff 0.16.0 Found 103 errors. This repo has no test/lint CI workflow, so nothing would have caught it; the only consumer of the linter here is a developer running it locally, which is exactly the case an unbounded range makes unreproducible. Real defect (1): * `VectorIndex.get_vectors` caught every exception from `faiss.reconstruct_n(...)` and `pass`ed, falling through to `return None`. `None` is this method's DOCUMENTED answer for "FAISS-only storage, nothing to hand back", so a failed reconstruction was indistinguishable from the normal empty case and a caller would conclude the index simply had no vectors. It now catches what FAISS actually raises (`RuntimeError`, `AttributeError`) and logs a warning naming the vector count and the exception type. The return value is unchanged for every input that previously succeeded. Fixed in code (no behaviour change): UP045/UP007/UP006/UP035 (76) `Optional[X]` and `Union[...]` to PEP 604, `Type`/`Tuple` to builtins -- all valid at the declared `requires-python = ">=3.10"`; I001 (9) import ordering; F401 (5) unused imports; PIE790 (5) `pass` after a docstring; RUF022 (2) `__all__` sorting; SIM102 (2) two nested `if`s collapsed in `_compute_context_bonus`; SIM117 (1) nested `with` collapsed in a test. Disabled deliberately (1, narrowly): * RUF022 is off for `src/openadapt_retrieval/__init__.py` only, via per-file-ignores with the reasoning inline. That `__all__` is grouped by subsystem with `# Embedders` / `# Retriever` / `# Storage` headings, and an alphabetical sort strands all three comments against unrelated names. The grouping documents the package layout and is worth more than the ordering. Every other `__all__` in the package was sorted. No `select` was added. After the above the package clears the whole 413-rule default set, so narrowing it would discard coverage it is earning today -- including the S110 rule that surfaced the defect. Predictability comes from bounding `ruff>=0.16,<0.17` instead: ruff only grows its default set in a minor release, so the rule set cannot move until a person raises that ceiling and owns the new findings in the same change. `target-version = "py310"` is now stated rather than inferred. uv.lock is relocked so ruff actually resolves 0.16.0; without that the bound is inert for anyone running `uv sync`. The lock rewrite is large because uv also upgraded the lockfile format (revision 1 to 3, adding upload-time metadata); no dependency version changed except ruff, and the local package version was corrected from a stale 0.1.0 to the declared 0.1.2. 28 passed, 1 skipped (3 gpu/slow deselected, as before). `uv run ruff check .` is clean under the locked ruff 0.16.0. Note: `src/**/__pycache__/*.pyc` and `tests/__pycache__/*.pyc` are committed to this repo and were left untouched here. They are stale build artifacts that do not belong in version control and should be removed separately. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NyCHrzA1psrKMFfroYbzaM
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ruff>=0.1.0with no explicit[tool.ruff.lint] selectmeansruff checkhere answers differently depending on the day it runs. ruff 0.16.0 grew its
DEFAULT rule set from 59 rules to 413 and this project selected no rules, so it
silently inherited 354 of them. Verified against this tree:
ruff 0.14.x All checks passed!
ruff 0.16.0 Found 103 errors.
This repo has no test/lint CI workflow, so nothing would have caught it; the
only consumer of the linter here is a developer running it locally, which is
exactly the case an unbounded range makes unreproducible.
Real defect (1):
VectorIndex.get_vectorscaught every exception fromfaiss.reconstruct_n(...)andpassed, falling through toreturn None.Noneis this method's DOCUMENTED answer for "FAISS-only storage, nothing tohand back", so a failed reconstruction was indistinguishable from the normal
empty case and a caller would conclude the index simply had no vectors. It now
catches what FAISS actually raises (
RuntimeError,AttributeError) and logsa warning naming the vector count and the exception type. The return value is
unchanged for every input that previously succeeded.
Fixed in code (no behaviour change): UP045/UP007/UP006/UP035 (76)
Optional[X]and
Union[...]to PEP 604,Type/Tupleto builtins -- all valid at thedeclared
requires-python = ">=3.10"; I001 (9) import ordering; F401 (5) unusedimports; PIE790 (5)
passafter a docstring; RUF022 (2)__all__sorting;SIM102 (2) two nested
ifs collapsed in_compute_context_bonus; SIM117 (1)nested
withcollapsed in a test.Disabled deliberately (1, narrowly):
src/openadapt_retrieval/__init__.pyonly, viaper-file-ignores with the reasoning inline. That
__all__is grouped bysubsystem with
# Embedders/# Retriever/# Storageheadings, and analphabetical sort strands all three comments against unrelated names. The
grouping documents the package layout and is worth more than the ordering.
Every other
__all__in the package was sorted.No
selectwas added. After the above the package clears the whole 413-ruledefault set, so narrowing it would discard coverage it is earning today --
including the S110 rule that surfaced the defect. Predictability comes from
bounding
ruff>=0.16,<0.17instead: ruff only grows its default set in a minorrelease, so the rule set cannot move until a person raises that ceiling and owns
the new findings in the same change.
target-version = "py310"is now statedrather than inferred.
uv.lock is relocked so ruff actually resolves 0.16.0; without that the bound is
inert for anyone running
uv sync. The lock rewrite is large because uv alsoupgraded the lockfile format (revision 1 to 3, adding upload-time metadata);
no dependency version changed except ruff, and the local package version was
corrected from a stale 0.1.0 to the declared 0.1.2.
28 passed, 1 skipped (3 gpu/slow deselected, as before).
uv run ruff check .is clean under the locked ruff 0.16.0.
Note:
src/**/__pycache__/*.pycandtests/__pycache__/*.pycare committed tothis repo and were left untouched here. They are stale build artifacts that do
not belong in version control and should be removed separately.
🤖 Generated with Claude Code
Co-Authored-By: Claude Opus 5 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_01NyCHrzA1psrKMFfroYbzaM