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

Added API Proxy functionality to API LM #68

Merged
merged 4 commits into from
Sep 23, 2024
Merged

Added API Proxy functionality to API LM #68

merged 4 commits into from
Sep 23, 2024

Conversation

AakritiKinra
Copy link
Collaborator

@AakritiKinra AakritiKinra commented Sep 15, 2024

Description

Added the api_base parameter to the completion() and batch_completion() functions to use the API Proxy.

References

@AakritiKinra AakritiKinra changed the title Added base_url to API LM Added API Proxy functionality to API LM Sep 15, 2024
Copy link
Contributor

@neubig neubig left a comment

Choose a reason for hiding this comment

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

Thanks @AakritiKinra !

But I think this changes the signature of APIBasedLM.generate() to be different than that of the parent LanguageModel.generate(). We should keep the signature of parent and child abstract functions the same.

Instead, I think you can specify api_base in the constructor of APIBasedLM.

@AakritiKinra
Copy link
Collaborator Author

I have added the api_base to the constructor. The signature of the two generate functions is still different because of the completion function. I am passing the message which is the prompt for which the response is generated. In the generate function of the LanguageModel class, there is no message parameter. The chat_generate function of both classes has messages as a parameter.

@AakritiKinra AakritiKinra requested a review from neubig September 16, 2024 15:26
@neubig
Copy link
Contributor

neubig commented Sep 17, 2024

it looks like the modified version is not passing tests. We’ll have to make sure that the “generate” interface is the same for all LMs

@AakritiKinra AakritiKinra self-assigned this Sep 20, 2024
@neubig
Copy link
Contributor

neubig commented Sep 23, 2024

Thanks!

@neubig neubig merged commit ecd8691 into main Sep 23, 2024
5 checks passed
@neubig neubig deleted the api-lm branch September 23, 2024 14:40
rohanmodi2810 pushed a commit that referenced this pull request Sep 26, 2024
* added base_url

* Updated function descriptions

* Added api_base to the constructor

* matched structure with lm class

Pull latest changes
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