Repository navigation
Conversation
Contributor
|
[For maintainers] Suggested jobs to run (before merge) run-slow: grounding_dino, mm_grounding_dino |
Contributor
CI recapDashboard: View test results in Grafana |
Collaborator
|
I think the cast should be at |
Collaborator
|
coming back to this @Yigtwxx: position_ids also feed the BERT embedding, so they have to stay Maybe something like if text_position_embedding is not None:
return text_position_embedding
if text_position_ids is None:
text_position_ids = torch.arange(seq_length, device=text_features.device).expand(batch_size, -1)
return encode_sinusoidal_position_embedding(
text_position_ids[..., None].to(text_features.dtype), num_pos_feats=self.d_model
) |
This branch has not been deployed
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.
What does this PR do?
GroundingDinoEncoderLayer.get_text_position_embeddingspasses the int64text_position_idsstraight intoencode_sinusoidal_position_embedding, which ends with.to(pos_tensor.dtype). Every sin/cos value in[-1, 1]is therefore truncated toward zero and the embedding collapses to{-1, 0, 1}— at the reald_model=256, 93.4% of it becomes exactly zero.The branch is taken on every forward:
GroundingDinoModel.forwardcalls the encoder withtext_position_embedding=Noneandtext_position_ids=position_ids(modeling_grounding_dino.py:2166-2168), whereposition_idscomes fromgenerate_masks_with_special_tokens_and_transfer_mapand is.to(torch.long)(:1854). The other branch, which does.float()before calling, is dead on the model path — and it is the one that shows the intended dtype.This casts the ids to the text feature dtype instead, which keeps the values and also keeps the embedding in the model dtype under half precision:
mm_grounding_dinoinherits this encoder layer through modular, so the change propagates to its generated file without a separate edit.Where it came from
c9de1097ee(#41693) replaced the model's ownget_sine_pos_embedwith the sharedencode_sinusoidal_position_embedding. The old helper returnedtorch.cat(position_embeddings, dim=-1)with no cast, so it produced float32 regardless of input dtype. The new one casts back to the input dtype, which is a no-op for every other caller (conditional_detr,dab_detr,lw_detr,rf_detrpass float anchor coordinates) and destructive for this one, which passes token position ids.That commit changed the two modeling files and no test file, so the checked-in integration expectations still describe the pre-refactor behavior.
Tests
@slowintegration tests against the realIDEA-Research/grounding-dino-tinycheckpoint. These ran on CPU, so they compare against the(None, None)entry of theExpectationsblocks; I do not have the A10 the slow CI uses, so the exact deltas there may differ:GroundingDinoModelIntegrationTeststest_cross_attention_masktest_inference_object_detection_headtest_grounding_dino_lossSo one of the repo's own integration expectations goes green and another's divergence halves, without those expectations being touched.
I want to be precise about what this does not do: it does not fully restore them.
test_grounding_dino_lossstill fails and the detection head is still outside tolerance, which suggests the same refactor changed at least one more thing. I could not identify it, so I have left it rather than guess, and I would rather land the part that is established than bundle it with something unverified. Happy to keep digging if that is preferred.Fast tests, both models, unchanged:
ruff check,ruff format --check,modular_conversionandcopiesare clean.Note this changes Grounding DINO's numerical outputs, in the direction of the expectations that predate the refactor. Let me know if you would like the title marked accordingly.
Fixes #47674
Disclosure: this fix was implemented with the assistance of a code agent, and reviewed and validated by me.
Before submitting
Pull Request checks?
Who can review?
@yonigozlan @molbap