Skip to content

Refactor Aya Vision with modular - #36688

Merged
yonigozlan merged 10 commits into
huggingface:mainfrom
yonigozlan:refactor-aya-vision
Mar 20, 2025
Merged

Refactor Aya Vision with modular#36688
yonigozlan merged 10 commits into
huggingface:mainfrom
yonigozlan:refactor-aya-vision

Conversation

@yonigozlan

@yonigozlan yonigozlan commented Mar 12, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor

What does this PR do?

Inherit from Llava with modular to remove most of the custom code.

I ran the integration tests with both 8B and 32B version and everything seems to work as expected.

@github-actions
github-actions Bot marked this pull request as draft March 12, 2025 23:13
@github-actions

github-actions Bot commented Mar 12, 2025

Copy link
Copy Markdown
Contributor

Hi 👋, thank you for opening this pull request! The pull request is converted to draft by default. When it is ready for review, please click the Ready for review button (at the bottom of the PR page).

@yonigozlan
yonigozlan marked this pull request as ready for review March 13, 2025 16:17
@yonigozlan

yonigozlan commented Mar 13, 2025

Copy link
Copy Markdown
Contributor Author

Hey @Cyrilvallez !
I have made a little modifications in modular_model_converter, the idea is to be able to provide a custom MODEL_NAME_START/INPUT_DOCSTRING. I used an ugly workaround to also keep the current behavior, that is to copy the MODEL_NAME_START/INPUT_DOCSTRING of the inherited model when it is set to None:

transformers/utils/modular_model_converter.py

Lines 861 to 865 in 0ba8784

should_keep = any(re.search(pattern, assignment) for pattern in ASSIGNMENTS_REGEX_TO_KEEP)
if (
should_keep and (not hasattr(node.body[0].value, "value") or node.body[0].value.value != "None")
) or assignment not in self.assignments:
self.assignments[assignment] = node

This new behavior is also extended to all constants in ASSIGNMENTS_REGEX_TO_KEEP. It doesn't crash with any of the current modular files, but it does seem like in this state it could crash at any moment 😅. Maybe there is a better way to check if the value of a constant is None with libcst?

@HuggingFaceDocBuilderDev

HuggingFaceDocBuilderDev commented Mar 13, 2025

Copy link
Copy Markdown

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.

@Cyrilvallez

Cyrilvallez commented Mar 14, 2025

Copy link
Copy Markdown
Member

Hey @yonigozlan! I agree that we should allow something like that. It is a bit magic, and it's not ideal, but for now it will make our lives easier with the docstrings (while we wait on better docstring support). What you did is quite close to what is proposed in https://github.com/huggingface/transformers/pull/35902/files#diff-dcfd09afc0f209bd5e8403add12d0cc6707dca021dcb639bc0b9a95c41843c7e.

Actually, I'd like a mix of both approaches: yours checks for all other patterns None value, and the one proposed by @nikosanto13 is a bit opaque, being directly hard-coded in the merge_assignment function. What I believe is the best approach right now would be to have a new list ASSIGNMENTS_REGEX_TO_KEEP_IF_NOT_NONE or something along those lines, and perform the None check only for assignment matching those patterns (which would be only r""DOCSTRING" as of now I think?)

Let me know what you think. It's a bit annoying to have the None magic, but I don't really see any better solution for those big blocks of docstrings 🥲🥲

@Cyrilvallez Cyrilvallez mentioned this pull request Mar 14, 2025
Merged
5 tasks
@yonigozlan

yonigozlan commented Mar 14, 2025

Copy link
Copy Markdown
Contributor Author

@Cyrilvallez sounds good to me! Maybe it's best that I open a separate PR to make those changes then :)

@yonigozlan yonigozlan mentioned this pull request Mar 14, 2025
Merged
@yonigozlan

yonigozlan commented Mar 19, 2025

Copy link
Copy Markdown
Contributor Author

Ready for final review @ArthurZucker ;)

ArthurZucker
ArthurZucker approved these changes Mar 20, 2025
View reviewed changes

@ArthurZucker ArthurZucker left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know about the image patch changes in the processor but the rest LGTM.
Just make sure we don't have issue with some of the code that is replaced!

Comment thread src/transformers/models/aya_vision/modular_aya_vision.py Outdated

class AyaVisionMultiModalProjector(LlavaMultiModalProjector):
def __init__(self, config: AyaVisionConfig):
LlavaMultiModalProjector().__init__()

@ArthurZucker ArthurZucker Mar 20, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

super should just work here as well

Comment thread src/transformers/models/aya_vision/processing_aya_vision.py

@ArthurZucker ArthurZucker Mar 20, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

are these expected?

@yonigozlan yonigozlan Mar 20, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes they are not needed anymore :)

Comment thread src/transformers/models/aya_vision/modeling_aya_vision.py
Comment on lines -301 to -309
def tie_weights(self):
return self.language_model.tie_weights()

def resize_token_embeddings(self, new_num_tokens: Optional[int] = None, pad_to_multiple_of=None) -> nn.Embedding:
model_embeds = self.language_model.resize_token_embeddings(new_num_tokens, pad_to_multiple_of)
# update vocab size
self.config.text_config.vocab_size = model_embeds.num_embeddings
self.vocab_size = model_embeds.num_embeddings
return model_embeds

@ArthurZucker ArthurZucker Mar 20, 2025

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these are needed

@yonigozlan
yonigozlan merged commit 6a26279 into huggingface:main Mar 20, 2025
zucchini-nlp pushed a commit to zucchini-nlp/transformers that referenced this pull request May 14, 2025
@yonigozlan @zucchini-nlp
Refactor Aya Vision with modular (huggingface#36688)
85c6dac 
* refactor aya_vision with modular (incorrect docstring)

* Fix docstrings

* Fix other modulars

* fix docstring

* revert changes

* add tie_weights and resize_token_embeddings
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ArthurZucker ArthurZucker ArthurZucker approved these changes
@Rocketknight1 Rocketknight1 Awaiting requested review from Rocketknight1

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@yonigozlan @HuggingFaceDocBuilderDev @Cyrilvallez @ArthurZucker