Conversation
Refactor check_install.py to support both PyTorch and TensorFlow backends and improve temporary file handling. Introduces TMP_DIR, MODELS_FOLDER, and separate run_pytorch_test/run_tensorflow_test helpers; PyTorch now exports and benchmarks an exported .pt checkpoint, TensorFlow model download logic is preserved. Replaces --nodisplay with --display, centralizes video download and assertions, tightens error handling for downloads, and ensures proper cleanup of temporary files. Also updates imports (urllib.error, export_modelzoo_model) and updates backend availability checks to require at least one backend.
Expose a single_animal flag on benchmark_videos (default False) and forward it to the underlying analysis call. This allows benchmarking to run in single-animal mode when using DeepLabCut-live exported models.
Update .github/workflows/testing.yml to run the Model Benchmark Test with --display instead of --nodisplay. This allows dlc-live-test to run tests that require a display (e.g., visual/benchmarking checks) in the CI workflow.
There was a problem hiding this comment.
Pull request overview
This pull request refactors the DLC-Live installation check script to support both TensorFlow and PyTorch backends, while also applying code style improvements to the benchmarking module. The changes enable backend-agnostic testing by detecting available backends and running appropriate tests for each.
Changes:
- Added PyTorch backend support to check_install.py with new
run_pytorch_test()andrun_tensorflow_test()functions - Changed argument parsing from
--nodisplayto--display, reversing the default display behavior (breaking change) - Applied code formatting and import organization improvements throughout benchmark.py
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 10 comments.
| File | Description |
|---|---|
| dlclive/check_install/check_install.py | Refactored to support both TensorFlow and PyTorch backends with separate test functions, added backend detection loop, and improved error handling |
| dlclive/benchmark.py | Applied code formatting improvements, reorganized imports with TYPE_CHECKING, added single_animal parameter to benchmark_videos, and corrected spacing/line wrapping throughout |
Comments suppressed due to low confidence (1)
dlclive/benchmark.py:45
- This import of module os is redundant, as it was previously imported on line 16.
import os
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Refactor test startup and error handling for check_install and simplify a type-only import. - dlclive/benchmark.py: replace the try/except tensorflow import under TYPE_CHECKING with a direct import (type ignored) to simplify typing logic. - dlclive/check_install/check_install.py: - Defer importing export_modelzoo_model into run_pytorch_test to avoid importing heavy modules unless PyTorch test runs. - Move MODELS_FOLDER.mkdir to after temporary directory creation. - Add a --nodisplay flag and set default for --display to False so CLI can explicitly disable display. - Comment out resize parameters in test calls and remove an unnecessary model_dir.exists() assertion. - Wrap per-backend test runs in try/except, collect backend failures, allow other backends to continue, and raise an aggregated RuntimeError if all backend tests fail. These changes improve robustness when some backends fail or are unavailable and reduce unnecessary imports during initial checks.
sneakers-the-rat
left a comment
sneakers-the-rat
left a comment
There was a problem hiding this comment.
looks good! some small optional notes.
Re: the formatting comment in the PR body, I see ruff in the dev dependencies, but we don't have a CI linter/formatter check action set up for this. I think it would probably be worth just setting up a set of ruff rules and a ruff CI action to do formatting all in one go and then not have to worry about it again in any further PRs.
| tmp_dir = Path(dlclive.__file__).parent / "check_install" / "dlc-live-tmp" | ||
| tmp_dir.mkdir(mode=0o775, exist_ok=True) | ||
| if Engine.PYTORCH not in get_available_backends(): | ||
| raise NotImplementedError( |
There was a problem hiding this comment.
| raise NotImplementedError( | |
| raise ImportError( |
super minor/optional thing - it is implemented, they just don't don't have the package installed
| # download model from the DeepLabCut Model Zoo | ||
| def run_tensorflow_test(video_file: str, display: bool = False): | ||
| if Engine.TENSORFLOW not in get_available_backends(): | ||
| raise NotImplementedError( |
There was a problem hiding this comment.
| raise NotImplementedError( | |
| raise ImportError( |
| "checkpoint": MODELS_FOLDER / f"exported_quadruped_{TORCH_MODEL}.pt", | ||
| "super_animal": "superanimal_quadruped", | ||
| } | ||
| TF_MODEL_DIR = TMP_DIR / "DLC_Dog_resnet_50_iteration-0_shuffle-0" |
There was a problem hiding this comment.
why not in the models folder?
| SNAPSHOT_NAME = "snapshot-700000.pb" | ||
| TMP_DIR = Path(__file__).parent / "dlc-live-tmp" | ||
|
|
||
| MODELS_FOLDER = TMP_DIR / "test_models" |
There was a problem hiding this comment.
probably use _DIR or _FOLDER consistently? doesn't really matter tho
| print("\nCreating temporary directory...\n") | ||
| tmp_dir = TMP_DIR | ||
| tmp_dir.mkdir(mode=0o775, exist_ok=True) | ||
| MODELS_FOLDER.mkdir(parents=True, exist_ok=True) |
There was a problem hiding this comment.
since this is only used in pytorch test, maybe make it there? or put all models in the model folder.
| video_file = str(tmp_dir / "dog_clip.avi") | ||
|
|
||
| # download dog test video from github: | ||
| # Use raw.githubusercontent.com for direct file access | ||
| if not Path(video_file).exists(): |
There was a problem hiding this comment.
gets a little odd switching back and forth, maybe we could just make the download_file accept a path
| try: | ||
| if tmp_dir is not None and tmp_dir.exists(): | ||
| shutil.rmtree(tmp_dir) | ||
| except PermissionError: | ||
| warnings.warn( | ||
| f"Could not delete temporary directory {str(tmp_dir)} due to a permissions error, but otherwise dlc-live seems to be working fine!" | ||
| ) |
There was a problem hiding this comment.
if we move the start of the try to after the directories are created, this could be a little cleaner, e.g. not having to init tmp_dir to None.
|
|
||
| if not any_backend_succeeded and backend_failures: | ||
| failure_messages = "; ".join( | ||
| f"{b}: {exc}" for b, exc in backend_failures.items() | ||
| ) | ||
| raise RuntimeError(f"All backend tests failed. Details: {failure_messages}") |
There was a problem hiding this comment.
the copilot suggestion is fine, but now it doesn't look like we get confirmation when the backends do succeed. like if both succeed we'd probably want the output to look something like
tensorflow [SUCCESS]
pytorch [SUCCESS]
and then if they fail like
tensorflow [SUCCESS]
pytorch [ERROR]
---
Pytorch error:
{blah blah blah}
doesn't have to be that, just saying we should probably also tell the user when things went right
2 participants
This pull request refactors and enhances the benchmarking and installation check scripts for DLC-Live, with a primary focus on supporting both TensorFlow and PyTorch backends in the installation check. The changes improve code structure, add backend-agnostic testing, and include various code style and clarity improvements.
Major enhancements to installation check and backend support
dlclive/check_install/check_install.pyto add support for testing both TensorFlow and PyTorch backends, including backend detection, model export/download, and backend-specific benchmarking via newrun_pytorch_testandrun_tensorflow_testfunctions. The script now iterates over available backends and runs tests for each, with improved error handling and clearer output. [1] [2] [3]Code style, clarity, and maintainability improvements
dlclive/benchmark.pyfor better readability, including argument formatting, function signatures, and block structure. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14] [15]TYPE_CHECKINGandnoqacomments to avoid unnecessary runtime imports and linter warnings. [1] [2] [3] [4]single_animalinbenchmark_videos). [1] [2]These changes make the benchmarking and installation check scripts more robust, maintainable, and compatible with multiple deep learning backends.