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

Pass **kwargs in "generate" function instead of passing each argument by name #236

Merged
merged 2 commits into from
Aug 3, 2023

Conversation

DavidMChan
Copy link
Contributor

Instead of wrapping the huggingface generate by copying each argument, it might be better to pass the arguments dynamically -- thus, when the underlying generate API changes, or more features are added, we can take advantage of them directly (instead of having to continue wrapping the arguments). This also gives users more flexibility to override the arguments.

The only default that I noticed was different from the default arguments in huggingface is open_flamingo sets top_k=0 instead of top_k=50. If we want to maintain parity with the existing code, we could easily overwrite this, but it might be better to just have the default arguments be in parity with the huggingface arguments.

The only downside to this I can see is that we lose some nice auto-completions in text editors, but I think this tradeoff is worth it.

Testing:

  • I ran this locally with OpenFlamingo-3B-vitl-mpt1b and OpenFlamingo-9B-vitl-mpt7b, but there's no official tests, so I can't say that it for sure passes.

@anas-awadalla
Copy link
Collaborator

Looks great! I think you just need to run the code formatter for us to merge. We use black so can you run black . in the open_flamingo directory and push again?

@DavidMChan
Copy link
Contributor Author

Yep! Sorry about that! I didn't realize my black config differed from yours!

@anas-awadalla
Copy link
Collaborator

That was quick 😄! Ok merging thanks!!

@anas-awadalla anas-awadalla merged commit e44d60e into mlfoundations:main Aug 3, 2023
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.

2 participants