Repository navigation
[Model] Gr00t-N1.7: honor processor_config preprocessing flags (use_albumentations, crop_fraction, letter_box_transform) /claim #7081 - #7082
Conversation
…tions, crop_fraction, letter_box_transform) /claim vllm-project#7081
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
This PR appears to belong to: docs/design/module/diffusion/diffusion_model_integration.md. Module owners: @fhfuih @Bounty-hunter @wtomin @RuixiangMa @jialfaro, please review your own changes and leave a short self-review comment describing what you checked. PRs without author self-review may not be assigned a reviewer. Please take a look when you have a chance. If you would like an automated review, mention @vllm-omni-review-bot in a comment. |
Omni ReviewBot triage noteAutomated triage of commit
Quality evidence:
These are automated triage suggestions only — the final decision belongs to the maintainers. |
hsliuustc0106
added
bug
jialfaro
changed the title
Author Self-Review
Ready for review by module owners (@fhfuih @Bounty-hunter @wtomin @RuixiangMa). |
| if crop_fraction is not None: | ||
| steps.append(FractionalCenterCrop(crop_fraction)) | ||
| # Resize back to target size using BOX/area-like interpolation. | ||
| steps.append(transforms.Resize(size=image_target_size, interpolation=transforms.InterpolationMode.BOX)) |
There was a problem hiding this comment.
Use cv2.INTER_AREA for both resizes; torchvision tensor resize rejects BOX.
Omni ReviewBot: no human activity for 8 days@jialfaro this pull request has had no human commit, comment or review since 2026-09-15. Please confirm the current plan and next step. The author or a maintainer decides whether to change the PR state. To keep it moving, any one of these is enough: push an update, reply to the open blocker, or post the current plan and timeline. |
Omni ReviewBot: no human activity for 15 days@jialfaro this pull request has had no human commit, comment or review since 2026-09-15. Please consider marking this PR as draft until work can resume. The author or a maintainer decides whether to change the PR state. To keep it moving, any one of these is enough: push an update, reply to the open blocker, or post the current plan and timeline. |
Omni ReviewBot routing recordAssigned Strict on cursor (cursor-grok-4.6-high) under experiment |
vllm-omni-review-bot
left a comment
vllm-omni-review-bot
left a comment
There was a problem hiding this comment.
Omni ReviewBot review
2 actionable finding(s).
CI at
1a0f2d603db6(2026-10-09T23:17:51.150406+00:00): required check(s) blocking:buildkite/vllm-omni(missing).
Full review analysis
Scan:
| Category | Result |
|---|---|
| Tests / verification | 2 finding(s) below |
| Security | no finding reported |
| Docs / comments | no finding reported |
| Behavior / compatibility | no finding reported |
| Correctness | no finding reported |
Validated:
- [resolved] flags no longer swallowed by **kwargs — they are named params; residual is the broken BOX implementation vs already-merged #7083.
- [resolved] #7081 **kwargs swallow — named params at :267–268; residual BOX/geometry/tests already kept
- [claim-verified] honor processor_config: NVIDIA processor_kwargs include use_albumentations/letter_box_transform; from_pretrained passes processor_kwargs into cls(); save_pretrained now writes both keys (L569–570).
- [claim-verified] no albumentations import; opencv is only a dev/demo extra in pyproject, not requirements/common.txt.
- [claim-verified] no albumentations import added in this file.
- [claim-verified] no albumentations import added; opencv-python is already in pyproject optional-dev (5.0.0.93).
2 actionable finding(s).
Verdict: REQUEST CHANGES
See inline comments below.
🤖 This review was generated by InferMatrix Copilot, an open-source repo-maintenance agent for PR review, CI debugging and issue triage. Try it on your own repo, and ⭐ star it if it helped!
| return transforms.functional.pad(img, padding=[pad_left, pad_top, pad_right, pad_bottom], fill=0) | ||
|
|
||
|
|
||
| class FractionalCenterCrop: |
There was a problem hiding this comment.
[P0] Close #7082 as superseded by already-merged #7083 (d774033, 2026-09-08)
Evidence and suggested fix
Close #7082 as superseded by already-merged #7083 (d774033, 2026-09-08). This head adds FractionalCenterCrop/SmallestMaxSize and a use_albumentations Compose that does transforms.ToImage() then InterpolationMode.BOX resize (processing_gr00t_n1d7.py:128/:136/:140). Gr00tPolicy loads AutoProcessor.from_pretrained (policy.py:76, unchanged) and from_pretrained injects processor_config processor_kwargs; nvidia/GR00T-N1.7-3B ships use_albumentations:true, so every serving video frame hits tensor BOX (torchvision v2: BOX is PIL-only). GitHub reports mergeable=false / mergeable_state=dirty against main 66bbebd because #7083 already landed AlbumentationsEvalImageTransform with cv2.INTER_AREA. Do not keep these helpers in the conflict resolution — that also subsumes the existing L140 INTER_AREA inline.
Evidence: PR-time processing_gr00t_n1d7.py:128 steps = [transforms.ToImage()]; :136 steps.append(SmallestMaxSize(shortest_image_edge, interpolation=transforms.InterpolationMode.BOX)); :140 steps.append(transforms.Resize(size=image_target_size, interpolation=transforms.InterpolationMode.BOX)) — this arm runs when use_albumentations is true. Trigger: unchanged by this diff, present in the PR-time tree: policy.py:76 self.processor: Gr00tN1d7Processor = AutoProcessor.from_pretrained(processor_dir) and processing_gr00t_n1d7.py:619 processor_kwargs = config["processor_kwargs"] / :651 return cls(**processor_kwargs, transformers_loading_kwargs=transformers_loading_kwargs), then :493 image_transform = self.eval_image_transform applied to video frames. nvidia/GR00T-N1.7-3B processor_config.json has "use_albumentations": true, so serving takes the BOX path; torchvision v2 documents BOX as PIL-only after ToImage(), so every Gr00tPolicy frame is sent through an unsupported tensor resize (crash or non-INTER_AREA pixels). GitHub pulls/7082 mergeable=false mergeable_state=dirty vs main 66bbebd; #7083 already merged 2026-09-08 as d774033 with AlbumentationsEvalImageTransform (cv2.INTER_AREA) — not in this PR-time tree (grep has no AlbumentationsEvalImageTransform). Keeping these helpers while resolving the conflict would reintroduce BOX onto main.
| """ | ||
| steps = [transforms.ToImage()] | ||
|
|
||
| if use_albumentations: |
There was a problem hiding this comment.
[P1] The new use_albumentations=True arm (processing_gr00t_n1d7.py:130) appends…
Evidence and suggested fix
The new use_albumentations=True arm (processing_gr00t_n1d7.py:130) appends SmallestMaxSize(..., InterpolationMode.BOX) then Resize(..., BOX) after ToImage(). Gr00tPolicy loads via AutoProcessor.from_pretrained (policy.py:76, unchanged) and from_pretrained does cls(**processor_kwargs) (:651), so checkpoint flags use_albumentations: true / letter_box_transform: false select this arm. SmallestMaxSize early-returns only when min(h, w) == self.size (:91, default 256); DROID handshake Gr00tN1d7.yaml:22 image_resolution: [180, 320] does not, so the BOX resize runs. This PR adds no test of _build_eval_image_transform; test_pipeline.py monkeypatches Gr00tPolicy, and tests/helpers/client.py:162 uses 256×256 frames that skip that resize (int(256*0.95)=243 crop is frame[6:249, 6:249]). Add a CPU unit test of _build_eval_image_transform(use_albumentations=True, letter_box_transform=False) vs cv2.INTER_AREA on that 256×256 crop and on a 180×320 frame so SmallestMaxSize cannot regress; it should fail at this head until both BOX resizes are replaced.
Evidence: Trigger: _build_eval_image_transform(use_albumentations=True, letter_box_transform=False) (selected when checkpoint processor_kwargs set those flags) on a frame whose min side is not 256, including the DROID handshake size. Adverse/unmet: SmallestMaxSize does not early-return and applies tensor Resize(..., BOX) after ToImage(); this PR adds that arm with zero tests, and the in-tree 256×256 helper skips the resize. processing_gr00t_n1d7.py:128 steps = [transforms.ToImage()]; :130 if use_albumentations:; :136 steps.append(SmallestMaxSize(shortest_image_edge, interpolation=transforms.InterpolationMode.BOX)); :91 if min(h, w) == self.size: return img; :97 resize = transforms.Resize(size=(new_h, new_w), interpolation=self.interpolation). Unchanged by this diff, present in the PR-time tree: policy.py:76 self.processor: Gr00tN1d7Processor = AutoProcessor.from_pretrained(processor_dir); processing_gr00t_n1d7.py:651 return cls(**processor_kwargs, transformers_loading_kwargs=transformers_loading_kwargs); Gr00tN1d7.yaml:22 image_resolution: [180, 320]; tests/diffusion/models/gr00t/test_pipeline.py:58 monkeypatch.setattr(pipeline_gr00t, "Gr00tPolicy", FakeGr00tPolicy); tests/helpers/client.py:162 "exterior_image_1_left": np.zeros((1, 2, 256, 256, 3), dtype=np.uint8).
Omni ReviewBot: finding feedback[p0] Close #7082 as superseded by already-merged #7083 (d774033, 2026-09-08) — See the review for details. If you are the PR author and disagree, react 👎 here; the maintainer will see your disagreement. |
Omni ReviewBot: finding feedback[p1] The new See the review for details. If you are the PR author and disagree, react 👎 here; the maintainer will see your disagreement. |
Omni ReviewBot: no human activity for 31 days@jialfaro this pull request has had no human commit, comment or review since 2026-09-15. Please consider closing this PR if the work is no longer planned. The author or a maintainer decides whether to change the PR state. To keep it moving, any one of these is enough: push an update, reply to the open blocker, or post the current plan and timeline. |
|
@jialfaro Are you planning to continue work on this PR? Please reply by 2026-10-20 04:15:47 UTC. A yes or a brief plan is enough; code changes do not need to be finished within these three days. If you are waiting on a review or other maintainer action, please let us know. If there is no reply, new commit, or maintainer takeover by then, we will recheck the PR's status before deciding whether to close it as inactive. It can be reopened when work resumes. |
4 participants
Summary
Files changed
Verification
Run the python snippet in the issue body to confirm the transformed pixel_values match the Isaac-GR00T reference when use_albumentations=True and letter_box_transform=False. The expected maximum absolute difference is 0 on the synthetic deterministic frame shown in the snippet.
Notes
/claim #7081
Closes #7081
Closes #7081
Enviado por un agente Monexa; revisión humana: no.