Conversation
* Switch zamba to pixeltable-yolox Replace the YOLOX dependency in pyproject and add compatibility shims for the updated YOLOX and megadetector package layouts so CLI inference works in the isolated env. * test fixes on gpu * Pin npy cache hash test to cpu device
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #400 +/- ##
========================================
- Coverage 82.8% 82.4% -0.4%
========================================
Files 37 39 +2
Lines 3194 3352 +158
========================================
+ Hits 2646 2764 +118
- Misses 548 588 +40
🚀 New features to boost your workflow:
|
|
@ejm714 This should be ready to go, I ran the test suite on a Windows machine, a Mac, and a linux box with a GPU. The diff looks big, but almost all of the substantive code changes are just refactoring so that image/video code paths are separable and imports are lazy loaded if need be so that you don't have to have all the deps for both sides available. Other code changes are almost all about stabilizing the test suite across platforms. Easiest way to start the review may be to look at the pyproject.toml changes, the Makefile changes, and the README changes to see what surface area changed and then the things in the weeds will make more sense. Don't want to rock the boat if you're demoing this week but could be nice to have the new version for folks pip-installable! |
pjbull
changed the title
ejm714
left a comment
ejm714
left a comment
There was a problem hiding this comment.
So much good stuff in here! Just a couple small things
| $ git clone https://github.com/drivendataorg/zamba.git | ||
| $ cd zamba | ||
| $ pip install -r requirements-dev.txt | ||
| $ uv pip install -e ".[tests,image,video,docs]" |
There was a problem hiding this comment.
maybe worth creating a "dev" dependency group for simpler install? https://docs.astral.sh/uv/concepts/projects/dependencies/#nesting-groups
|
|
||
|
|
||
| #### FFmpeg version 4 | ||
| #### FFmpeg > 4.3 (only needed for video workflows) |
There was a problem hiding this comment.
I think we saw different predictions with ffmpeg 5 #180 that were sufficient enough to change the top class. Probably needs more testing before we can remove the ceiling
| filtered_frames[:, 0, 0, 0] | ||
| == np.array( | ||
| [90, 80, 70, 60, 44, 67, 73, 5, 54, 64, 65, 34, 93, 72, 56, 50, 87, 83, 47, 88] | ||
| [90, 80, 70, 60, 72, 96, 66, 75, 95, 93, 69, 23, 76, 57, 45, 83, 50, 51, 56, 73] |
There was a problem hiding this comment.
are these differences concerning or expected? want to make sure we're not dramatically changing the output of MDLite where it's going to affect overall classification accuracy
| filtered_frames[:, 0, 0, 0] | ||
| == np.array( | ||
| [90, 80, 70, 60, 50, 87, 30, 40, 34, 10, 22, 20, 71, 16, 39, 14, 77, 65, 42, 13] | ||
| [90, 80, 70, 60, 50, 13, 30, 40, 49, 10, 34, 14, 52, 36, 81, 99, 79, 24, 0, 20] |
| DensePose dependencies are not installed. They are not distributed on PyPI. | ||
| Install them from GitHub after installing PyTorch: | ||
|
|
||
| pip install torch |
There was a problem hiding this comment.
worth having this be uv pip to match docs?
| ] | ||
|
|
||
| [tool.uv] | ||
| override-dependencies = [ |
| pytest tests -vv | ||
|
|
||
| ## Dev: fail fast on the first test failure | ||
| tests-fast: clean-test |
There was a problem hiding this comment.
[nit] should we standardize so all test commands are test (singular) not test?
- make test
- make test-fast
- make test-debug
- make test-image-only
- make test-video-only
| ZAMBA_RUN_DENSEPOSE_TESTS=1 pytest tests/test_densepose.py tests/test_cli.py::test_densepose_cli_options -vv | ||
|
|
||
| ## Test image deps in isolation: fresh venv, install image extra only, run image + agnostic tests, cleanup | ||
| test-image-only: |
There was a problem hiding this comment.
looks like this is duplicative of images-tests which i don't think we need any more. can you double check all test commands in the makefile?
| - Import image classifier lazily from `zamba.images` to avoid pulling in torch at package import time. Images config and manager use `zamba.models.config_common` and `zamba.models.instantiation` instead of config/utils from video. | ||
| - Make megadetector import resilient: try `megadetector.detection.run_detector`, fall back to `detection.run_detector`. Make MLflow optional in image training (log warning and continue without it if import or setup fails). Disable `torch.compile` on Windows in addition to macOS. | ||
| - Add pytest markers `video` and `image` and skip test files when the corresponding extra is not installed (conftest `collect_ignore` based on `_HAS_VIDEO` / `_HAS_IMAGE`). Define video-only fixtures and the dummy video model only when video deps are present. Set `CUDA_VISIBLE_DEVICES=0` in conftest to avoid DDP issues under pytest-xdist. | ||
| - Add Makefile targets `tests-fast` (fail on first failure) and `test-image-only` / `test-video-only` (isolated venvs with only image or video extra). CI runs these isolation steps and installs image,video extras for the main test matrix. DensePose tests install detectron2 from GitHub with `--no-build-isolation` instead of using a densepose extra. |
There was a problem hiding this comment.
flagging that if we rename commands in the makefile, need to reflect that here too
| # Installing `zamba` | ||
|
|
||
| Zamba has been developed and tested on macOS and Ubuntu Linux for both CPU and | ||
| Zamba has been developed and tested on macOS, Ubuntu Linux, and Windows for both CPU and |
There was a problem hiding this comment.
from a user-perspective, this file would probably be easier to follow if the departure point was the operating system rather than the install step.
so instead of
step 1
- linux
- macos
- windows
it was
linux
- step 1
- step 2
- step 3
macos
- ...
2 participants
Lots of updates with the goals of:
Changelog:
video(av, ffmpeg-python, pytorchvideo, pixeltable-yolox, etc.),image(megadetector, Pillow),tests(pytest, black, flake8, coverage, nvidia-ml-py, etc.), anddocs(mkdocs, mike, mkdocstrings). Base install no longer pulls in video/image stacks; usepip install zamba[video],zamba[image], orzamba[video,image].requirements-dev.txtandrequirements-dev/; usepip install -e ".[tests]"for development andpip install -e ".[docs]"for building docs.mlflowwithmlflow-skinnyin core dependencies. Addpyarrow>=23.0.0. Add Windows-specific torch version constraint for gloo bug. Declare Python 3.11–3.13 support andrequires-python = ">=3.11, <3.14".zamba denseposewithout them prints install instructions and exits. Add[tool.uv.extra-build-dependencies]for detectron2 (torch) so CI can build it from GitHub.setuptools<82so the image extra works on Python 3.12 (megadetector’s yolov5 stack requirespkg_resources, which setuptools 82 removed). Add[tool.uv]override-dependencies for protobuf and setuptools.zamba.models.config_commonwith shared types and validators (ModelEnum, MonitorEnum, ZambaBaseModel, RegionEnum, get_filepaths, validate_model_cache_dir, etc.). Move video-specific file/checkpoint logic intozamba.models.configand reuse common helpers.zamba.models.instantiationand moveinstantiate_model, head-replacement, and resume logic out ofmodel_managerfor clearer separation.zamba.models.registry(ensure_registered()): video model classes are imported only when needed so the package can be imported without video dependencies. Config validation callsensure_registered()when resolving checkpoint/model name.zamba --helpand non-image/non-utils commands work without image or densepose dependencies. Defer imports of VideoLoaderConfig and config classes into the train, predict, and depth command callbacks.denseposecommand: catch ImportError when running the model and print install-from-GitHub instructions and exit with code 1.str(hashed_part). For local absolute paths, use a relative-style path in the cache key.npy_cache.__del__by comparingPath(cache_path).parents[0]withPath(tempfile.gettempdir()).zamba.imagesto avoid pulling in torch at package import time. Images config and manager usezamba.models.config_commonandzamba.models.instantiationinstead of config/utils from video.megadetector.detection.run_detector, fall back todetection.run_detector. Make MLflow optional in image training (log warning and continue without it if import or setup fails). Disabletorch.compileon Windows in addition to macOS.videoandimageand skip test files when the corresponding extra is not installed (conftestcollect_ignorebased on_HAS_VIDEO/_HAS_IMAGE). Define video-only fixtures and the dummy video model only when video deps are present. SetCUDA_VISIBLE_DEVICES=0in conftest to avoid DDP issues under pytest-xdist.tests-fast(fail on first failure) andtest-image-only/test-video-only(isolated venvs with only image or video extra). CI runs these isolation steps and installs image,video extras for the main test matrix. DensePose tests install detectron2 from GitHub with--no-build-isolationinstead of using a densepose extra.pip install zamba[video]etc.), and Windows (image extra no extra tools; video needs Visual Studio Build Tools and FFmpeg). State FFmpeg only required for video workflows. Contribute page: dev install with.[tests,image,video,docs], DensePose deps from GitHub,make requirementswith uv. DensePose doc: install detectron2/detectron2-densepose from GitHub; note thatzamba denseposeprints instructions if deps missing..[docs]instead ofrequirements-dev/docs.txt.