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

Use thought flag. #376

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

MarkDaoust
Copy link
Contributor

No description provided.

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@github-actions github-actions bot added the status:awaiting review PR awaiting review from a maintainer label Dec 19, 2024
@MarkDaoust MarkDaoust requested a review from Giom-V December 19, 2024 18:24
Copy link
Collaborator

Giom-V commented Dec 20, 2024

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?

Copy link

review-notebook-app bot commented Dec 20, 2024

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


Copy link

review-notebook-app bot commented Dec 20, 2024

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.


Copy link

review-notebook-app bot commented Dec 20, 2024

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.


Copy link

review-notebook-app bot commented Dec 20, 2024

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 v1alpha API, you'll see a thought=True, indicating that the first part is indeed thoughts."


Copy link

review-notebook-app bot commented Dec 20, 2024

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?


Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:awaiting review PR awaiting review from a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants