-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Use thought
flag.
#376
base: main
Are you sure you want to change the base?
Use thought
flag.
#376
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
As you'll see in my big comment, I have mixed feeling about using a function to display the output of the calls. I think we should keep the examples self-understandable as much as possible when it's just about a couple easy to understand lines.
That said, I'm also concerned that the tag is only available in v1alpha as someone jumping directly to the examples won't understand why it's not working for them, so I'm not sure it's a good idea to have our examples be reliant on the alpha API.
Personally I would only add streaming (and maybe chat) for the time being and wait for v1beta to display the thought tag before updating the rest. @markmcd WDYT? |
View / edit / reply to this conversation on ReviewNB Giom-V commented on 2024-12-20T10:40:38Z I think we should add a line that explain that the "thought" flag is only available in v1alpha at the moment, but that everything else work with the default version of the API |
View / edit / reply to this conversation on ReviewNB Giom-V commented on 2024-12-20T10:40:39Z I have mixed feelings about using functions for that TBH. On one hand it makes the examples easier to read, but on the other end they are harder to understand as you might have to do back and forth between the function definition and where it is used. I usually try to avoid it whenever possible because I think our code snippets should be as self-understandable as possible (which is why I rewrote it compared to your code example).
Now that we have the thoughts I have even more ambivalent feelings about it as we should explain that tag in details. So because we need it to be clear my personal take would be: add somewhere an explanation of the thoughts tag, and instead of using a function write the loop in each example. The reason being that it will be clearer that this tag exists if someone jumps directly to a specific one.
Maybe if we go towards the function path, we should at least add a check on whether there's inline_data content in the answers for the sake of robustness. |
View / edit / reply to this conversation on ReviewNB Giom-V commented on 2024-12-20T10:40:40Z Line #1. # First part is the inner thoughts of the model You need to rerun this cell and the next one so that the content is the same as in the previous one. EDIT: I think you did since there are the thoughts/answer tittles but it's not displayed as updated for some reason. |
View / edit / reply to this conversation on ReviewNB Giom-V commented on 2024-12-20T10:40:40Z You'll need to remove this part: "If you are using the |
View / edit / reply to this conversation on ReviewNB Giom-V commented on 2024-12-20T10:40:42Z Maybe we could just replace an existing example and add "streaming" in its title? I think we should also for chat. It could be first example is unary, second is chat, third is streaming. WDYT? |
No description provided.