feat: depend on the groundingdino conda package instead of vendoring it - #2
Draft
phreed wants to merge 7 commits into
Draft
feat: depend on the groundingdino conda package instead of vendoring it#2phreed wants to merge 7 commits into
phreed wants to merge 7 commits into
Conversation
Removes the vendored copy of GroundingDINO. Every fix this project made to that tree is now a pull request against IDEA-Research/GroundingDINO, and the conda-forge feedstock builds it, so keeping a fork in a subdirectory buys nothing and hides which changes are ours. groundingdino/config/ stays: every eval and demo script loads the two model configs by path, not by import. groundingdino/__init__.py is deleted, which is what makes that safe -- a directory without it is only a namespace portion, and a namespace portion loses to the installed regular package. Left in place it would shadow the dependency. setup.py and recipe.yaml go with it: both existed to build the detector. pixi.toml is merged into pyproject.toml, so one file describes the project to both pip and pixi, and the CUDA build toolchain drops out of the environment entirely.
docker/Dockerfile.ros2 still built the detector out of this repository: `python3 setup.py clean --all` followed by an editable install. setup.py is gone, so the image no longer built at all. It now clones GroundingDINO and compiles the extension from there, pinned to the branch carrying this project's fixes until they merge upstream. That file also asked pip for requirements.txt, which has never existed here -- the file is r.txt. Fixed while in the area. The image now asserts at build time that `import groundingdino` resolves to site-packages. The ./groundingdino config directory has no __init__.py, so it is only a namespace portion and loses to the installed package, but that is subtle enough to be worth failing the build over rather than debugging in a running container. Dockerfile is GroundingDINO's own, inherited by the fork. It COPYs docker_test.py, which exists in GroundingDINO and not here, so it has been unbuildable since the fork, and nothing references it. environment.yaml is superseded by [tool.pixi] in pyproject.toml, and points at the same nonexistent requirements.txt. pixi.lock described the environment pixi.toml declared, and pixi.toml is merged away. It is regenerated once groundingdino-py is resolvable.
Verified end to end for the first time, which turned up three problems the pixi.toml merge introduced. The `pytorch` and `nvidia` channels are gone. groundingdino-py-cuda is built against conda-forge's CUDA pytorch, so keeping them meant two builds of the same library in one environment -- and the merge had also dropped the `channel-priority = "disabled"` that let the three channels coexist, so the solve failed outright on libcusolver. [project].dependencies is now empty. Listing the conda dependencies there as well made pixi resolve them a second time against PyPI, where groundingdino-py is still 0.4.0 and carries none of the fixes this project contributed. [tool.pixi.dependencies] is the one authoritative list. pytorch-cuda is dropped: it is the `pytorch` channel's spelling, and groundingdino-py-cuda already requires `pytorch *cuda*`, so the CUDA build is selected by the detector rather than pinned here. [system-requirements] moves onto the platform entry, which is where pixi now wants it. Confirmed in the built environment: `import groundingdino` resolves to site-packages rather than the ./groundingdino config directory, _C loads and exposes ms_deform_attn_forward, both transformers fixes are present in the shipped package, and SLConfig still loads the kept config by path.
The feedstock no longer builds a groundingdino-py-cpu metapackage; it was an empty package whose only content was a constraint against something opt-in. Running without the extension is just not installing groundingdino-py-cuda.
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.
Part of #1 — this is the downstream half. The upstream half is
IDEA-Research/GroundingDINO#458, #459, #460 and #461.
Removes the vendored copy of GroundingDINO and depends on the conda package instead.
Why
Every change this repository had made under
groundingdino/is now a pull request againstupstream, and the conda-forge feedstock builds the result. Keeping a fork in a subdirectory
after that buys nothing and actively hides which changes are ours.
The four files, and where they went:
models/GroundingDINO/bertwarper.pymodels/GroundingDINO/csrc/MsDeformAttn/ms_deform_attn_cuda.cuversion.pyconfig/GroundingDINO_SwinB_cfg.pyWhat survives, and why it has to
groundingdino/config/stays. Twenty call sites across eighteen files load those two modelconfigs by path, not by import:
groundingdino/__init__.pyis deleted, and that deletion is the load-bearing part of thischange rather than tidying.
A
groundingdino/directory in the working directory is found before site-packages. With an__init__.pyit is a regular package and wins immediately, silently shadowing the conda packagethat is supposed to replace it. Without one it is only a namespace portion: Python records it,
keeps scanning
sys.path, finds the installed regular package, and uses that.docker/Dockerfile.ros2now asserts this at image build time rather than leaving it to bediscovered at run time:
Also removed
setup.py— existed to build the vendored CUDA extension.recipe.yaml— built the detector; the feedstock owns that now.pixi.toml— merged intopyproject.toml, so one file describes this project to both pipand pixi. Two files stating the same dependency set is how they drift apart.
pixi.lock— regenerated against the new environment.Dockerfile— GroundingDINO's own, inherited by the fork. ItCOPYsdocker_test.py, whichexists upstream and not here, so it has been unbuildable since the fork, and nothing
references it.
environment.yaml— superseded by[tool.pixi], and pointed at arequirements.txtthat hasnever existed in this repository (the file is
r.txt).docker/Dockerfile.ros2also asked pip for that same nonexistentrequirements.txt; fixed whilein the area. It now clones GroundingDINO and compiles the extension from there, pinned to the
branch carrying the four fixes until they merge upstream.
Dependency changes worth review
[project].dependenciesis deliberately empty and[tool.pixi.dependencies]is the singleauthoritative list. Stating the same names in both makes pixi resolve them a second time against
PyPI, where
groundingdino-pyis still 0.4.0 and carries none of these fixes:It would be wrong even if it resolved —
torchfrom PyPI besidepytorchfrom conda is twobuilds of one library in one environment.
The
pytorchandnvidiachannels are gone, and with them thechannel-priority = "disabled"they needed to coexist.
groundingdino-py-cudais built against conda-forge's CUDA pytorch, sodrawing pytorch from the
pytorchchannel would mix two builds of it. One channel, one pytorch.The CUDA build toolchain drops out of the environment entirely —
cuda-nvcc,cuda-toolkit,gxx_linux-64,ninja, and thelibcublas/libcusolver/libcusparse/libcurand-devpackages were all there only to compile the vendored extension.
Swap
groundingdino-py-cudaforgroundingdino-py-cpuand the environment resolves on a machinewith no GPU at all — correctly, but slowly, and it must stay in float32 because
grid_samplehasno half-precision CPU kernel.
Verified
Not mergeable yet
pyproject.tomlpoints at a local channel, becausegroundingdino-py0.4.1 does not existpublicly until conda-forge/groundingdino-py-feedstock#3 lands and publishes.
That feedstock PR no longer waits on upstream review: it builds upstream's own tree and carries
the three fixes as patches, which is conda-forge's mechanism for exactly this. So the remaining
chain is short — feedstock merges and publishes, this drops the local channel and comes out of
draft. The upstream PRs can land on their own schedule; each patch is deleted as its PR does.
docker/Dockerfile.ros2still clonesphreed:integration/split-fixes. That container builds theextension with pip rather than installing the conda package, so it needs a git ref rather than a
channel. It is a fetchable branch and only a build-time input, but it is worth revisiting once the
upstream PRs merge — at which point it can clone
IDEA-Research/maindirectly.