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.

Sign up for GitHub

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

Jump to bottom

Fix PPLM #8779

Merged
merged 3 commits into from
Nov 26, 2020
Merged

Fix PPLM #8779

merged 3 commits into from
Nov 26, 2020

Conversation

chutaklee
Copy link
Contributor

What does this PR do?

API changes break PPLM example, this PR should fix it. However I haven't test it on 'run_pplm_discrim_train.py'.

Fixes # (issue)

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 which may be interested in your PR.

@LysandreJik
Copy link
Member

PPLM is unfortunately not maintained anymore. The fix would be to pin the transformers version in the PPLM README.

@julien-c
Copy link
Member

pinging @w4nderlust and @mimosavvy just in case

@w4nderlust
Copy link
Contributor

This looks good to me, I believe the return of dict (very welcome change by the way) from the model should be the only thing breaking the pplm code, right?

@chutaklee
Copy link
Contributor Author

This looks good to me, I believe the return of dict (very welcome change by the way) from the model should be the only thing breaking the pplm code, right?

Yeah and the named argument of model past_key_values. I have no issue running the provided example command in here and python run_pplm_discrim_train.py --dataset SST --epochs 1 --batch_size 8.

patrickvonplaten
patrickvonplaten approved these changes Nov 26, 2020
View reviewed changes
Copy link
Contributor

@patrickvonplaten patrickvonplaten left a comment

Choose a reason for hiding this comment

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

Thanks a lot!

@patrickvonplaten
Copy link
Contributor

Can you run make style to fix the code quality check? Then we should be good for merge :-)

@patrickvonplaten patrickvonplaten merged commit 52708d2 into huggingface:master Nov 26, 2020
@chutaklee chutaklee deleted the mybranch branch November 27, 2020 00:45
@w4nderlust
Copy link
Contributor

Much appreciated!

stas00 pushed a commit to stas00/transformers that referenced this pull request Dec 5, 2020
@chutaklee @patrickvonplaten @stas00
Fix PPLM (huggingface#8779)
07235d9 
* Fix pplm

* fix style

* make style

Co-authored-by: Patrick von Platen <patrick.v.platen@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@patrickvonplaten patrickvonplaten patrickvonplaten approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@chutaklee @LysandreJik @julien-c @w4nderlust @patrickvonplaten