Skip to content

Fix modular model converter unable to generate Processor classes#33737

Merged
ArthurZucker merged 1 commit intohuggingface:mainfrom
tonywu71:fix-modular-model-converter-issue-with-processor
Sep 26, 2024
Merged

Fix modular model converter unable to generate Processor classes#33737
ArthurZucker merged 1 commit intohuggingface:mainfrom
tonywu71:fix-modular-model-converter-issue-with-processor

Conversation

@tonywu71
Copy link
Copy Markdown
Contributor

@tonywu71 tonywu71 commented Sep 26, 2024

What does this PR do?

Modular 🤗 transformers is awesome, but a typo in the code prevents the user to generate a processing_xxx.py file from modular_xxx.py. This PR solves this problem.

Before submitting

  • This PR fixes a typo or improves the docs (you can dismiss the other checks if that's the case).
  • Did you read the contributor guideline,
    Pull Request section?
  • Was this discussed/approved via a Github issue or the forum? Please add a link
    to it if that's the case.
  • Did you make sure to update the documentation with your changes? Here are the
    documentation guidelines, and
    here are tips on formatting docstrings.
  • Did you write any new necessary tests?

Note: I believe the last 3 checks don't apply to my PR. Feel free to correct me if I'm wrong!

Who can review?

E2E Test

Before:

❯ python utils/modular_model_converter.py --files_to_parse src/transformers/models/colpali/modular_colpali.py
Converting src/transformers/models/colpali/modular_colpali.py to a single model single file format
Traceback (most recent call last):
  File "/Users/tony/Documents/illuin-repositories/transformers/utils/modular_model_converter.py", line 781, in <module>
    converted_files = convert_modular_file(file_name, args.old_model_name, args.new_model_name)
                      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/tony/Documents/illuin-repositories/transformers/utils/modular_model_converter.py", line 726, in convert_modular_file
    wrapper.visit(cst_transformers)
  File "/Users/tony/Documents/illuin-repositories/transformers/.venv/lib/python3.11/site-packages/libcst/metadata/wrapper.py", line 204, in visit
    return self.module.visit(visitor)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/tony/Documents/illuin-repositories/transformers/.venv/lib/python3.11/site-packages/libcst/_nodes/module.py", line 89, in visit
    result = super(Module, self).visit(visitor)
             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/tony/Documents/illuin-repositories/transformers/.venv/lib/python3.11/site-packages/libcst/_nodes/base.py", line 227, in visit
    _CSTNodeSelfT, self._visit_and_replace_children(visitor)
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/tony/Documents/illuin-repositories/transformers/.venv/lib/python3.11/site-packages/libcst/_nodes/module.py", line 74, in _visit_and_replace_children
    body=visit_body_sequence(self, "body", self.body, visitor),
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/tony/Documents/illuin-repositories/transformers/.venv/lib/python3.11/site-packages/libcst/_nodes/internal.py", line 227, in visit_body_sequence
    return tuple(visit_body_iterable(parent, fieldname, children, visitor))
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/tony/Documents/illuin-repositories/transformers/.venv/lib/python3.11/site-packages/libcst/_nodes/internal.py", line 193, in visit_body_iterable
    new_child = child.visit(visitor)
                ^^^^^^^^^^^^^^^^^^^^
  File "/Users/tony/Documents/illuin-repositories/transformers/.venv/lib/python3.11/site-packages/libcst/_nodes/base.py", line 236, in visit
    leave_result = visitor.on_leave(self, with_updated_children)
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/tony/Documents/illuin-repositories/transformers/.venv/lib/python3.11/site-packages/libcst/_visitors.py", line 71, in on_leave
    updated_node = leave_func(original_node, updated_node)
                   ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/Users/tony/Documents/illuin-repositories/transformers/utils/modular_model_converter.py", line 681, in leave_ClassDef
    self.files[key][class_name] = {"insert_idx": self.global_scope_index, "node": updated_node}
    ~~~~~~~~~~^^^^^
KeyError: 'processor'

After, with the proposed fix:

❯ python utils/modular_model_converter.py --files_to_parse src/transformers/models/colpali/modular_colpali.py
Converting src/transformers/models/colpali/modular_colpali.py to a single model single file format

Copy link
Copy Markdown
Collaborator

@ArthurZucker ArthurZucker left a comment

Choose a reason for hiding this comment

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

Good job! Indeed, would you like to ad an example in the examples? 🤗 seeing as a model will use it, let's go!

@tonywu71
Copy link
Copy Markdown
Contributor Author

Good job! Indeed, would you like to ad an example in the examples? 🤗 seeing as a model will use it, let's go!

I think I'll finish my ColPali integration PR first, then open a new PR to showcase the modular transformers in the examples. Wdyt? 😉

@HuggingFaceDocBuilderDev
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.

@ArthurZucker ArthurZucker merged commit e1b1508 into huggingface:main Sep 26, 2024
@ArthurZucker
Copy link
Copy Markdown
Collaborator

Sounds good!

BernardZach pushed a commit to BernardZach/transformers that referenced this pull request Dec 5, 2024
…gingface#33737)

fix: fix wrong file type for processor in `modular_model_converter.py`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants