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

Remove unused arguments from openai generate functions. #376

Merged
merged 1 commit into from
Nov 17, 2023

Conversation

HerrIvan
Copy link
Contributor

@HerrIvan HerrIvan commented Nov 17, 2023

Removes unused arguments in generate_base and generate_choice openai model functions.

  • self.client was not necessary, as the function already uses a callable pointing to a method of the self.client instance.
  • max_tokens is not used in generate_choice.
  • reduces samples to 1 when calling the openai API in generate_choice, as discussed in Reduce token consumption for "choice" generation with openAI. #368 . This fix is not too relevant, as the extra token-cost generated per call was just samples-1, but it corrects the code. Note that the alternative solution suggested is not feasible: One can call the api in parallel, but only with a same prompt. As the different samples diverge, both prompts and masks do so, hence any parallelization should be upstream from the API call.

@HerrIvan HerrIvan changed the title removes unused arguments from openai generate functions. Remove unused arguments from openai generate functions. Nov 17, 2023
@rlouf
Copy link
Member

rlouf commented Nov 17, 2023

Thank you! Can we go all the way and remove it from generate_base? Also we don't need the client to be a property of the class.

@HerrIvan
Copy link
Contributor Author

  • removed client from as an argument from generate_base, it somehow escaped me.
  • removed as a property from the class. It is created but just stored in the closure.
  • Warning: please check if y changes to these kind of arguments ( signature="(),(),(m),()->(s)" ) are ok.

@rlouf rlouf merged commit f1f5c07 into dottxt-ai:main Nov 17, 2023
5 checks passed
@rlouf
Copy link
Member

rlouf commented Nov 17, 2023

Thank you!

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