Cleanup return_text and return_full_text options in TextGenerationPipeline #33542
Conversation
|
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. |
|
cc @LysandreJik for review! |
|
Maybe cc @amyeroberts actually, since Lysandre is still away (also I know this is 6 on a Friday, don't worry about reviewing it until Monday!) |
| # Explicitly setting return_full_text = False is caught here, and results in ReturnType.NEW_TEXT | ||
| return_type = ReturnType.NEW_TEXT |
There was a problem hiding this comment.
I'm confused why we would have ReturnType.NEW_TEXT if return_text=None and return_full_text=False
| with self.assertRaises(ValueError): | ||
| outputs = text_generator("test", return_full_text=True, return_text=True) |
There was a problem hiding this comment.
I'm also not sure why we remove a check that they're mutually exclusive -- I think it's far better to raise an exception if both are set rather than have one silently override the other
There was a problem hiding this comment.
I was trying to reconcile the docs (which used to be very wrong) with the options, but you're right - I'll make them mutually exclusive like they were before, and update the docs instead.
| if return_type is not None and ( | ||
| return_text is not None or return_full_text is not None or return_tensors is not None | ||
| ): | ||
| raise ValueError( | ||
| "`return_type` is mutually exclusive with `return_text`, `return_full_text`, and `return_tensors`" | ||
| ) | ||
| if return_tensors is not None: | ||
| if return_text or return_full_text: | ||
| raise ValueError("`return_text` and `return_full_text` are mutually exclusive with `return_tensors`") | ||
| return_type = ReturnType.TENSORS | ||
| elif return_full_text and return_type is None: | ||
| return_type = ReturnType.FULL_TEXT | ||
| elif (return_text or return_full_text is not None) and return_type is None: | ||
| # Explicitly setting return_full_text = False is caught here, and results in ReturnType.NEW_TEXT | ||
| return_type = ReturnType.NEW_TEXT |
There was a problem hiding this comment.
This whole block is really hard to parse, is it possible to add white space/comments?
Rocketknight1
force-pushed
the
update_return_full_text
branch
from
5107db2 to
776e25c
Compare
|
@amyeroberts @LysandreJik I reworked the PR in response to Amy's comment - I think she was right about the new behaviour not being any better. I restored the tests and behaviour to how they were before, and instead updated the docs so they correctly reflect that behaviour. As a result, this PR only touches docstrings now! |
…eline (huggingface#33542) * Cleanup return_text and return_full_text options in TextGenerationPipeline * Cleanup return_text and return_full_text options in TextGenerationPipeline * Cleanup return_text and return_full_text options in TextGenerationPipeline * Cleanup return_text and return_full_text options in TextGenerationPipeline * Revert pipeline code, but update docs instead * Restore pipeline test
…eline (huggingface#33542) * Cleanup return_text and return_full_text options in TextGenerationPipeline * Cleanup return_text and return_full_text options in TextGenerationPipeline * Cleanup return_text and return_full_text options in TextGenerationPipeline * Cleanup return_text and return_full_text options in TextGenerationPipeline * Revert pipeline code, but update docs instead * Restore pipeline test
…eline (huggingface#33542) * Cleanup return_text and return_full_text options in TextGenerationPipeline * Cleanup return_text and return_full_text options in TextGenerationPipeline * Cleanup return_text and return_full_text options in TextGenerationPipeline * Cleanup return_text and return_full_text options in TextGenerationPipeline * Revert pipeline code, but update docs instead * Restore pipeline test
…eline (huggingface#33542) * Cleanup return_text and return_full_text options in TextGenerationPipeline * Cleanup return_text and return_full_text options in TextGenerationPipeline * Cleanup return_text and return_full_text options in TextGenerationPipeline * Cleanup return_text and return_full_text options in TextGenerationPipeline * Revert pipeline code, but update docs instead * Restore pipeline test
The extra options in
TextGenerationPipelineright now are confusing and poorly documented. This PR refactors things so that the documentation is clearer. It also creates a proper hierarchy of options (return_full_textoverrides and ignoresreturn_text, andreturn_tensorsis incompatible with both)Before this PR, the documentation was incorrect, as it claimed that
return_full_textrequiredreturn_text, whereas actually these options were not compatible with each other!Behaviour after the change should be mostly unchanged, with the exception that
return_full_textoverridesreturn_textif both are set, rather than raising an error. The test that checked for those two options being incompatible has been removed.Fixes #33535