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

fix: TextIteratorStreamer cannot work with pipeline #23641

Merged
merged 5 commits into from
Jun 13, 2023
Merged

fix: TextIteratorStreamer cannot work with pipeline #23641

merged 5 commits into from
Jun 13, 2023

Conversation

yuanwu2017
Copy link
Contributor

@yuanwu2017 yuanwu2017 commented May 22, 2023

Deepcopying the TextIteratorStreamer object causes the exception.

What does this PR do?

Fixes #23552

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?

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.

Sorry, something went wrong.

@HuggingFaceDocBuilderDev
Copy link

HuggingFaceDocBuilderDev commented May 22, 2023

The documentation is not available anymore as the PR was closed or merged.

Deepcopying the TextIteratorStreamer object causes the exception.

Signed-off-by: yuanwu <yuan.wu@intel.com>
@yao-matrix
Copy link

@sgugger , pls help review, thx.

@yao-matrix
Copy link

@sywangyi

@yuanwu2017
Copy link
Contributor Author

@gante Please help to review.

@opcode81
Copy link

I ran into the same issue and also had another issue about a week ago (which I don't remember now) that was also related to the deepcopy operation. If the sole purpose of the deep copy is to allow calls to pop without modifying the dictionary, then shouldn't a shallow copy do?

@yuanwu2017
Copy link
Contributor Author

@gante We used the transformers pipeline with TextIteratorStreamer for chatbot, but it raised the deepcopy exception. Can you help to check the issue and review the PR?
Thanks a lot.

@sgugger
Copy link
Collaborator

sgugger commented May 26, 2023

@gante is on vacation, please be patient until he comes back :-)
Also cc @Narsil

@yuanwu2017
Copy link
Contributor Author

@gante is on vacation, please be patient until he comes back :-) Also cc @Narsil

Got it. Thanks.

Copy link
Member

@gante gante left a comment

Choose a reason for hiding this comment

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

Apologies for the long delay 🤗 Have a look at the suggestion I added, I believe it solves the problem while keeping it short!

src/transformers/pipelines/text_generation.py Outdated Show resolved Hide resolved
Got it. I will update the patch.

Co-authored-by: Joao Gante <joaofranciscocardosogante@gmail.com>
@yuanwu2017
Copy link
Contributor Author

Do I need to fix these test failures which are not related with this patch? @gante

@gante
Copy link
Member

gante commented Jun 12, 2023

@yuanwu2017 to get rid of the CI errors, please rebase the PR. It is a hard requirement -- LMK if you need instructions to do so :)

yuanwu2017 and others added 2 commits June 13, 2023 01:02
@gante gante requested a review from amyeroberts June 12, 2023 17:26
@gante
Copy link
Member

gante commented Jun 12, 2023

@amyeroberts see the issue here, which explains why this copy is not needed :)

Copy link
Collaborator

@amyeroberts amyeroberts left a comment

Choose a reason for hiding this comment

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

Thanks for fixing!

@gante gante merged commit d7389cd into huggingface:main Jun 13, 2023
@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint.

novice03 pushed a commit to novice03/transformers that referenced this pull request Jun 23, 2023
* fix: TextIteratorStreamer cannot work with pipeline

Deepcopying the TextIteratorStreamer object causes the exception.

Signed-off-by: yuanwu <yuan.wu@intel.com>

* Update src/transformers/pipelines/text_generation.py

Got it. I will update the patch.

Co-authored-by: Joao Gante <joaofranciscocardosogante@gmail.com>

* Update src/transformers/pipelines/text_generation.py

Co-authored-by: Joao Gante <joaofranciscocardosogante@gmail.com>

* Update text_generation.py

---------

Signed-off-by: yuanwu <yuan.wu@intel.com>
Co-authored-by: Joao Gante <joaofranciscocardosogante@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.

TextIteratorStreamer cannot be used with TextGenerationPipeline
7 participants