fix: SamSegmenter crashed on every run — checkpoint=None - #12
Merged
Conversation
SamSegmenter passed `checkpoint=self.checkpoint` unconditionally. samgeo
runs `os.path.exists(checkpoint)` as soon as that key is present, so the
default `None` raised
TypeError: stat: path should be string, bytes, os.PathLike or integer,
not NoneType
before a single mask was generated. The SAM backend has therefore never
worked; omitting the key entirely makes samgeo download the default
weights for the model type, which is what was intended.
The bug survived because CI cannot see it. tests/test_sam.py opens with
`pytest.importorskip("samgeo")` and CI installs neither the sam nor the
cellpose extra, so the only test covering this path has always skipped.
It surfaced the moment the extras were installed locally.
The regression test therefore goes in tests/test_make_segmenter.py, which
deliberately runs without the heavy dependencies, rather than next to the
test it fixes — otherwise it would skip in CI too and guard nothing. It
asserts the key is absent when unset and forwarded as a string when set.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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.
The SAM backend has never worked. Found by installing the
samextra locally for the first time.The bug
SamSegmenter.segmentpassedcheckpoint=self.checkpointunconditionally. samgeo's constructor:With the default
checkpoint=Nonethis takes the first branch and raises before a single mask is generated:Omitting the key entirely takes the
elsebranch, which downloads the default weights for the model type — the intended behaviour.Why it survived
tests/test_sam.pyopens withpytest.importorskip("samgeo"), and CI installs neither thesamnor thecellposeextra. The only test covering this path has always skipped.v0.3.0ships the backend as done, and nothing has exercised it since.Where the regression test lives
In
tests/test_make_segmenter.py, not beside the test it fixes. That module deliberately runs without the heavy dependencies — its existing test asserts that constructing the backends does not import samgeo/cellpose/torch. Putting the new test intest_sam.pywould have made it skip in CI too, guarding nothing.It asserts both directions: the key is absent when unset, and forwarded as a string when set.
A related gap, not fixed here
Installing the extras un-skips two tests that CI never runs. That is a standing blind spot rather than a one-off: any breakage in the SAM or Cellpose path is invisible until someone installs several GB of torch by hand. Worth deciding separately whether CI should carry that cost, or whether these backends need smoke tests that stub the heavy call.
Verification
ruffclean,mypyclean on 26 files,test_make_segmenter.py4 passed in 0.01 s — with the extras absent, which is the condition CI runs under.🤖 Generated with Claude Code