Add Siglip2Tokenizer to enforce training-time text preprocessing defaults - #43101
Conversation
|
here are the test results including the one that I added in this PR. |
|
I verified locally that SigLIP2 text embeddings behave as expected once the training-time preprocessing is applied (lowercasing + fixed-length padding with max_length=64). This PR intentionally avoids changing processor defaults and instead documents the correct usage and adds a small test to prevent regressions. |
There was a problem hiding this comment.
I think it should be done on a tokenizer level. Siglip model for example loads a SiglipTokenizer class which has a lower case called on inputs before tokenizing
@itazap can we use SiglipTokenizer here as well or maybe add a special Siglip2Tokenizer class?
@zucchini-nlp Thanks for reviewing this PR |
this sounds like the most reasonable solution no? |
thanks a lot for reviewing the PR @ArthurZucker |
Thanks for the guidance @ArthurZucker , this is now addressed I implemented automatic lowercasing and the SigLIP2 default text padding/truncation directly in SiglipProcessor.call, gated to SigLIP2-branded checkpoints. This avoids changing GemmaTokenizerFast globally or introducing a new tokenizer class, while ensuring SigLIP2 users get the correct preprocessing by default via AutoProcessor. I have also updated the SigLIP2 docs to clarify the preprocessing expectations and note that these defaults are now handled automatically by the processor and added regression tests covering the following: please let me know if you’d prefer this logic to live elsewhere, but this seemed like the least intrusive fix. |
|
looking for your inputs here as well @ArthurZucker |
ArthurZucker
left a comment
There was a problem hiding this comment.
Hey! I really don't think we should do any of the fixes here, but rather on the hub.
We can add a siglip2 tokenizer but it should be explicit that it uses a lowercase normalizer, and should be for all Siglip2 / siglip_2 tokenizer. If its just for 1-2 then it should be updated on the hub directly !
|
Thanks for detailed review of this @zucchini-nlp @ArthurZucker |
e25cc6c to
e1aafd1
Compare
|
all the test are passing except the glm_image test which is unrelated to this change. I am not sure why but I tested the same locally in main and PR branch and both were successful . please take a look at the PR for the change it is intended to solve. I am not sure why the CI test is failing on glm_image. sorry to bother you guys again |
|
The failing glm-image tests is supposed to be skipped. I see that there is a typo in test file and it's not being skipped, so feel free to ignore it |
9993a74 to
b167482
Compare
|
It looks like the PR is failing in the “CircleCI Failure Summary Comment” GitHub Action, not in CircleCI itself. What I’m seeing: I’ve rebased on latest, and the SigLIP2 changes don’t touch CI or CircleCI config, so this seems like an edge case in the failure-summary workflow. not sure, if I missed anything. @zucchini-nlp |
|
hi @zucchini-nlp |
|
looks like there is an open issue for t5 test failures, I will try and rebase again when this is fixed. all other tests are passing. |
0c8f75d to
64e9c82
Compare
0379e11 to
d150056
Compare
|
View the CircleCI Test Summary for this PR: https://huggingface.co/spaces/transformers-community/circle-ci-viz?pr=43101&sha=9bac1b |
zucchini-nlp
left a comment
There was a problem hiding this comment.
LGTM, thanks a lot for iterating on it! The failing test is flaky and seems unrelated
4131a53 to
e648a65
Compare
zucchini-nlp
left a comment
There was a problem hiding this comment.
One tiny comment in docs, to be precise 😄
|
Thanks a ton @zucchini-nlp and @ArthurZucker I really appreciate the patience and all the helpful nudges along the way 🙏 |
|
Thanks, we can merge imo. The CI is not feeling well since yesterday so let's wait a bit until it's fixed |
Thanks @zucchini-nlp 🙏 , will wait for CI to recover. I’ll keep an eye on the checks. |
ArthurZucker
left a comment
There was a problem hiding this comment.
the attribute error should normally prevent the function to be written
| backend.normalizer = normalizers.Sequence([normalizers.Lowercase(), backend.normalizer]) | ||
|
|
||
| def _unk_id(self) -> int: | ||
| raise ValueError("_unk_id is not needed for SigLIP2.") |
There was a problem hiding this comment.
| raise ValueError("_unk_id is not needed for SigLIP2.") | |
| raise AttributeError("_unk_id is not needed for SigLIP2.") |
There was a problem hiding this comment.
Ah shit it does not work one sec
|
[For maintainers] Suggested jobs to run (before merge) run-slow: auto, siglip2 |
|
thanks @ArthurZucker for fixing the last issue and for merging the PR |
|
for sure, sorry for the delay |
| # Persist for save/load + push_to_hub dynamic tokenizer test | ||
| if hasattr(self, "init_kwargs") and isinstance(self.init_kwargs, dict): | ||
| self.init_kwargs.setdefault("tokenizer_class", self.__class__.__name__) | ||
|
|
||
| backend = getattr(self, "_tokenizer", None) | ||
| if backend is not None and backend.normalizer is not None: | ||
| backend.normalizer = normalizers.Sequence([normalizers.Lowercase(), backend.normalizer]) |
There was a problem hiding this comment.
we should remove that tho
What does this PR do?
This PR makes SigLIP2 text preprocessing explicit and consistent with how the model was trained.
It introduces a model specific tokenizer (Siglip2Tokenizer) that wraps the existing GemmaTokenizer while enforcing SigLIP2’s training-time defaults (lowercasing and fixed padding/truncation to 64 tokens). AutoTokenizer and AutoProcessor are updated to use this tokenizer for SigLIP2 checkpoints, so users get the correct behavior automatically without relying on implicit processor logic.
The underlying tokenization and vocabulary remain unchanged. This is a lightweight wrapper that improves correctness, reproducibility, and clarity, especially for text embedding and retrieval use cases.
This addresses the behavior reported in #43054.
Fixes # 43054
Before submitting
Pull Request section?
to it if that's the case.
documentation guidelines, and
here are tips on formatting docstrings.
Who can review?
@ArthurZucker @itazap
Anyone in the community is free to review the PR once the tests have passed. Feel free to tag
members/contributors who may be interested in your PR.