Windows & performance fixes - #61
Conversation
torchcodec 0.11.0 (resolved by uv from uncapped >=0.9.1) requires torch 2.11, but Windows is pinned to 2.7.1+cu128. The DLL mismatch causes an OS-level 'Entry Point Not Found' dialog for aoti_torch_aten_narrow before Python can catch the ImportError. No Windows wheels exist for any torchcodec version compatible with torch 2.7.x (0.3–0.5 are Linux/macOS only). audio_duration.py already has soundfile, mutagen, and ffprobe as fallbacks, so torchcodec is not needed on Windows. Linux keeps torchcodec<=0.10 (compatible with torch 2.10). macOS keeps torchcodec>=0.5 (flexible for torch 2.9+).
check_compatibility() was importing torch (~2GB), lightning, and PEFT unconditionally for every CLI invocation, including lightweight commands like convert-sidecars that only need json+pathlib. On Windows, repeated invocations accumulated unreleased working set pages, eventually exhausting all 128GB RAM. Now lightweight subcommands (convert-sidecars, dataset, tags, settings, history) skip the compatibility check entirely.
The subcommand was fully implemented (argparse parser, dispatcher, handler) but missing from the gate check, causing the CLI to launch the wizard/GUI instead of routing to the converter.
Duration cache keyed by (path, size, mtime); mp3/m4a/aac read via mutagen to kill libmpg123 stderr spam; dataset/audio scans via asyncio.to_thread so a full library scan no longer freezes the GUI. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…dec markers - training subprocess no longer writes .pyc files, preventing stale bytecode after mid-session code updates (from 6eebbcc) - requirements.txt now carries the same platform markers for torchcodec as pyproject.toml (omission in the original torchcodec fix) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe changes add Electron destination allowlisting, platform-specific torchcodec constraints, revised Windows training defaults, cached audio duration probing, threaded GUI scans, a subprocess environment setting, and lightweight CLI handling for ChangesNavigation Protection
Platform Runtime Behavior
Asynchronous Library Scanning
Lightweight CLI Handling
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@sidestep_engine/data/audio_duration.py`:
- Around line 23-25: Replace the unbounded _DURATION_CACHE with a bounded LRU
keyed by (path, size, mtime_ns), preserving successful duration entries and
caching dur == 0 results so failed probes are not repeated for the same key.
Update the duration lookup and insertion logic near the cache definition and
lines 66-68 to enforce the configured capacity while retaining existing duration
behavior.
In `@sidestep_engine/gui/server.py`:
- Line 447: Add concrete return annotations to the new route handlers
list_datasets and the handler defined immediately after it, using the response
type they return (such as JSONResponse) to satisfy the project's annotation
policy.
In `@sidestep_engine/training_defaults.py`:
- Around line 141-144: Remove the Windows-specific override in the advanced
training UI flow that converts positive num_workers values to 0. Preserve the
DEFAULT_NUM_WORKERS, DEFAULT_PREFETCH_FACTOR, DEFAULT_PIN_MEMORY, and
DEFAULT_PERSISTENT_WORKERS values from training_defaults.py so GUI-configured
runs pass these defaults through to the trainer without disabling prefetching or
persistent workers.
In `@train.py`:
- Around line 319-326: Update the exception handling around
check_compatibility() in the non-lightweight subcommand path so unexpected
failures are no longer silently discarded. Preserve the non-fatal CLI behavior,
but log the caught exception with traceback, or narrow the handler to only
expected compatibility errors.
- Around line 144-146: Update _is_lightweight_subcommand so it identifies only
the parsed subcommand, rather than matching every token in sys.argv and option
values. Use the existing positional parsing or an argv parser that consumes
option arguments, preserving lightweight behavior for actual lightweight
commands while ensuring values such as “history” or “convert-sidecars” after
training options do not bypass compatibility checks. Add a regression test
covering these training invocations.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: c5774b37-d15e-49be-94bd-cecb02c9c2ca
📒 Files selected for processing (9)
frontend/electron/main.jspyproject.tomlrequirements.txtsidestep_engine/core/trainer.pysidestep_engine/data/audio_duration.pysidestep_engine/gui/server.pysidestep_engine/gui/task_manager.pysidestep_engine/training_defaults.pytrain.py
| # Duration cache keyed by (path, size, mtime_ns). GUI dataset scans probe | ||
| # every file on every call; durations only change when the file does. | ||
| _DURATION_CACHE: dict = {} |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Bound the cache and retain failed probe results.
This dict grows forever as files change or disappear, while dur == 0 entries repeatedly execute every fallback—including a possible 10-second ffprobe call—on each scan. Use a bounded LRU keyed by (path, size, mtime_ns) and cache zero results too (or apply a short TTL for negative results).
Also applies to: 66-68
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sidestep_engine/data/audio_duration.py` around lines 23 - 25, Replace the
unbounded _DURATION_CACHE with a bounded LRU keyed by (path, size, mtime_ns),
preserving successful duration entries and caching dur == 0 results so failed
probes are not repeated for the same key. Update the duration lookup and
insertion logic near the cache definition and lines 66-68 to enforce the
configured capacity while retaining existing duration behavior.
| # length of a full library scan (~7 s for 2.5k files). | ||
|
|
||
| @app.get("/api/datasets") | ||
| async def list_datasets(tensors_dir: str = ""): |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add return annotations to the new route handlers.
Ruff reports missing annotations for both handlers. Add the concrete response type used by these routes, such as -> JSONResponse, to keep the new endpoints consistent with the project’s annotation policy.
Proposed fix
- async def list_datasets(tensors_dir: str = ""):
+ async def list_datasets(tensors_dir: str = "") -> JSONResponse:
- async def list_all_datasets():
+ async def list_all_datasets() -> JSONResponse:Also applies to: 452-452
🧰 Tools
🪛 Ruff (0.16.0)
[warning] 447-447: Missing return type annotation for private function list_datasets
(ANN202)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sidestep_engine/gui/server.py` at line 447, Add concrete return annotations
to the new route handlers list_datasets and the handler defined immediately
after it, using the response type they return (such as JSONResponse) to satisfy
the project's annotation policy.
Source: Linters/SAST tools
| DEFAULT_NUM_WORKERS: int = 2 if sys.platform == "win32" else 4 | ||
| DEFAULT_PREFETCH_FACTOR: int = 2 | ||
| DEFAULT_PIN_MEMORY: bool = True | ||
| DEFAULT_PERSISTENT_WORKERS: bool = sys.platform != "win32" | ||
| DEFAULT_PERSISTENT_WORKERS: bool = True |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Remove the UI override that disables these Windows defaults.
The advanced UI flow still forces every positive Windows num_workers value to 0 (sidestep_engine/ui/flows/train_steps_advanced.py:372-382). Consequently, these defaults never reach the trainer for GUI-configured runs, and config_factory.py disables prefetching and persistent workers again.
- if IS_WINDOWS and a["num_workers"] > 0:
- print_message("Warning: Windows detected -- forcing num_workers=0", kind="warn")
- a["num_workers"] = 0
+ if IS_WINDOWS and a["num_workers"] > 4:
+ print_message("Warning: Windows workers capped at 4 by the trainer", kind="warn")🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@sidestep_engine/training_defaults.py` around lines 141 - 144, Remove the
Windows-specific override in the advanced training UI flow that converts
positive num_workers values to 0. Preserve the DEFAULT_NUM_WORKERS,
DEFAULT_PREFETCH_FACTOR, DEFAULT_PIN_MEMORY, and DEFAULT_PERSISTENT_WORKERS
values from training_defaults.py so GUI-configured runs pass these defaults
through to the trainer without disabling prefetching or persistent workers.
| def _is_lightweight_subcommand() -> bool: | ||
| """True when the CLI targets a subcommand that needs no heavy imports.""" | ||
| return bool(_LIGHTWEIGHT_SUBCOMMANDS & set(sys.argv[1:])) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Inspect only the actual subcommand, not option values.
_LIGHTWEIGHT_SUBCOMMANDS & set(sys.argv[1:]) misclassifies valid commands such as train --dataset-dir history or train --output-dir convert-sidecars, causing compatibility checks to be skipped for training. Derive the subcommand from the parsed positional value or an argv parser that understands option arguments, and add a regression test for this case.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@train.py` around lines 144 - 146, Update _is_lightweight_subcommand so it
identifies only the parsed subcommand, rather than matching every token in
sys.argv and option values. Use the existing positional parsing or an argv
parser that consumes option arguments, preserving lightweight behavior for
actual lightweight commands while ensuring values such as “history” or
“convert-sidecars” after training options do not bypass compatibility checks.
Add a regression test covering these training invocations.
| # Skip for lightweight subcommands to avoid importing torch/PEFT/Lightning | ||
| # (~2-3 GB RAM) when all we need is json + pathlib. | ||
| if not _is_lightweight_subcommand(): | ||
| try: | ||
| from sidestep_engine._compat import check_compatibility | ||
| check_compatibility() | ||
| except Exception: | ||
| pass # never let the compat check itself crash the CLI |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
Do not silently suppress compatibility-check failures.
check_compatibility() already handles expected import failures; the outer except Exception: pass hides unexpected bugs and removes diagnostic context. Log the exception with traceback while preserving non-fatal behavior, or narrow the exception type.
🧰 Tools
🪛 Ruff (0.16.0)
[error] 325-326: try-except-pass detected, consider logging the exception
(S110)
[warning] 325-325: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@train.py` around lines 319 - 326, Update the exception handling around
check_compatibility() in the non-lightweight subcommand path so unexpected
failures are no longer silently discarded. Preserve the non-fatal CLI behavior,
but log the caught exception with traceback, or narrow the handler to only
expected compatibility errors.
Source: Linters/SAST tools
1 participant
Windows & performance fixes
A bundle of small platform and performance fixes, independent of each other and of everything else we'll be submitting.
pyproject.tomlnow carries platform markers (linux/darwin only);requirements.txtmirrored to match.--help,settings,history,convert-sidecars, …) no longer import torch/PEFT/Lightning — startup drops from ~10s to well under a second for those paths.convert-sidecarsadded to the known-subcommand gate set.(path, size, mtime); mp3/m4a/aac read via mutagen header parse instead of libsndfile's embedded mpg123, which was both slower and spamming stderr withlibmpg123 id3.cerrors on every scan.asyncio.to_thread— a full-library scan (~7s for 2.5k files) previously froze every other GUI request.num_workers=0with a cap of 4 plus prefetch and persistent workers — a large real-world preprocessing/training throughput win on Windows.PYTHONDONTWRITEBYTECODE=1so mid-session code updates can't be shadowed by stale.pycfiles./theme-editorpage (was blocked by the nav allow-list).All Python files compile clean; GUI smoke-tested on Windows 11.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
convert-sidecarscommand.Bug Fixes
Performance