Refactor repository - #6
Merged
Merged
Conversation
…geometry.py (parallel view)
…swai logs, removed blankspaces
Merge from sidewalk_width
Commented lines regarding optional runtime overrides variables
Heading selection, obstacle extraction, API/CLI request and exception handling
…nction and whole cli.py
…enter and _predict_depth_without_logo
… 4 captures each side)
… now rely on --debug args
- Updated refinement.py to improve debug logging and code readability. - Enhanced test cases in test.zoe.py, test_fusion.py, test_geometry.py, test_pipeline_mock.py, and test_refinement.py for better coverage and clarity. - Introduced test_package_exports.py to verify package exports. - Improved webapi.py for better logging and code organization. - Adjusted type hints and annotations across various modules for consistency.
…ization and import handling in the codebase
Review of the previous refactor surfaced a crash, several silently wrong
measurements, and four public parameters that did nothing. Entries marked
[numbers] change pipeline output: existing measurements need recomputing.
Correctness
- compute_width: fix UnboundLocalError in the band-retry pass, where the
bridge-fill block lost an indentation level in the previous refactor and
raised on any frame whose rows were all rejected by the du gate.
- compute_clearances [numbers]: measure the sidewalk span directly on the
mask at the obstacle's base row. The previous code inverted the fitted
top/bottom envelopes (x = (y - b)/m), which are near-horizontal, so every
clearance collapsed to zero on perpendicular views and was measured
against the full image width on oblique ones.
- EnsembleSegmenter [numbers]: pass through the first member's panoptic map.
Returning (mask, None, None, []) made the pipeline skip obstacle
extraction, so ensemble runs always reported rating III / meets_ratio 1.0.
- Pipeline: raise TypeError when a segmenter returns an unexpected arity
instead of leaving names unbound and failing later as NameError.
Dead knobs, now wired
- refine: stored and never read; refine_sidewalk_mask always ran. Added
--refine/--no-refine to the CLI.
- divergence_pct [numbers]: divergence between the depth and geometry paths
was computed, logged and discarded. Now switches to geometry above the
threshold, as WIDTH_PARAMS documented. Pass None to disable.
- force_fallback/fallback_scale [numbers]: wrote SWAI_* environment
variables that nothing read, and leaked between API requests. Replaced by
an explicit DepthScale argument threaded through the analyse_* methods.
- MiDaS [numbers]: is_metric was only logged. MiDaS emits disparity,
consumed as if it were metres. Now inverted to relative depth and scaled
by a ground-plane RANSAC fit (estimate_ground_scale/to_metric_depth) with
the fallback constant as backup. Not validated end to end here: timm is
not installed in this environment.
- --ensemble-method majority was accepted by argparse and rejected by the
ensemble. Fusion now delegates to logical_fuse, which supports all three.
Determinism
- Seed the RNG in compute_width and fit_line_ransac (default 0, None opts
out). Identical inputs previously spread across ~2% of the width.
- Derive obstacle colours from blake2b rather than hash(), which is salted
per process, and order ensemble label synonyms deterministically.
CLI
- play.py: extract main(argv) with a __main__ guard; the module ran the
whole pipeline at import time. Register the sidewalk-ai console script.
- --image runs now forward --pitch/--fov [numbers]; they were dropped, so
local files were analysed with pitch=0 while the argparse default is -10.
- --help crashed with UnicodeEncodeError on a cp1252 Windows console.
- --seg a+b+c built only the first two back-ends; deeplab inside an
ensemble raised KeyError('ckpt_path').
- Add the public SidewalkPipeline.analyse_image so callers stop reaching
for the private _analyse_path.
Web API
- Cache pipelines per (depth, variant, refine) instead of overwriting the
shared entry on a zoe_variant request, which leaked one caller's variant
into every later request and rebuilt the model each time.
- Serialise model work behind SWAI_API_MAX_CONCURRENCY (default 1) with a
queue timeout: sync endpoints run concurrently in the thread pool.
- Make CORS origins configurable, log swallowed accessibility failures, and
migrate Field(example=...) off the Pydantic v2 deprecation.
- Document the API in docs/webapi.md; it had no documentation at all.
Quality gates
- black and pytest were both red at HEAD; both are green now.
- Enforce the offline test policy: gpu/network markers are deselected by
default, so heavy checks stay out of the default suite by construction.
- Add .github/workflows/checks.yml running the gates on 3.11 and 3.13.
- scripts/check.ps1 reported success with red gates: $ErrorActionPreference
does not stop on native exit codes in Windows PowerShell 5.1.
- Tests 7 -> 71, including a 1000-scenario characterization test pinning
compute_width.
Structure
- Deduplicate the compute_width scan loop (~140 duplicated lines, the source
of the crash above) into _scan_band, verified against 2040 recorded
scenarios with zero differences. One asymmetry is preserved deliberately:
the retry pass never applied the soft Z clamp, now an explicit parameter.
- Remove dead code: core/config.py, project_line_to_ground,
_largest_dense_cluster, aggregate_headings, MAX_INST_FRAC, the
commented-out block at the end of geometry.py, and a duplicated regex.
- Move label parsing to sidewalk_ai/labels.py so io no longer imports a
private helper from processing.
- Stop writing debug_5_twoline.png into the working directory, and keep the
default debug dir out of site-packages.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
io.streetview.Settings reads GOOGLE_API_KEY, which has no SWAI_ prefix, so its model_config points env_file at ".env" directly instead of scoping through env_prefix like the sibling Settings classes do. pydantic-settings' default extra="forbid" then rejected every unrelated key the same .env file carries for those other classes (SWAI_DEBUG, SWAI_LOG_LEVEL, SWAI_DEPTH, SWAI_IMG_*, ...), so importing sidewalk_ai.io.streetview raised ValidationError before the CLI or API did anything. This was invisible in my own environment, which only ever had GOOGLE_API_KEY in its .env, and wasn't caught by the existing tests, which construct Settings() with explicit kwargs rather than loading a file. It surfaces the moment someone follows the README's own setup instructions: copy .env.example to .env and fill it in. Reported by the user after cloning to a second machine. Fixed by adding extra="ignore" to that Settings class, and added a regression test that loads the actual .env.example content (with the API key filled in) to catch this shape of bug rather than a hand-picked one. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
`pip install -e ".[ml]"` followed by the README's own example command failed
with ModuleNotFoundError: No module named 'timm'. ZoeDepth builds its core
through torch.hub("intel-isl/MiDaS"), whose hubconf imports timm, so the
default --depth zoe cannot start without it, yet nothing declared it.
The version window is narrow and had to be pinned exactly:
- timm 1.x renamed the BEiT block internals (drop_path -> drop_path1/2) that
MiDaS calls directly, so inference dies with AttributeError.
- timm 0.6.12 and older fail to import on Python 3.11+, which this project
requires, so ZoeDepth's own environment.yml pin cannot be used as-is.
- 0.6.13 is the only release satisfying both.
Installing it then exposed two real bugs in models/zoe.py, both of which had
been masked:
- The checkpoint was loaded whole. ZoeDepth publishes *training* checkpoints
with the parameters under a "model" key next to "optimizer" and "epoch", so
`load_state_dict(state, strict=False)` matched nothing: 511 missing keys and
no exception. That call has been a silent no-op all along, which also means
the documented `ckpt_path` option for user checkpoints never loaded anything.
Now unwrapped the same way zoedepth.models.model_io does, DataParallel prefix
included.
- torch.hub.load passed pretrained=True directly under a comment reading "build
architecture only", so ZoeDepth loaded the official checkpoint itself, with a
strict load that timm 0.6.13 survives but timm >= 1.0 does not. Since the
weights are applied by the code below, this was a redundant second download.
Now pretrained=False, matching the stated intent.
Because strict=False is now the only load, it is checked rather than trusted:
any missing key raises instead of silently yielding a randomly initialised
network. That guard is what caught the unwrapping bug.
Validated end to end on a real image, closing the gap flagged when the MiDaS
scale recovery went in: `--depth zoe` gives 4.28 m and `--depth midas` 5.26 m
on the same frame, the latter via a recovered ground-plane scale (alpha=2.25,
source=ground). Before that work MiDaS reported raw disparity units as metres.
Also documents the OneFormer load report that recent transformers prints. It is
benign: the relative_position_index entries are non-persistent buffers, and the
missing swin.layernorm is discarded by OneFormer, which consumes the per-stage
feature maps rather than the Swin sequence_output. Multiplying those weights by
7 leaves the predicted masks bit-identical.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two problems the --debug run on a fresh checkout exposed. Accessibility output died silently on Windows. The single- and multi-view summaries printed U+2265, U+2248 and U+2192, none of which exist in cp1252, so the print raised UnicodeEncodeError inside the try block that wraps the whole accessibility section. The result: the NBR 9050 rating -- the headline metric -- never appeared on the project's primary platform, leaving only a logged warning. Replaced with ">=", "~" and "->"; the other symbols in that output (+/- and the degree sign) are cp1252-safe and stay. stdout/stderr are also reconfigured with errors="replace" so a future slip degrades to "?" instead of dropping a section. Debug images ignored --outdir. write_debug_sheet saved the composite under args.outdir but wrote the individual panoptic/depth PNGs to _project_root(), i.e. the repository root. That is what scattered streetview_*.png across the checkout. With an editable install whose target differs from the current directory it is worse: a run inside one checkout wrote into another. They now go to outdir with the composite, and _project_root() is gone with its last caller. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`--device cuda` on a CPU-only PyTorch build surfaced as
AssertionError: Torch not compiled with CUDA enabled
thrown from inside Module.to(), after OneFormer's weights had already been
downloaded and moved. Three of the four adapters take `device or (auto)`, so an
explicit "cuda" bypassed every availability check; MiDaS did the reverse and
silently forced CPU, ignoring whatever the caller asked for.
Worse, --device defaulted to "cuda", so the plain documented command crashed on
any machine without a CUDA build -- which is what `pip install -e ".[ml]"` gives
you unless you install PyTorch separately.
--device now takes auto|cuda|cpu and defaults to auto: CUDA when the installed
build can, CPU otherwise, logged either way. Naming cuda explicitly still fails
when it cannot, but as a usage error naming the torch build and pointing at
--device cpu and pytorch.org, in milliseconds rather than after a multi-gigabyte
download. MiDaS now matches the other adapters: None auto-selects, an explicit
device is honoured.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The README delegates backend installation to upstream, which is fine, but Detectron2 on Windows has no wheels and two failure modes that cost real time to diagnose: - Selecting a CPU-only extension build needs CUDA_VISIBLE_DEVICES=-1. Clearing CUDA_HOME/CUDA_PATH/FORCE_CUDA does not work, because on Windows torch discovers CUDA_HOME by globbing the toolkit directory. With a toolkit that does not match the one torch was built against (12.8 against cu118 here) the build aborts on a version mismatch. - setuptools >= 81 removed pkg_resources, which detectron2 0.6 imports, so model_zoo fails at import with a misleading ModuleNotFoundError. Verified end to end on Windows 11 / Python 3.13 / torch 2.6.0+cu118: single view on CPU and CUDA agree, --debug writes its sheets, and the oneformer+detectron2 ensemble reports obstacles rather than the empty list that used to make every ensemble run look perfectly accessible. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Unlike OneFormer and Detectron2 this backend has no one-command install: upstream ships no setup.py, publishes weights as Dropbox/Drive links, and has no Hugging Face mirror. Records the pinned-checkout plus venv-scoped .pth recipe, and why that is preferable to pip installing the checkout, whose find_packages() would put datasets/utils/metrics into site-packages and shadow real distributions -- Hugging Face's `datasets` among them. Also records a caveat that matters for cross-backend comparison. On three sample frames DeepLab estimates width normally but returns zero obstacles, while OneFormer finds a tree on the sidewalk in the same frame. Not a tunable threshold: the vegetation region has no pixel adjacent to DeepLab's sidewalk mask. It follows from the Cityscapes label set -- what OneFormer calls `grass` is `terrain`, which _obstacles.py ignores as ground, and semantic segmentation merges every tree into one region. Since zero obstacles resolves to meets_ratio 1.0 and rating III, DeepLab always reports the most favourable accessibility rating, so its figures are not comparable with the other backends. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
load_deeplab_checkpoint filtered the checkpoint down to tensors whose names and
shapes matched the model, then raised only if *nothing* matched. A checkpoint
for a different backbone still matches a handful by coincidence: the mobilenet
Cityscapes checkpoint fills 44 of resnet101's 674 tensors. So the guard passed,
strict=False accepted the rest, and the loader returned a 93% randomly
initialised network.
That is what --deeplab-model defaulting to deeplabv3plus_resnet101 does to the
mobilenet checkpoint most users have: the network segments almost nothing, and
the only symptom is "No sidewalk support for width estimation" and WIDTH 0.00
on most frames -- indistinguishable from a genuinely hard image, and easy to
mistake for a weak pretrained model.
Now every model tensor must be present in the checkpoint, and the error names
the checkpoint, the model_name, how many tensors matched, and what to do:
Checkpoint 'best_deeplabv3plus_mobilenet_cityscapes_os16.pth' does not fit
model_name='deeplabv3plus_resnet101': 44 of 674 tensors matched, 630 would
stay randomly initialised (first missing: ['backbone.conv1.weight', ...]).
Pass the --deeplab-model that matches the checkpoint's backbone.
Same failure mode as the ZoeDepth checkpoint fixed in 715c090: strict=False
without checking what actually landed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
--deeplab-model defaulted to deeplabv3plus_resnet101 regardless of which checkpoint was passed, so the mobilenet weights most users have were loaded into a resnet101 and 630 of 674 tensors stayed random. ea9148e made that fail loudly; this removes the trap. Upstream names its weights after the architecture they belong to (best_deeplabv3plus_mobilenet_cityscapes_os16.pth), so with no --deeplab-model the architecture is now read from the filename and logged. An explicit --deeplab-model still wins, and a filename carrying no recognisable pair asks for one rather than guessing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both were swallowed: one as a "Failed to write debug sheet" warning, the other as a debug_plot_error event whose payload is invisible in the text log format. The panoptic overlay indexed its palette with `seg % 256`. Segment ids arrive as uint8 from DeepLab and int32/int64 from OneFormer and Detectron2, and NumPy 2 rejects `uint8_array % 256` outright because 256 does not fit the dtype. So the debug sheet raised OverflowError for DeepLab alone. The map is now normalised to int32 before the lookup. _plot_compute_width_debug imported pyplot without choosing a backend, so matplotlib picked an interactive one and failed with "Can't find a usable init.tcl" on any machine without Tcl/Tk -- a plain virtualenv on Windows, or a CI runner. This affected every back-end, not just DeepLab: the per-frame compute_width diagnostic plot was never produced for anyone. Both plotting sites now force Agg, since they only ever write files. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
An AST sweep of `except Exception` across the package found 26 handlers, 11 of them silent. Three bugs this session hid behind exactly this pattern, so the risky ones are now audible. The one that could change measurements: core.pipeline guarded `from sidewalk_ai.io.image_io import get_google_bar_height_px` with a bare `except Exception: pass` that fell back to 20. That value becomes bottom_ignore_px for compute_width, so had the import ever failed the width band would have shifted with nothing said -- while guarding an import from this very package, which cannot fail unless the install is broken. Now a plain import. _debug_viz wrote through print(), so eight messages -- including "failed to write" -- bypassed the logging package entirely and were invisible to --log-file and --log-format json. All now go through a module logger. Silent handlers that now say something: segment-info collection and seg_map resizing in _debug_viz (a failed resize leaves the overlay misaligned, which reads as a segmentation fault rather than a plotting one), the image reload for the debug sheet, the obstacle base-mask lookup, the multi-view overlay in api.request, and the obstacle outline in image_io. geometry's debug-plot failure moves from a _swai_log payload -- rendered only by the JSON log format -- to a plain warning, since a user who passed --debug and got no plot should be told. Two silent handlers are left on purpose: _fmt_num returning "N/A" is its contract, and diagnostics records the exception into the report it emits. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every defect that survived the previous refactor and reached a user did so by failing quietly, and the same shapes kept recurring: a broad guard turning a real failure into a plausible-looking result, far from its cause. Writes the rules down so the next change does not reintroduce them: no `except Exception: pass`; do not guard imports of this package's own modules; check what `load_state_dict(..., strict=False)` actually loaded; put the reason in the log message rather than a `debug_event` payload that only the JSON format renders; library code logs while only the CLI prints; and a degraded result needs a signal distinct from a clean one, since "no obstacles found" and "obstacle detection failed" both score as a perfect accessibility rating today. Each rule names the concrete bug behind it. The two deliberately silent handlers are listed so they are not mistaken for oversights. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A review for Linux compatibility, verified against a real Ubuntu 22.04 rather than reasoned about. The package code itself was already portable -- no drive letters, no hardcoded separators, no case collisions, and git normalises line endings on checkout -- so everything below is tooling and documentation. datetime.UTC in log.py was the package's only barrier to Python 3.10: the rest compiles and runs there, checked with python3.10 on Ubuntu. It is a 3.11+ alias for timezone.utc, so it is now spelled the portable way. requires-python stays at >=3.11; dropping the floor is a support-matrix decision, but the code no longer forces it. scripts/ only had PowerShell, so Linux users had no equivalent of the documented helpers. Adds check.sh and setup-dev.sh with the executable bit set, matching their .ps1 counterparts and propagating a failing gate's exit code -- verified on Ubuntu, including the failure path. .gitattributes did not exist. Shell scripts checked out with CRLF fail as "bad interpreter", which is exactly the trap a Windows-primary repository sets for its first Linux contributor; *.sh is now pinned to LF, *.ps1 to CRLF, and the golden JSON to LF so no conversion can rewrite recorded values. README now covers both systems. Linux needs python3-venv and libgl1, neither of which Debian/Ubuntu installs with the interpreter -- all three package names confirmed available on 22.04 -- and Ubuntu 22.04 LTS ships Python 3.10, below the floor, which the setup section now says outright. Commands identical on both shells lost their `powershell` fence, which was implying otherwise; the one that genuinely differs, line continuation, is shown in both forms. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Verified on a real Ubuntu 22.04: with requires-python relaxed, pip resolves the whole dependency set for 3.10 (numpy 2.2.6, scipy 1.15.3, opencv 5.0.0), the suite passes -- 77 passed, 4 skipped, the skips being exactly the 17 tests that need torch, which is not part of the [dev,api] layer -- and ./scripts/check.sh takes all five gates green. The ml extra holds too: timm 0.6.13, which the pin selects, installs on 3.10 and still exposes the BEiT `drop_path` attribute MiDaS calls. The floor was never a deliberate choice. It came from `datetime.UTC` in log.py, a 3.11 alias replaced in eceedb9; nothing else in the package needs 3.11. Keeping it shut out Ubuntu 22.04 LTS, still the most widely deployed Linux, from using its system interpreter. The CI matrix moves from 3.11/3.13 to 3.10/3.13 so the new floor is a tested promise rather than a claim, and black and ruff now target py310 so neither rewrites code into syntax the floor cannot parse. Also fixes a literal "\n" that landed in the README's structured-logging example instead of a line continuation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The README covered both systems after eceedb9 but the three documents it points to for detail were still PowerShell-only, so a Linux reader hit Windows syntax the moment they followed a link. Blocks whose commands are identical in both shells lost the `powershell` fence that implied otherwise. Where the syntax genuinely differs -- virtualenv activation, environment variables, path separators, writing the DeepLab .pth, invoking the helper scripts -- both forms are shown. The five PowerShell blocks left in reproducibility.md are all Windows-specific by nature. "Detectron2 on Windows" becomes "Detectron2", since the section's real subject is that the package builds C++ extensions everywhere. Linux needs build-essential and python3-dev: g++ is present on a stock Ubuntu but Python.h is not, checked on 22.04, and the build fails without it. Verified rather than assumed: all 29 bash blocks across the four documents parse under `bash -n` on Ubuntu, the four referenced helper scripts exist, and the site-packages one-liner for the DeepLab .pth resolves and takes effect on Linux -- site-packages still preceding the checkout in sys.path, which is what makes the shadowing argument in that section true. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The DeepLab back-end was unreachable on any machine without a GPU. The CLI resolved --device correctly and passed it down, but build_segmenter listed the loader's keyword arguments explicitly and "device" was not among them, so the value was handed only to DeepLabSegmenter. load_deeplab_checkpoint then fell back to its own default, a hardcoded "cuda", and moved the model there before the segmenter ever got a say. The symptom was a bare "RuntimeError: No CUDA GPUs are available" raised from model.to(device), with nothing connecting it to the --device cpu the user had just typed. Forward the caller's device by reading it instead of popping it -- both the loader and the segmenter place the model, so both need it -- and default the loader to the same availability check DeepLabSegmenter already used. The existing tests all named device="cpu" explicitly, which is why none of them covered the path the CLI actually takes. Add one test for the loader's default and one asserting the factory forwards the requested device. Ignore checkpoints/, where the DeepLab Cityscapes weights are fetched.
The README's own first example could not run non-interactively. torch.hub asks for confirmation the first time it caches a GitHub repo unless trust_repo is set, so ZoeDepthEstimator died as a bare "EOFError: EOF when reading a line" raised from inside torch.hub, naming neither ZoeDepth nor trust. Since --depth zoe is the default, that was the first thing a new machine hit. Pass trust_repo=True. The repo is not caller-supplied: the github branch hardcodes isl-org/ZoeDepth, and repo_or_path only takes effect for source=local. Three tests cover the flag, the hardcoded repo, and the local path, with torch.hub.load replaced by a spy so nothing is cloned or downloaded. The docs said ZoeDepth had to be "installed or cloned according to the target environment", the one non-actionable line in the install section. It needs neither: torch.hub fetches it. Replace that with a section saying so, including why diagnostics reports zoedepth_local missing until the cache is warm. The README also assumed more than a new Linux machine offers: - "Install the CPU or CUDA build that matches your machine" gave no way to tell which. Name nvidia-smi as the test, and the two false positives that led this astray: an nvcc from nvidia-cuda-toolkit and a libcuda.so from libnvidia-compute-* with no GPU under either. - Two of the three examples pass --device cuda with no note that it fails hard off a GPU, and that --device auto is the default. - Nothing mentioned that the first run pulls ~3 GB of weights, or that a frame costs tens of seconds on CPU. Both are measured now. - Detectron2's toolchain prerequisite was only in docs/reproducibility.md. Mention it where the ML layer is installed. Correct one claim there while at it: a stock Ubuntu 22.04 desktop had no g++, not just missing headers, and the second failure only surfaces after the first is fixed.
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
2 participants
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.
Refactor whole repo for enhanced modularity, reproducibility, architecture, clean code, documentation and compatibility with Linux-based systems. Also added plenty test cases.