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

fix:fix response format bug #282

Merged
merged 9 commits into from
Mar 8, 2023
Merged

fix:fix response format bug #282

merged 9 commits into from
Mar 8, 2023

Conversation

guaguaguaxia
Copy link
Contributor

No description provided.

@guaguaguaxia guaguaguaxia changed the title fix:fix bug fix:fix response format bug Mar 7, 2023
@guaguaguaxia
Copy link
Contributor Author

@MattFisher please review it, i don't know how i close last same PR 😭

@MattFisher
Copy link

MattFisher commented Mar 7, 2023

I'm not a maintainer of the project or anything, just another user!

Though as I said here, I think this will fix the issue without further code changes.

The Content-Type header on the response is always set correctly (at least for the audio transcription responses), so we can deduce whether to json-decode within APIRequestor._interpret_response.

For the json and verbose_json response formats, the Content-Type is 'application/json', while for text, srt, and vtt, the Content-Type is 'text/plain; charset=utf-8'.

@guaguaguaxia
Copy link
Contributor Author

I'm not a maintainer of the project or anything, just another user!

ok thanks

@MattFisher
Copy link

I've manually tested the change and it works for all the different audio response formats. I can't verify that it doesn't break any other endpoints though.

@guaguaguaxia
Copy link
Contributor Author

@MattFisher I thought about it for a second,i still think we should define a enum and add a function parameter to make code more clearly

@MattFisher
Copy link

@guaguaguaxia that would make sense if the Audio module was the only code that consumed the APIRequestor class, but it's not. APIRequestor is used all through the codebase, so if we were going to add an extra parameter to its __init__ method, that would imply that all the other modules need to know about that parameter. In this case, that doesn't make sense, because the choices for that parameter are specific to the Audio transcribe function.
Other callers of APIRequestor() would then expect to be able to pass response_format='text' and get back some textual representation of the response, which isn't going to work.

@guaguaguaxia
Copy link
Contributor Author

@guaguaguaxia that would make sense if the Audio module was the only code that consumed the APIRequestor class, but it's not. APIRequestor is used all through the codebase, so if we were going to add an extra parameter to its __init__ method, that would imply that all the other modules need to know about that parameter. In this case, that doesn't make sense, because the choices for that parameter are specific to the Audio transcribe function. Other callers of APIRequestor() would then expect to be able to pass response_format='text' and get back some textual representation of the response, which isn't going to work.

you r right, i am not familiar with this project structure

@MattFisher
Copy link

Might want to roll back the whitespace changes in openai/util.py

@guaguaguaxia
Copy link
Contributor Author

@MattFisher thanks.

@logankilpatrick @mpokrass you can see this:#243 (comment)

@hallacy
Copy link
Collaborator

hallacy commented Mar 8, 2023

Looks awesome! Thanks for writing the PR!

@hallacy hallacy merged commit 9b5f8db into openai:main Mar 8, 2023
@dusdb1
Copy link

dusdb1 commented Mar 8, 2023

Now, response_format is in plain text and not defaults to json format as it says in docs, right?

@guaguaguaxia
Copy link
Contributor Author

Now, response_format is in plain text and not defaults to json format as it says in docs, right?

No,if response_format type is json or verbose_json,it still return json,other return text

@stainless-bot stainless-bot mentioned this pull request Nov 6, 2023
davedittrich pushed a commit to davedittrich/openai-python that referenced this pull request Nov 14, 2023
* fix:fix bug

* fix:fix response_format bug

* fix:fix response_format bug

* fix:fix response_format bug

* fix:fix response_format bug

* fix:fix response_format bug

* fix:fix response_format bug

* fix:fix response_format bug
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.

4 participants