fix: pass an explicit factor to smart_resize for Qwen2.5-VL - #241
fix: pass an explicit factor to smart_resize for Qwen2.5-VL#241roshaninfordham wants to merge 2 commits into
Conversation
qwen-vl-utils 0.0.13 made `factor` a required argument of `smart_resize`,
so `detections_to_suffix_formatter` raised
TypeError: smart_resize() missing 1 required positional argument: 'factor'
on any COCO detection dataset, which is the path the Qwen2.5-VL fine-tuning
cookbook takes.
Pass the value explicitly rather than bounding the dependency: the argument
exists in every released version of qwen-vl-utils, so an explicit call works
on both sides of the change, and the constant it needs -- the vision encoder's
14px patch size times its 2x2 spatial merge -- is a property of the model
rather than of the helper library. 0.0.13 also removed the IMAGE_FACTOR
constant, so it cannot be imported from qwen-vl-utils any more.
Exposed as an `image_factor` parameter so a variant with different vision
encoder geometry can override it.
|
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 11d080d847
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| from maestro.trainer.models.qwen_2_5_vl.detection import ( | ||
| QWEN_2_5_VL_IMAGE_FACTOR, | ||
| detections_to_suffix_formatter, | ||
| ) |
There was a problem hiding this comment.
In the default test environment this new test is collected unconditionally, but tox.ini only installs pytest plus the package’s normal dependencies, while qwen-vl-utils lives under the optional qwen_2_5_vl extra in pyproject.toml. On a standard tox/dev run without that extra, importing maestro.trainer.models.qwen_2_5_vl.detection fails during collection before any tests can run; gate this with pytest.importorskip("qwen_vl_utils") or install the qwen extra in the test env.
Useful? React with 👍 / 👎.
The module under test imports qwen_vl_utils, which ships with the optional qwen_2_5_vl extra. Collected unguarded, that import aborts the entire test run rather than only its own file: in an environment with transformers but without qwen-vl-utils, the florence_2 suite collects 15 tests today and collected none with this file present.
2 participants
Description
Closes #227.
qwen-vl-utils0.0.13 madefactora required argument ofsmart_resize(QwenLM/Qwen3-VL@0dcc180).detections_to_suffix_formattercalls it without one, so every COCO detection dataset fails:That is the path the Qwen2.5-VL object detection cookbook takes, and
pyproject.tomlpinsqwen-vl-utils>=0.0.8with no upper bound, so a fresh install resolves to a broken combination.Type of Change
Motivation and Context
I passed the argument explicitly rather than capping the dependency, for three reasons.
factorhas existed in every released version ofqwen-vl-utils— only its default was removed — so an explicit call works on both sides of 0.0.13 and needs no version bound.The value is a property of the model, not of the helper library: Qwen2.5-VL's vision encoder uses a 14px patch with a 2×2 spatial merge, so resized side lengths must be multiples of 28. The repository already assumes this everywhere —
min_pixelsandmax_pixelsdefault to256 * 28 * 28and1280 * 28 * 28.The issue suggests importing the old
IMAGE_FACTORconstant, but 0.0.13 removed it too. On 0.0.14:So the constant has to be defined locally. It is exposed as an
image_factorparameter so a variant with different vision-encoder geometry can override it without editing the module.Changes Made
maestro/trainer/models/qwen_2_5_vl/detection.py— addQWEN_2_5_VL_IMAGE_FACTOR = 28, take animage_factorparameter defaulting to it, and pass it tosmart_resize. Add a Google-style docstring covering the new parameter.test/meastro/trainer/models/qwen_2_5_vl/test_detection.py— new, 6 cases: coordinate scaling at two source resolutions, multi-detection label resolution, a non-defaultimage_factor, the empty-detections case, and a check that the resized side lengths really are multiples of the factor.Testing
With the defect present the new tests reproduce the reported error exactly:
Commands run, on macOS with
qwen-vl-utils 0.0.14installed:The expected coordinates in the tests were derived rather than recorded from the implementation. For a 640×480 source,
smart_resizereturns(h=476, w=644)— 17×28 and 23×28 — so a box aty=20maps to20 × 476/480 = 19.83, truncating to19, which is what the test asserts.Google Colab (optional)
Not applicable; the failure is a
TypeErroron import-time-visible signature drift, covered by the unit tests above and reproducible without a GPU or a model download.