Open ai wrapper #1371
Open ai wrapper #1371
Conversation
|
Should we put this in datasource or in service package instead? |
|
Currently it's in data source. I had some confusions about this. So at last after that issue discussion I decided to build it in data source. |
|
@ccoVeille I have updated the PR. |
|
@ccoVeille I have updated the pr and replied to some of the conversations. |
|
@Umang01-hash approval with write access required for the PR. it's already reviewed. |
|
@Umang01-hash @vikash a review required with write access. |
|
@ccoVeille @Umang01-hash in workflow-pipeline PKG unit-testing v1.22 passes but PKG unit-testing v1.21 fails and says was cancelled. |
There was a problem hiding this comment.
Thank you for your contribution! I have reviewed the changes, and I have a suggestion regarding the directory structure:
Since OpenAI is primarily a communication service and not a database, it would be more appropriate to place this under the service directory instead of datasource. This would better reflect its role as a service for generating responses.
Would you please consider moving the OpenAI integration to the service directory?
Also we need to make updates in the documentation regarding any new feature addition so that other users can know about it easily. Please also add the documenttaion regarding this wrapper and an example of how to use it.
|
is it okay ? i have placed openai folder in service folder. |
|
@yash-sojitra I think you might have forgot to push your commit.. |
|
I was just asking i haven't yet committed changes. |
|
@yash-sojitra Please go ahead and commit the changes |
|
@yash-sojitra are you still active on the issue? Please let us know if you have any doubts regarding the PR or the issue |
|
Yupp I have done the changes to code. I didn't got much time to complete documentation. Will be doing in short time. Also a thing I noticed since this is an optional service package I thought to add it to container struct just like external dbs are added and this open ai service will be added and accessible via gofr.context just like data sources. Is this approach to add OpenAI services good ? |
|
No problems. It was very insightful as it was my first issue into open source. |
You did right then. I can tell by the way you reacted to the feedbacks that you were open to critics. You confirm it by telling you find feedbacks insightful. We all started with open-source PR. Yours is pretty good. |
|
@Umang01-hash @coolwednesday PR is ready for review. |
|
@yash-sojitra Please complete the review changes suggested by @coolwednesday. And please also resolve the merge conflicts in your PR. |
|
@coolwednesday i have some comments on conversation if you can review about it. other changes i have done. |
I think it will be better to add link to the documentation page of OpenAI package. |
|
@yash-sojitra , Thanks for bearing with me. I just have few more doubts .... Do you plan to extend this interface in future ? If yes, how would you address the breaking change when you would be changing the interface .... ? |
|
I will be adding more function rather than changing existing implementations. I have currently implemented only one endpoint That is create completions. |
Here are the logs, it is actually trying to print the reference to whatever you are passing there, please fix it.... you can just give meaningful information there whatever fits the usecase..... Also, here my API key had less limit so an error was returned in the response but a 200 code is shown ...to handle this you must fix check for error in your received response. ... If present you return the error.... or else you return the response and nil for error ....this would help set proper status codes in GoFr. Also is |
|
|
||
| c.metrics.RecordHistogram(ctx, "openai_api_request_duration", float64(duration)) | ||
| c.metrics.IncrementCounter(ctx, "openai_api_total_request_count") | ||
| c.metrics.DeltaUpDownCounter(ctx, "openai_api_token_usage", float64(ql.Usage.TotalTokens)) | ||
|
|
There was a problem hiding this comment.
Add an attribute operation that is createCompletion for some context in the metric .....
|
I want to just confirm one thing. do I need to make error struct in openai package ? because i saw that how http status codes are passed to the user and in that, error was implementing certain interface so do i need to implement it in my package ? |
|
I am making an error struct that implements built in error interface and also StatusCodeResponder that the framework uses to set the status code of the current request. And now all the errors will be passed wrapped in this struct with appropriate status code. It will also be a valid error type since it implements built in error interface. This will be my approach to handle particular status codes will it be okay? |
Hey ! Yeah that works. Do add some tests so that, we can verify how are you setting status codes through examples. |
|
it like ErrorDB |
|
Hi @yash-sojitra, Thank you for your effort on this PR! After carefully reviewing the changes, I feel that the current implementation might be quite complex from an end developer’s perspective. To improve the developer experience, we should consider some design refinements—specifically, introducing a unified interface for all AI agents to abstract away the underlying complexity. Given this, I’d suggest keeping this PR on hold for now. It would be great to start a discussion (on Discord or GitHub) to align on the best approach for integrating AI agent usage in GoFr through a unified interface before proceeding with the implementation. Apologies for any inconvenience, and I truly appreciate your contributions. I also recommend closing this PR for now and revisiting it after we finalize the design direction. Looking forward to your thoughts! |
|
Yes, it will be best to provide interface for all ai agents in the market. I am looking forwards to this discussion on discord. |
I have created a discussion on discord. Please let me know what you feel about the approach there .... |
Also, let's close this PR for the time being It can be refactored when you push the refactored code.... |
#1242
Description:
Checklist:
goimportandgolangci-lint.