Addition of a DialoguePipeline #5516

guillaume-be · 2020-07-04T10:45:00Z

Addition of a Conversation object to keep track of multi-turn conversation
Creation of a DialoguePipeline to process Conversations using the history context
Integration tests for DialoguePipeline using microsoft/DialoGPT-medium

Addition of input processing and history concatenation

…sation inputs

…ion_pipeline # Conflicts: # tests/test_pipelines.py

guillaume-be · 2020-07-04T12:45:11Z

This is a back-port of guillaume-be/rust-bert#57. I did not implement the ConversationManager as I felt it did not quite fit the general API of this library. I however added the concept of Conversations keeping track of past user inputs, generated responses and the history token ids. Conversations include a print option that will display the entire dialogue.

print(conversation)

Conversation id: 2716da3e-8cde-4071-97bc-218d88764b7b 
user >> What's the last book you have read? 
bot >> The Last Question 
user >> Why do you recommend it? 
bot >> It's a good book.

(ps: note that this example is the response of DialoGPT-medium without sampling, which is an interesting coincidence for a computer-generated response)

codecov · 2020-07-04T12:47:23Z

Codecov Report

Merging #5516 into master will increase coverage by 1.49%.
The diff coverage is 84.34%.

@@            Coverage Diff             @@
##           master    #5516      +/-   ##
==========================================
+ Coverage   78.35%   79.85%   +1.49%     
==========================================
  Files         146      146              
  Lines       26454    26568     +114     
==========================================
+ Hits        20729    21215     +486     
+ Misses       5725     5353     -372

Impacted Files	Coverage Δ
src/transformers/__init__.py	`99.24% <ø> (ø)`
src/transformers/pipelines.py	`79.36% <84.34%> (+0.86%)`	⬆️
src/transformers/modeling_tf_flaubert.py	`24.22% <0.00%> (-63.98%)`	⬇️
src/transformers/tokenization_t5.py	`71.83% <0.00%> (-23.95%)`	⬇️
src/transformers/modeling_tf_gpt2.py	`71.65% <0.00%> (-23.68%)`	⬇️
src/transformers/generation_tf_utils.py	`86.46% <0.00%> (+0.25%)`	⬆️
src/transformers/generation_utils.py	`97.11% <0.00%> (+0.28%)`	⬆️
src/transformers/tokenization_utils.py	`90.36% <0.00%> (+0.40%)`	⬆️
src/transformers/modeling_tf_auto.py	`66.66% <0.00%> (+3.63%)`	⬆️
src/transformers/modeling_tf_distilbert.py	`98.79% <0.00%> (+34.61%)`	⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 91cb954...9734829. Read the comment docs.

src/transformers/pipelines.py

patrickvonplaten · 2020-07-15T17:42:49Z

src/transformers/pipelines.py

+
+        conversation_1.add_user_input("Is it an action movie?")
+
+        conversation_pipeline([conversation_1, conversation_2])


Suggested change

conversation_pipeline([conversation_1, conversation_2])

dialogue_pipeline([conversation_1, conversation_2])

patrickvonplaten · 2020-07-15T17:53:58Z

src/transformers/pipelines.py

+            self.pad_token_id = self.tokenizer.eos_token_id
+        self.min_response_allowed_length = kwargs.get("min_response_allowed_length", 32)
+
+    def __call__(self, *args, clean_up_tokenization_spaces=True, **generate_kwargs):


Can we call args => conversations?

patrickvonplaten · 2020-07-15T17:57:11Z

src/transformers/pipelines.py

+                    [conversation.new_user_input for conversation in active_conversations]
+                )
+                histories = [conversation.history for conversation in active_conversations]
+                max_length = generate_kwargs.get("max_length", 1000)


I think I would prefer to not set the default max_length to 1000 here. The user can set the default value for each model individually in the model's config (under task specific params) => compare for example with XLNet here: https://s3.amazonaws.com/models.huggingface.co/bert/xlnet-base-cased-config.json .

Suggested change

max_length = generate_kwargs.get("max_length", 1000)

max_length = generate_kwargs.get("max_length", self.model.config.max_length)

I agree the value of 1000 is arbitrary (taken from the illustrative example in the model card). The issue is that the DialoGPT configuration files do not set a max_length. To my understanding, this means that without specifying it to the generate method, it will be set to the GPT2 default, that is 20. This seems very low for a conversation pipeline as the input alone is likely to exceed this value. I am not sure defaulting to the configuration value is going to be a good user experience. Maybe the way to go is to force the user to provide a value? Or maybe update the configuration of the DialoGPT configuration files?

patrickvonplaten · 2020-07-15T17:58:23Z

src/transformers/pipelines.py

+                            input_length, max_length
+                        )
+                    )
+                generate_kwargs["max_length"] = max_length


Suggested change

generate_kwargs["max_length"] = max_length

patrickvonplaten · 2020-07-15T17:58:59Z

src/transformers/pipelines.py

+
+                cleaned_history = self._clean_padding_history(generated_responses)
+                if isinstance(args[0], Conversation):
+                    args[0].mark_processed()


would be nice to rename args to conversations

patrickvonplaten · 2020-07-15T18:03:29Z

src/transformers/pipelines.py

+        Builds an input prepended by the history for this conversation, allowing multi-turn conversation with context
+        """
+        outputs = []
+        for input, history in zip(inputs, histories):


Very nice! I like it

patrickvonplaten · 2020-07-15T18:08:45Z

src/transformers/pipelines.py

+            on the associated CUDA device id.
+    """
+
+    def __init__(self, *args, **kwargs):


I think as it is implemented now dialogue_pipeline = pipeline("dialogue", min_response_allowed_length=32) would throw an error because it is passed to super().__init__(*args, **kwargs) => can we just change it to:

def __init__(self, min_respones_allowed_length=32, *args, **kwargs): super().__init__(*args, **kwargs)

and maybe the name min_length_for_response is better here. The word allowed is confusing me a bit. What do you think @guillaume-be ?

Good catch and agreed - will update

patrickvonplaten · 2020-07-15T18:11:39Z

tests/test_pipelines.py

+    @require_torch
+    def test_integration_torch_dialogue(self):
+        # When
+        nlp = pipeline(task="dialogue", device=DEFAULT_DEVICE_NUM)


Maybe pass min_response_allowed_length or (IMO clearer name min_length_for_response) here to test it.

patrickvonplaten

I like the PR! Thanks a lot @guillaume-be!

This makes the dialogue pipeline a bit different from the other pipelines in that it expects a Conversation object instead of a string, but that's OK IMO.

One other option would be to integrate the conversation class under-the-hood into the DialoguePipeline so that the user would always either input a string or a list of strings. This way the user would not have to use the predefined Conversation object, but could just input strings. The advantage is that we don't need to expose Conversation this way - the disadvantage is that we would need an additional function for DialoguePipeline that shows the current conversation.

IMO, the logic / design is good as it is now. Since Dialogue is a special pipeline, the user will have to create a Conversation object first - which is OK for me. What do you think @julien-c @mfuntowicz (also considering the connection to the API?)

One thing which we should still add here @guillaume-be is to add Conversation (or DialoguePipelineConversation so it's clear that the class is related to pipelines) to __init__ so that the user can import it directly from transformers (we more or less expose all classes in transformers as far as I know).

- Added `min_length_for_response` as an initialization parameter - Renamed `*args` to `conversations`, `conversations` being a `Conversation` or a `List[Conversation]` - Updated truncation to truncate entire segments of conversations, instead of cutting in the middle of a user/bot input

- removed hardcoded default value of 1000 and use config.max_length instead - added `append_response` and `set_history` method to the Conversation class to avoid direct fields mutation - fixed bug in history truncation method

…(otherwise a ValueError is raised)

julien-c · 2020-07-16T19:15:50Z

LGTM

…on tests

…ion_pipeline # Conflicts: # src/transformers/pipelines.py

guillaume-be · 2020-07-23T20:22:37Z

@julien-c @patrickvonplaten I believe all comments have been addressed - please let me know if I have missed anything. Just resolved the conflict with master. Getting an error with code quality, not quite sure what is wrong as I did not change the torch requirement with this PR.

julien-c · 2020-07-23T21:58:29Z

Given that patrickvonplaten is off for one more week I believe, do you want to give this PR a last look-over @sgugger and merge if it's fine?

LysandreJik

This is really cool! I left a few comments, as I think there's a few remaining bugs.

I think this would greatly benefit from having a documentation, which would document both the pipeline and the Conversation. The pipeline already has some documentation, but it would need to be added to the pipelines.rst file. You could add it beneath the GenerationPipeline, alongside a bit of docs for the Conversation class.

LysandreJik · 2020-07-30T10:35:37Z

src/transformers/pipelines.py

+        "tf": TFAutoModelWithLMHead if is_tf_available() else None,
+        "pt": AutoModelWithLMHead if is_torch_available() else None,


Nitpick, those two classes are deprecated

Suggested change

"tf": TFAutoModelWithLMHead if is_tf_available() else None,

"pt": AutoModelWithLMHead if is_torch_available() else None,

"tf": TFAutoModelForCausalLM if is_tf_available() else None,

"pt": AutoModelForCausalLM if is_torch_available() else None,

LysandreJik · 2020-07-30T10:42:57Z

src/transformers/pipelines.py

+        conversational_pipeline = pipeline("conversational")
+
+        conversation_1 = Conversation("Going to the movies tonight - any suggestions?")
+        conversation_2 = Conversation("What's the last book you have read?")
+
+        conversational_pipeline([conversation_1, conversation_2])
+
+        conversation_1.add_user_input("Is it an action movie?")
+
+        conversational_pipeline([conversation_1, conversation_2])


This example does not work for me. It fails with the following:

ValueError: Conversation with UUID <class 'uuid.UUID'> does not contain new user input to process. Add user inputs with the conversation's `add_user_input` method

A user input must be added to the second conversation as well for that example to work.

Suggested change

conversational_pipeline = pipeline("conversational")

conversation_1 = Conversation("Going to the movies tonight - any suggestions?")

conversation_2 = Conversation("What's the last book you have read?")

conversational_pipeline([conversation_1, conversation_2])

conversation_1.add_user_input("Is it an action movie?")

conversational_pipeline([conversation_1, conversation_2])

conversational_pipeline = pipeline("conversational")

conversation_1 = Conversation("Going to the movies tonight - any suggestions?")

conversation_2 = Conversation("What's the last book you have read?")

conversational_pipeline([conversation_1, conversation_2])

conversation_1.add_user_input("Is it an action movie?")

conversation_2.add_user_input("What is the genre of this book?")

conversational_pipeline([conversation_1, conversation_2])

LysandreJik · 2020-07-30T10:43:13Z

src/transformers/pipelines.py

+    Usage::
+        conversational_pipeline = pipeline("conversational")


Needs a line return to be correctly rendered in the docs

Suggested change

Usage::

conversational_pipeline = pipeline("conversational")

Usage::

conversational_pipeline = pipeline("conversational")

LysandreJik · 2020-07-30T10:44:19Z

src/transformers/pipelines.py

+        Builds an input prepended by the history for this conversation, allowing multi-turn conversation with context
+        """
+        outputs = []
+        for input, history in zip(inputs, histories):


Would it be possible to change input to something not shadowing the built-in input?

LysandreJik · 2020-07-30T10:50:33Z

src/transformers/pipelines.py

+                    cutoff_eos_index = input[cutoff_eos_index:].index(self.tokenizer.eos_token_id)
+                    if cutoff_eos_index == 0 or cutoff_eos_index == len(input) - 1:
+                        break
+                    else:
+                        input = input[cutoff_eos_index + 1 :]


This should also break when the cutoff_eos_index is larger than the length of the remaining input. Otherwise it fails because input[cutoff_eos_index:] returns an empty list, which cannot be indexed with self.tokenizer.eos_token_id.

An easy fix is the following, which could probably be made cleaner.

Suggested change

cutoff_eos_index = input[cutoff_eos_index:].index(self.tokenizer.eos_token_id)

if cutoff_eos_index == 0 or cutoff_eos_index == len(input) - 1:

break

else:

input = input[cutoff_eos_index + 1 :]

if cutoff_eos_index >= len(input):

break

cutoff_eos_index = input[cutoff_eos_index:].index(self.tokenizer.eos_token_id)

if cutoff_eos_index == 0 or cutoff_eos_index == len(input) - 1:

break

else:

input = input[cutoff_eos_index + 1 :]

…ion_pipeline # Conflicts: # src/transformers/pipelines.py # tests/test_pipelines.py

…, addition of docstrings for Conversation, added both to the docs

guillaume-be · 2020-07-30T15:42:07Z

@LysandreJik Thank you very much for the review. Good catch on the behaviour of the eos token cut-off. I have updated based on your suggestions, and added docstrings to the Conversation class. I have also added both Conversation and ConversationalPipeline to the top-level __init__ for consistency with the other pipelines.

LysandreJik

Great, thanks for iterating @guillaume-be!

src/transformers/pipelines.py

sgugger · 2020-07-30T16:51:24Z

Thanks for the PR @guillaume-be
Docstrings could be improved but I'll clean up the docs in the pipelines file soon, so will take of that. For future PRs, please remember that thing will render thing in italics in the docs, and not in code (you have to use thing or :obj:thing).

Updated docsting following review Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>

guillaume-be · 2020-07-30T16:56:41Z

@sgugger Thank you for the review - was indeed a typo on my end. The tests got triggered again and unfortunately a hash verification on torch fails. Could you please restart the build if you have a chance?

julien-c · 2020-07-31T09:30:32Z

This is awesome, congrats everyone on shipping this! 🔥

sshleifer · 2020-10-22T19:58:28Z

test_torch_conversation and test_integration_torch_conversation are broken on github actions CI. Could someone fix or delete? https://github.com/huggingface/transformers/runs/1289790225?check_suite_focus=true

LysandreJik · 2020-10-22T20:05:45Z

Will take a look.

thomwolf · 2020-10-22T22:06:03Z

Should be fixed in #7970

guillaume-be added 10 commits July 4, 2020 12:39

initial commit for pipeline implementation

a398564

Addition of input processing and history concatenation

Conversation pipeline tested and working for single & multiple conver…

c08fb5f

…sation inputs

Added docstrings for dialogue pipeline

29cc23c

Addition of dialogue pipeline integration tests

4c3d5c4

Merge remote-tracking branch 'remotes/upstream/master' into conversat…

00160a1

…ion_pipeline # Conflicts: # tests/test_pipelines.py

Delete test_t5.py

629dde9

Fixed max code length

d17cf21

Updated styling

f2c19cb

Fixed test broken by formatting tools

ce28bfb

Removed unused import

210aeee

guillaume-be mentioned this pull request Jul 4, 2020

Multiturn conversation guillaume-be/rust-bert#57

Merged

guillaume-be added 3 commits July 4, 2020 15:01

Added unit test for DialoguePipeline

8c5853d

Fixed Tensorflow compatibility

0f1e33e

Fixed multi-framework support using framework flag

7d40145

patrickvonplaten self-requested a review July 6, 2020 17:24

patrickvonplaten reviewed Jul 15, 2020

View reviewed changes

src/transformers/pipelines.py Outdated Show resolved Hide resolved

patrickvonplaten reviewed Jul 15, 2020

View reviewed changes

patrickvonplaten requested review from julien-c and mfuntowicz July 15, 2020 18:19

guillaume-be added 3 commits July 16, 2020 20:50

- renamed pipeline name from dialogue to conversational

55f4f6b

- removed hardcoded default value of 1000 and use config.max_length instead - added `append_response` and `set_history` method to the Conversation class to avoid direct fields mutation - fixed bug in history truncation method

- Updated ConversationalPipeline to accept only active conversations …

4371c51

…(otherwise a ValueError is raised)

- Simplified input tensor conversion

d26242c

guillaume-be added 4 commits July 16, 2020 21:26

- Updated attention_mask value for Tensorflow compatibility

92c042b

- Updated last dialogue reference to conversational & fixed integrati…

da3c68b

…on tests

Merge remote-tracking branch 'remotes/upstream/master' into conversat…

77d1826

…ion_pipeline # Conflicts: # src/transformers/pipelines.py

Fixed conflict with master

8d6288b

LysandreJik requested changes Jul 30, 2020

View reviewed changes

guillaume-be added 4 commits July 30, 2020 17:00

Merge remote-tracking branch 'remotes/upstream/master' into conversat…

b295de4

…ion_pipeline # Conflicts: # src/transformers/pipelines.py # tests/test_pipelines.py

Updates following review comments

7f6ff5e

Updated formatting

d1918be

Added Conversation and ConversationalPipeline to the library __init__…

396c4c4

…, addition of docstrings for Conversation, added both to the docs

LysandreJik approved these changes Jul 30, 2020

View reviewed changes

sgugger approved these changes Jul 30, 2020

View reviewed changes

src/transformers/pipelines.py Outdated Show resolved Hide resolved

Update src/transformers/pipelines.py

9734829

Updated docsting following review Co-authored-by: Sylvain Gugger <35901082+sgugger@users.noreply.github.com>

guillaume-be requested a review from julien-c July 30, 2020 17:19

sgugger merged commit e642c78 into huggingface:master Jul 30, 2020

julien-c mentioned this pull request Aug 6, 2020

Multi-turn dialogue using dialoGPT huggingface/hfapi#1

Open

guillaume-be mentioned this pull request Oct 13, 2020

Update of DialoGPT max_length #7764

Closed

thomwolf mentioned this pull request Oct 22, 2020

[tests|tokenizers] Refactoring pipelines test backbone - Small tokenizers improvements - General tests speedups #7970

Merged

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Addition of a DialoguePipeline #5516

Addition of a DialoguePipeline #5516

guillaume-be commented Jul 4, 2020

guillaume-be commented Jul 4, 2020 •

edited

Loading

codecov bot commented Jul 4, 2020 •

edited

Loading

patrickvonplaten Jul 15, 2020

patrickvonplaten Jul 15, 2020

patrickvonplaten Jul 15, 2020

patrickvonplaten Jul 15, 2020

guillaume-be Jul 15, 2020

patrickvonplaten Jul 15, 2020

patrickvonplaten Jul 15, 2020

patrickvonplaten Jul 15, 2020

patrickvonplaten Jul 15, 2020

patrickvonplaten Jul 15, 2020

guillaume-be Jul 15, 2020

patrickvonplaten Jul 15, 2020

patrickvonplaten left a comment

julien-c commented Jul 16, 2020

guillaume-be commented Jul 23, 2020 •

edited

Loading

julien-c commented Jul 23, 2020

LysandreJik left a comment

LysandreJik Jul 30, 2020 •

edited

Loading

LysandreJik Jul 30, 2020

LysandreJik Jul 30, 2020

LysandreJik Jul 30, 2020

LysandreJik Jul 30, 2020

guillaume-be commented Jul 30, 2020

LysandreJik left a comment

sgugger commented Jul 30, 2020

guillaume-be commented Jul 30, 2020 •

edited

Loading

julien-c commented Jul 31, 2020

sshleifer commented Oct 22, 2020

LysandreJik commented Oct 22, 2020

thomwolf commented Oct 22, 2020


		conversation_1.add_user_input("Is it an action movie?")

		conversation_pipeline([conversation_1, conversation_2])

	conversation_pipeline([conversation_1, conversation_2])
	dialogue_pipeline([conversation_1, conversation_2])

	max_length = generate_kwargs.get("max_length", 1000)
	max_length = generate_kwargs.get("max_length", self.model.config.max_length)

		"tf": TFAutoModelWithLMHead if is_tf_available() else None,
		"pt": AutoModelWithLMHead if is_torch_available() else None,

Addition of a DialoguePipeline #5516

Addition of a DialoguePipeline #5516

Conversation

guillaume-be commented Jul 4, 2020

guillaume-be commented Jul 4, 2020 • edited Loading

codecov bot commented Jul 4, 2020 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickvonplaten left a comment

Choose a reason for hiding this comment

julien-c commented Jul 16, 2020

guillaume-be commented Jul 23, 2020 • edited Loading

julien-c commented Jul 23, 2020

LysandreJik left a comment

Choose a reason for hiding this comment

LysandreJik Jul 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guillaume-be commented Jul 30, 2020

LysandreJik left a comment

Choose a reason for hiding this comment

sgugger commented Jul 30, 2020

guillaume-be commented Jul 30, 2020 • edited Loading

julien-c commented Jul 31, 2020

sshleifer commented Oct 22, 2020

LysandreJik commented Oct 22, 2020

thomwolf commented Oct 22, 2020

guillaume-be commented Jul 4, 2020 •

edited

Loading

codecov bot commented Jul 4, 2020 •

edited

Loading

guillaume-be commented Jul 23, 2020 •

edited

Loading

LysandreJik Jul 30, 2020 •

edited

Loading

guillaume-be commented Jul 30, 2020 •

edited

Loading