Skip to content

Add possibility to input multiple sentences#4

Merged
simonepri merged 13 commits intosimonepri:masterfrom
dldk-gael:batch_input
May 1, 2020
Merged

Add possibility to input multiple sentences#4
simonepri merged 13 commits intosimonepri:masterfrom
dldk-gael:batch_input

Conversation

@dldk-gael
Copy link
Contributor

@dldk-gael dldk-gael commented Apr 28, 2020

In order to be able to fully benefit from parallelization, I propose to add the possibility to input the sentences into the transformer models by batch.

The work is not finished yet and I still have to pass the CI test (I will proceed as you suggest in #3) but it is working and would like to have your opinion before going further.

In order to make minimal change to your code, for now tokens_score return a list of log_probs, ids, tokens (one item for each sentence). However, it would be much more efficient to make the reduction when we still have the log_probs scores as a tensor. One possibility would be that _tokens_log_prob now return a tensor of shape (number sentences, max sentences length). What do you think of that ?

Also, I did not use the pad_sequence function from torch.nn.utils.rnn but recode it in order that it returns a mask (with 1 at the place of the padding value) which is useful latter in the code to remove the score from padding value. I think this function should not be in GPT2LMScorer class but somewhere else.

@dldk-gael dldk-gael changed the title add possibility to input sentences by batch Add possibility to input sentences by batch Apr 28, 2020
Copy link
Owner

@simonepri simonepri left a comment

Choose a reason for hiding this comment

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

I left some comments more are coming.

Also if you don't manage to run the code formatter on your computer, you can copy-paste the code here: https://black.now.sh/

@dldk-gael
Copy link
Contributor Author

I have taking into account all your comments. There is still one typing issue, however not sure how to resolve this one. Am i supposed to use a # type ignore or is there an other possibility ?

@simonepri simonepri added the enhancement New feature or request label Apr 30, 2020
@simonepri
Copy link
Owner

Am i supposed to use a # type ignore or is there an other possibility ?

No, it is correctly pointing out a bug in the code

@simonepri simonepri mentioned this pull request Apr 30, 2020
@dldk-gael
Copy link
Contributor Author

dldk-gael commented Apr 30, 2020

I will also add some more tests concerning this new batch features in test_gpt2.

@simonepri
Copy link
Owner

simonepri commented Apr 30, 2020

Would you mind if I ask you to split this PR in two?

It would be convenient to have a first PR (we can use this one) with the API changes + GPT2 implemented as just a for loop on the old single sentence code.

Then the second PR will actually optimize GPT2 using batching.
In this way, we can have the new API merged for #3 in a timely manner.

@simonepri simonepri marked this pull request as draft April 30, 2020 22:18
@dldk-gael dldk-gael changed the title Add possibility to input sentences by batch Add possibility to input multiple sentences May 1, 2020
Copy link
Owner

@simonepri simonepri left a comment

Choose a reason for hiding this comment

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

Some small changes, we are almost ready to merge it.
Thanks for the work!!

@simonepri simonepri marked this pull request as ready for review May 1, 2020 14:30
dldk-gael and others added 2 commits May 1, 2020 20:44
Co-authored-by: Simone Primarosa <simonepri@outlook.com>
Co-authored-by: Simone Primarosa <simonepri@outlook.com>
@simonepri simonepri merged commit 5531890 into simonepri:master May 1, 2020
@simonepri
Copy link
Owner

@dldk-gael Thanks!

@dldk-gael
Copy link
Contributor Author

No problem and thank you for your patience, I was not familiar with all those tools and good practices, I learned a lot from your code.

@dldk-gael dldk-gael deleted the batch_input branch May 1, 2020 19:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants