Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

uniformize processor Mllama #33876

Merged
merged 3 commits into from
Oct 2, 2024
Merged

uniformize processor Mllama #33876

merged 3 commits into from
Oct 2, 2024
+59 −30

Conversation

yonigozlan
Copy link
Member

What does this PR do?

Adds uniformized processors following #31911 for Mllama.

  • Very small changes in Mllama processor to be coherent with other vlms processors
  • Add ProcessorMixin to Mllama processor tests

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?

Who can review?

@qubvel @molbap

Sorry, something went wrong.

@HuggingFaceDocBuilderDev
Copy link

Hey! 🤗 Thanks for your contribution to the transformers library!

Before merging this pull request, slow tests CI should be triggered. To enable this:

  • Add the run-slow label to the PR
  • When your PR is ready for merge and all reviewers' comments have been addressed, push an empty commit with the command [run-slow] followed by a comma separated list of all the models to be tested, i.e. [run_slow] model_to_test_1, model_to_test_2
    • If the pull request affects a lot of models, put at most 10 models in the commit message
  • A transformers maintainer will then approve the workflow to start the tests

(For maintainers) The documentation for slow tests CI on PRs is here.

@yonigozlan yonigozlan requested review from qubvel and molbap October 1, 2024 22:53
yonigozlan
yonigozlan commented Oct 1, 2024
View reviewed changes
tests/models/mllama/test_processor_mllama.py
self.bos_token = processor.bos_token
self.bos_token_id = processor.tokenizer.bos_token_id
self.tmpdirname = tempfile.mkdtemp()
processor.save_pretrained(self.tmpdirname)
Copy link
Member Author

Choose a reason for hiding this comment

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

changed the use of self.processor to this to have a "clean" processor at the beginning of each test.

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a good idea, could be replaced by a fixture with a function scope then?

Copy link
Member Author

@yonigozlan yonigozlan Oct 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

That's a good idea, could be replaced by a fixture with a function scope then?

Actually my bad, it looks like the setUp and tearDown functions are already fixtures with a function scope for unittest. So it's not necessary to use tempfile.
I don't know then why saving processors to a tempfile is used in the setUp function of so many test_processor files, and why tempfile in general is used in so many unittest setUp function in Transformers.
Unless there is a good reason to use tempfiles, maybe we should think about removing its use in setUp functions in all tests?

Copy link
Member Author

Choose a reason for hiding this comment

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

self.tmpdirname is used in ProcessorTesterMixin so I can't remove it here in fact, but maybe we could consider removing it from ProcessorTesterMixin in another PR

Copy link
Contributor

Choose a reason for hiding this comment

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

No strong opinion on it - it's just convenient to use and makes sure the save/load from files works, can't create race conditions either. Since so many of the transformers utils are designed to work in pair with the hub, I suppose it makes sense to use tempfile in that regard

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh I see, I have no strong opinion on it either, so maybe no need to change it :)

@HuggingFaceDocBuilderDev
Copy link

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.

molbap
molbap approved these changes Oct 2, 2024
View reviewed changes
Copy link
Contributor

@molbap molbap left a comment

Choose a reason for hiding this comment

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

LGTM! passing to @qubvel as well

src/transformers/models/mllama/processing_mllama.py Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

try:
    from typing import Unpack
except ImportError:
    from typing_extensions import Unpack

should be replaced by the import of Unpack from processing utils

tests/models/mllama/test_processor_mllama.py
self.bos_token = processor.bos_token
self.bos_token_id = processor.tokenizer.bos_token_id
self.tmpdirname = tempfile.mkdtemp()
processor.save_pretrained(self.tmpdirname)
Copy link
Contributor

Choose a reason for hiding this comment

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

That's a good idea, could be replaced by a fixture with a function scope then?

tests/models/mllama/test_processor_mllama.py Outdated
class MllamaProcessorTest(unittest.TestCase):
class MllamaProcessorTest(ProcessorTesterMixin, unittest.TestCase):
processor_class = MllamaProcessor

def setUp(self):
self.checkpoint = "hf-internal-testing/mllama-11b" # TODO: change
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this TODO still needed?

Copy link
Member

Choose a reason for hiding this comment

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

No, it's not, can be removed

qubvel
qubvel approved these changes Oct 2, 2024
View reviewed changes
Copy link
Member

@qubvel qubvel left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Agreed with the comments above, other than that, looks great to me! Thanks for updating it

ArthurZucker
ArthurZucker approved these changes Oct 2, 2024
View reviewed changes
Copy link
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.

LGTM Thanks for fixing

@yonigozlan yonigozlan force-pushed the uniformize-processor-mllama branch from f23d64f to a657967 Compare October 2, 2024 14:45
@yonigozlan yonigozlan merged commit d7950bf into huggingface:main Oct 2, 2024
11 checks passed
BernardZach pushed a commit to BernardZach/transformers that referenced this pull request Dec 5, 2024
@yonigozlan @BernardZach
uniformize processor Mllama (huggingface#33876)
591fa88 
* uniformize processor Mllama

* nit syntax

* nit
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@molbap molbap

@qubvel qubvel

@ArthurZucker ArthurZucker

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@yonigozlan @HuggingFaceDocBuilderDev @qubvel @molbap @ArthurZucker