-
Notifications
You must be signed in to change notification settings - Fork 2.5k
chore: Update code snippets in docs (audio and builders components) #10156
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
base: main
Are you sure you want to change the base?
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Pull Request Test Coverage Report for Build 19763653080Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have the impression that we should clarify the goal of this task.
In general, I would like to avoid errors in snippets, but also readability for users seem relevant to me.
(@dfokina can probably judge this better than me)
| whisper = RemoteWhisperTranscriber(api_key=Secret.from_token("<your-api-key>"), model="tiny") | ||
| transcription = whisper.run(sources=["path/to/audio/file"]) | ||
| whisper = RemoteWhisperTranscriber(api_key=Secret.from_env_var("OPENAI_API_KEY"), model="whisper-1") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure... Secret.from_token("<your-api-key>") was a meaningful placeholder.
Secret.from_env_var("OPENAI_API_KEY") is just the default value. (If we want to go this route, we can just remove the api_key parameter from the example)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can just use whisper = RemoteWhisperTranscriber(model="whisper-1")
If you think it's helpful, add a comment about setting the OPENAI_API_KEY env var.
| # Output example (truncated): | ||
| # {'llm': {'replies': [ChatMessage(...)]}} | ||
| >> {'llm': {'replies': [ChatMessage(_role=<ChatRole.ASSISTANT: 'assistant'>, _content=[TextContent(text= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the long output can be helpful for users
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok will put it back in
releasenotes/notes/fix-pydoc-examples-audio-builders-7b1d2d945099bb16.yaml
Outdated
Show resolved
Hide resolved
@anakin87 the goal is to make sure that pydocs are not just some hand-waving examples that actually don't work. As in usage of tiny whisper model that doesn't exist on OpenAI provider endpoint. We want to actually run these snippets nightly and confirm they are actually valid, executable scripts - to minimize the drift from pydocs and reality. |
Yes, I understand. I just have the impression that testing everything without compromising user learning experience is challenging. But let's keep this work going and see how it evolves. |
Totally, not taking this to extreme that every single pydoc code snippet has to run or.... but where it makes sense and it doesn't compromise learning experience. There is a way to tag snippet not to be run so where it is highly impractical to actually run the snippet we mark it so. |
|
@dfokina I'll let you decide here for these changes. I'm ok with whatever you think is the best direction for audio and builders and in the meantime I'll remove the reno note that should def. not be there |
This reverts commit 18733fa.
|
@vblagoje @anakin87 I don't mind these changes honestly, I still think the examples are meaningful for the audience, plus we add some more explanations in the documentation guides. Our goal was always to make the code immediately runnable, which is not possible yet for the snippets inside the guides, so there are these placeholders, so why not use actual files and values in the docstrings :) |
| # no parameter init, we don't use any runtime template variables | ||
| prompt_builder = ChatPromptBuilder() | ||
| llm = OpenAIChatGenerator(api_key=Secret.from_token("<your-api-key>"), model="gpt-4o-mini") | ||
| llm = OpenAIChatGenerator(api_key=Secret.from_env_var("OPENAI_API_KEY"), model="gpt-5-mini") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
We can just use llm = OpenAIChatGenerator(model="gpt-5-mini")
If you think it's helpful, add a comment about setting the OPENAI_API_KEY env var.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, deal!
Update of audio and builder pydocs components to ensure: