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.

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

Avoid using torch's Tensor or PIL's Image in chat template utils if not available #34165

Merged
merged 2 commits into from
Oct 16, 2024
Merged

Conversation

RezaRahemtola
Copy link
Contributor

@RezaRahemtola RezaRahemtola commented Oct 14, 2024

What does this PR do?

This PR adds conditional checks to the types mapping definitions to avoid defining types that are conditionally imported previously, if the same check doesn't succeed.

This allows a user (like me) to use transformers for simple stuff like tokenizers without needed to install PIL and/or torch if not needed (I'm not using the tokenizers library directly as it doesn't include chat templating functions, and I'm not using torch/tensorflow/JAX as I'm just calling models run locally with llamacpp, installing one of those just for chat templating seems overkill).

Before, a simple call to tokenizer.apply_chat_template would result in this error:

  File "...../venv/lib/python3.11/site-packages/transformers/utils/chat_template_utils.py", line 79, in _get_json_schema_type
    Image: {"type": "image"},
    ^^^^^
NameError: name 'Image' is not defined

With this fix, there's not error: if PIL (or torch) isn't installed, it's not imported (already the case) and not used (added here).

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?

@RezaRahemtola RezaRahemtola marked this pull request as ready for review October 14, 2024 19:05
@LysandreJik
Copy link
Member

Also cc @zucchini-nlp

Copy link
Member

@Rocketknight1 Rocketknight1 left a comment

Choose a reason for hiding this comment

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

Yes, this looks good to me! For clarity, this function is called when auto-generating JSON schema for functions passed as tools to the chat template interface.

The main thing to be wary of here is that this change might cause slightly different behaviour on systems where Image and Tensor types are unavailable. However, I think we can safely assume that when the user has defined a function with Image or Tensor type hints in the header, that means they have the relevant libraries.

@HuggingFaceDocBuilderDev

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.

@Rocketknight1
Copy link
Member

@zucchini-nlp @LysandreJik this PR is quite straightforward, and I think it's okay to accept - are you okay with me merging it once tests pass?

@zucchini-nlp
Copy link
Member

I am not qualified for review here, so whatever you find reasonable @Rocketknight1 :)

@Rocketknight1
Copy link
Member

Rocketknight1 commented Oct 16, 2024

In that case, since this is a simple bugfix, I think it's okay to merge. One last step, though - @RezaRahemtola can you fix up the code quality tests? To do that:

1) pip install transformers[quality]
2) make style in the transformers directory
3) commit and push

Never mind - this is actually a CI issue. Rerunning it, you don't need to do anything @RezaRahemtola !

Copy link
Member

@LysandreJik LysandreJik left a comment

Choose a reason for hiding this comment

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

Let's go!

@Rocketknight1
Copy link
Member

Documentation build is failing but this PR doesn't touch the docs at all, so I'm assuming that's an issue with the doc builder. Merging, and thanks @RezaRahemtola for the PR!

@Rocketknight1 Rocketknight1 merged commit 3a10c61 into huggingface:main Oct 16, 2024
21 of 23 checks passed
BernardZach pushed a commit to BernardZach/transformers that referenced this pull request Dec 5, 2024
…ot available (huggingface#34165)

* fix(utils): Avoid using torch Tensor or PIL Image if not available

* Trigger CI

---------

Co-authored-by: Matt <rocketknight1@gmail.com>
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.

5 participants