Skip to content

Auto-labeling cleanup: remove what the fast slices left behind #464

Description

@JArmandoAnaya

The interactive auto-labeling feature is stable. It was built across many fast slices — cf. #418, #421, #424, #425, #456, #457, #461 — and this is the inventory of what those slices left behind: temporary code, scaffolding a later slice obsoleted, redundant tests, and comments that no longer describe the code they sit above.

No behaviour changes. Every item below is either a removal with zero references, a test deletion with a named surviving test proving the same failure mode, or a comment correction where the code is the authority. Anything that could not be proven safe is recorded in the second half and left alone.

Surfaces walked: src/visionset/inference/**, the inference parts of src/visionset/kernel/**, src/visionset/server/routes/inference.py, src/visionset/cli/inference.py, the frontend suggest and Inference-section code, and every test covering them. src/visionset/mcp/ has no inference surface, so it is out of scope by absence rather than by choice.


LOW risk — will do

Redundant tests

Three tests in tests/inference/test_provider.py (#434) were re-created in tests/inference/test_providers.py (#458) when the family resolver got its own file. The originals were never removed. Each deletion below names the surviving test that turns red under the same mutation.

Deleted Surviving test that covers the same failure
test_provider.py::test_an_http_connection_is_refused_because_this_build_has_no_adapter test_providers.py::test_an_http_connection_is_refused_because_this_build_has_no_adapter
test_provider.py::test_a_local_connection_without_weights_is_refused_by_state test_providers.py::test_a_connection_without_weights_is_told_which_action_fixes_it
test_provider.py::test_the_connections_own_state_is_reported_before_the_machines test_providers.py::test_a_missing_runtime_is_reported_after_the_connections_own_state

The first pair share a name character for character. In all three the survivor is the stronger test: it drives provider_for through a real InferenceConnectionService rather than a hand-built model, and matches a longer substring of the message.

The third is worth naming separately, because it does not test what its name claims. It says the connection's own state is reported before the machine's, but it stubs nothing — it calls provider_for on a not-set-up connection and asserts InferenceConnectionNotSetUp, which is exactly the second row's assertion. On a machine that has the extra installed there is no second refusal for it to be ordered against. The survivor stubs require to raise and then asserts both directions, which is the ordering claim actually made.

tests/inference/test_provider.py keeps everything unique to it: the unstubbed missing-extra refusal, the isinstance-against-Protocol check, the two model_ref cases, and the whole regions_from half — including #434's named mutation test, which is untouched.

Dead parameter

  • src/visionset/inference/sam_provider.py:90best_of(iou_scores, masks) never reads masks. The choice is made entirely from the IoU head's scores. Flagged by ruff --select ARG; three call sites, all in this repository. Not part of the ModelProvider port and not in visionset.inference.__all__.

Dead export

  • frontend/ui-core/src/data/inferenceQueries.ts:96export type ConnectionAction has no importers. Its three neighbours on the same lines (ConnectionType, ConnectionSetupState, DownloadSizeOut) are each re-exported from frontend/ui-core/src/index.ts; this one is not, so it is not package API either. Anything needing it already has it through Connection["allowed_actions"].

Comments that no longer describe the code

  • pyproject.toml:66"uv.lock does not carry extras, so nothing here is resolved by a uv sync in this repository." The lockfile does carry them: uv.lock:2598 declares local-inference under [package.optional-dependencies], 26242635 carry all five requirement specifiers, and 2639 declares provides-extras. This matters beyond pedantry — a reader who believes it has no reason to re-lock after editing those lines, and uv lock --check then fails on the stale result. cf. #459.
  • frontend/ui-core/src/annotator/AnnotationPage.tsx:374onConfigureInference is documented as "expected to be absent for now: the Inference surface waits on Inference section — user-configured model connections #421's open rail question". That surface shipped, and frontend/app/src/routes.tsx:331 passes the prop. The prop is still legitimately optional — ui-core is embeddable by a host that has no such screen — so only the sentence about it being absent is wrong.
  • src/visionset/inference/__init__.py:16 and tests/architecture/test_optional_runtime.py:6 — two ragged docstring wraps left by the edit that added torchvision to the enumerations. Cosmetic; ruff does not reflow docstrings, which is why they passed every gate.

HIGH risk — recorded, not acted on

A defect, not scaffolding

SuggestRequest.detail is accepted on the wire, validated, published in openapi.json, and has no effect. Filed as #463 so it is not buried in a cleanup issue.

The chain breaks at one link. server/routes/inference.py:236 passes detail into suggest(); inference/suggestions.py:52 accepts it and never reads it. The value that actually reaches polygon_from is LocalSamProvider._detail, fixed at construction (sam_provider.py:128), and providers._local() builds the adapter without passing one — so it is always DEFAULT_DETAIL. A client sending detail: 0.9 gets the same polygon as one sending detail: 0.01.

Not a cleanup item in either direction. Deleting the parameter removes a published field; making it work is a real change, because the provider is pooled and shared across requests keyed on connection identity, so detail has to travel per-request rather than be set on the instance. No test covers the field reaching anything, which is why it shipped.

Left alone

Item Why it stays
visionset.inference.__all__ exports EXTRA, INSTALL_COMMAND, DEFAULT_SIZE_CAPACITY with no importers anywhere Removing a name from a package's __all__ changes its public API surface, whatever the current reference count says
masks.spans() could be derived from masks.runs() The one genuine consolidation candidate, and they are not identical helpers — spans collapses each row to its outer extent, which is what bbox_from wants and is cheaper. Equivalent for a bounding box, not proven equivalent in general, and it would edit #461's fresh code
tests/inference/test_provider.py and test_providers.py differ by one letter A rename is not a removal, and it would move tests named as mutation verification
Every test named as mutation verification in #430, #434, #448, #451, #454, #458 and the #461 fix Protected
#458's unknown-model-type and empty-model-type refusal tests; the #461 blob-selection tests Protected — they encode fresh, paid-for lessons
The allowed_actions parity suite, the import-linter contracts, the annotator core boundary gates, the base-import-graph test, the extra↔guard parity test Protected
Unit + contract + e2e coverage of the same behaviour Intentional design, not redundancy — layers are not collapsed

What the sweep did not find

No TODO/FIXME/HACK markers on any feature surface. No commented-out code. No .tmp, .bak or spike-residue files, tracked or untracked. No unreachable branches. ruff --select ERA,ARG,PIE,RUF,F401,F841,B007 over every backend feature module reports exactly the two unused parameters above and nothing else.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    backendvisionset Python distribution — kernel, server, cli, mcp, formatsdocumentationImprovements or additions to documentationfrontendannotator / ui-core / app packages

    Type

    No type

    Projects

    No projects

      Milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions