handle inputs from Siglip/Siglip2 non-automapped encoder layers #41930
Conversation
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
src/transformers/utils/generic.py
Outdated
|
|
||
| # _can_record_outputs is None by default | ||
| capture_flags = _CAN_RECORD_REGISTRY.get(str(self.__class__)) or {} # there is a weak ref for executorch | ||
| capture_flags = _CAN_RECORD_REGISTRY.get(str(self.__class__)) or getattr(self, "_can_record_outputs", {}) |
There was a problem hiding this comment.
I think we can revert this, the registry should already be either applied to the class or we get the default {}. See
transformers/src/transformers/modeling_utils.py
Line 1851 in 4d0b675
And I honestly don't wanna risk breaking executorch
There was a problem hiding this comment.
yeah the latter reason is compelling 😬
| return self.vision_model.embeddings.patch_embedding | ||
|
|
||
| @check_model_inputs(tie_last_hidden_states=False) | ||
| @auto_docstring |
There was a problem hiding this comment.
Nit: Ig this was already inherited?
There was a problem hiding this comment.
uh, interesting. yes it should have been, entirely unrelated to this lol
|
run-slow: siglip, siglip2 |
|
This comment contains run-slow, running the specified jobs: models: ['models/siglip', 'models/siglip2'] |
…ce/transformers into siglip_and_check_model_changes
|
run-slow: siglip, siglip2 |
|
This comment contains run-slow, running the specified jobs: models: ['models/siglip', 'models/siglip2'] |
|
ended up down the rabbit hole of wrong |
|
run-slow: siglip, siglip2 |
|
This comment contains run-slow, running the specified jobs: models: ['models/siglip', 'models/siglip2'] |
|
run-slow: siglip, siglip2 |
|
This comment contains run-slow, running the specified jobs: models: ['models/siglip', 'models/siglip2'] |
|
run-slow: siglip, siglip2 |
|
This comment contains run-slow, running the specified jobs: models: ['models/siglip', 'models/siglip2'] |
|
[For maintainers] Suggested jobs to run (before merge) run-slow: siglip, siglip2 |
|
run-slow: siglip, siglip2 |
|
This comment contains run-slow, running the specified jobs: models: ['models/siglip', 'models/siglip2'] |
| @unittest.skip(reason="This test is broken on A10 multi runners for now") | ||
| def test_multi_gpu_data_parallel_forward(self): | ||
| pass |
There was a problem hiding this comment.
We shouldn't skip these, will be hard to revert because everyone will forget imo
| nn.init.normal_(module.fc1.bias, std=1e-6) | ||
| nn.init.normal_(module.fc2.bias, std=1e-6) | ||
| elif isinstance(module, SiglipMultiheadAttentionPoolingHead): | ||
| elif "MultiheadAttentionPoolingHead" in module.__class__.__name__: |
What does this PR do?
Should fix #41929 . The
check_model_inputs/can_record_outputsinteraction is not always trivial and models with several entrypoints such asVisionModelvsVisionTransformerare missing some, adding it here. Also added a modification ingenericto make sure the flag was captured, not 100% sure it's needed.