adding OpenAI compatibility quickstart #459
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
github-actions
bot
added
status:awaiting review
|
View / edit / reply to this conversation on ReviewNB markmcd commented on 2025-02-17T04:19:19Z Not a blocker, but having all imports in one cell (ordered by line length) lowers readability.
Conventionally we import packages in the cell where they are first used. Important or heavily used imports (like the SDK under documentation, or the IPython imports) could go up front but otherwise it's easier to read when they are kept with their usage (so readers can answer questions like "what is
It's not codified other by convention, but we discussed it a little in this doc. lucianommartins commented on 2025-02-22T01:55:29Z fixed, thanks |
|
View / edit / reply to this conversation on ReviewNB markmcd commented on 2025-02-17T04:19:20Z Text is unclear. The first "If" implies that they may not need a key which is not true for users of the cookbook.
And instead of "If you still don't have an API key", be prescriptive and say "To get an API key..."
Also we have a common authentication cell if you want to be consistent with the rest of the content. Giom-V commented on 2025-02-17T11:18:28Z I think it should also be a level 2 title. lucianommartins commented on 2025-02-22T02:38:36Z fixed, thanks @markmcd. |
|
View / edit / reply to this conversation on ReviewNB markmcd commented on 2025-02-17T04:19:21Z Line #14. api_key=GOOGLE_API_KEY, base_url="https://generativelanguage.googleapis.com/v1beta/"
This is probably the most important line of this document. Can you split it out and explain what is happening? lucianommartins commented on 2025-02-17T12:59:55Z I'm assuming this guide targets people already using the OpenAI library and to show them how to use Gemini on their codes/experiments. My intention is not to show people how to start using the OpenAI library from scratch, understanding how the library works, etc (ie. this is the common way to instantiate a client using the library - the same arguments, etc).
But if you really think it could add better understanding for the audience, I can certainly add more details.
wdyt? |
|
View / edit / reply to this conversation on ReviewNB markmcd commented on 2025-02-17T04:19:21Z Can you include a section on listing models too? lucianommartins commented on 2025-02-22T02:02:41Z done. |
|
View / edit / reply to this conversation on ReviewNB markmcd commented on 2025-02-17T04:19:22Z Fix throughout - for task-based headings, they must be bare infinitive verb phrases (think: "Do this"). So prefer something like "Generate content" over text like this that is passively descriptive (and drop terms like simple).
And for the sentence, can you mention that you are introducing the OAI SDK here? e.g. "For your first request, use the OpenAI SDK to perform text generation with a text prompt." lucianommartins commented on 2025-02-22T02:04:06Z done. |
|
View / edit / reply to this conversation on ReviewNB markmcd commented on 2025-02-17T04:19:23Z Is this a reasoning example? The output saved in the notebook zero-shots the code and then retrospectively describes it.
I think this could look worse than the competitors example.
We should either use a model that does reasoning/thinking, induce it in the prompt, or just drop this example. lucianommartins commented on 2025-02-22T02:06:31Z fixed. |
|
View / edit / reply to this conversation on ReviewNB markmcd commented on 2025-02-17T04:19:24Z Line #2. Write a bash script that takes a matrix represented as a string with
This prompt appears to be from OpenAI documentation. You need to either properly cite the license and copyright for this, or use a novel prompt. lucianommartins commented on 2025-02-22T02:15:22Z done. I replaced by a C program. |
|
View / edit / reply to this conversation on ReviewNB markmcd commented on 2025-02-17T04:19:24Z IMPORTANT: Using the OpenAI compatibility, you can work with images and audio files.
Does this mean we can't do unstructured files and videos? This might be confusing with the text in the preceding paragraph. Could we clarify a bit? e.g. "The OpenAI SDK compatibility only supports (inline?) images and audio files. For video and ... use the Gemini API's Python SDK." (adds "only" for precision and a call to action) lucianommartins commented on 2025-02-22T02:41:16Z done. |
|
View / edit / reply to this conversation on ReviewNB markmcd commented on 2025-02-17T04:19:25Z Uber nit, but you don't need to precede code blocks with colons. Just use a normal sentence. lucianommartins commented on 2025-02-22T02:45:02Z done. |
|
View / edit / reply to this conversation on ReviewNB markmcd commented on 2025-02-17T04:19:26Z Line #23. "url": f"data:image/jpeg;base64,{encoded_image}",
Nit: does it matter that the mime type here is incorrect? lucianommartins commented on 2025-02-22T02:29:28Z fixed. |
|
View / edit / reply to this conversation on ReviewNB markmcd commented on 2025-02-17T04:19:26Z Could you simplify this using the IPython API?
for url in image_urls: display(Image(url=url, width=200, height=200))
I realise it's 4x1 instead of 2x2 but it's only 2 LOC. lucianommartins commented on 2025-02-22T02:47:27Z it is way uglier now :) but done. |
|
View / edit / reply to this conversation on ReviewNB markmcd commented on 2025-02-17T04:19:27Z Line #22. {
Idea: use a list comp here instead? e.g.
"content": [
{
"type": "text",
"text": "Describe for what type of living room each of those items are the best match",
},
] + [
{
"type": "image_url",
"image_url": {
"url": f"data:image/jpeg;base64,{image_data}",
},
}
for image_data in encoded_images
],
lucianommartins commented on 2025-02-22T02:35:03Z fixed, thanks. |
|
View / edit / reply to this conversation on ReviewNB markmcd commented on 2025-02-17T04:19:28Z Text is unclear here. Second sentence could be something like "Audio data provides a more rich input than text alone, and can be use for tasks like transcription, or as direct prompting like a voice assistant." lucianommartins commented on 2025-02-22T02:48:18Z done. |
|
View / edit / reply to this conversation on ReviewNB markmcd commented on 2025-02-17T04:19:28Z Line #3. audio = open(audio_filename, "rb")
Bug: the function argument is
Nit: we should
lucianommartins commented on 2025-02-22T02:52:05Z done. |
|
View / edit / reply to this conversation on ReviewNB markmcd commented on 2025-02-17T04:19:29Z This is a cool demo, but does this need to be this complex? The only thing the SDK is being used for is the JSON output. Can you simplify this to show just that case?
e.g. on the website we use a basic recipe class and ask for some cookie recipes. Giom-V commented on 2025-02-17T09:03:41Z I also think that quickstarts should focus on the features we want to show in the most simple way with simple examples (and bonus points, it will be easier to reuse in the doc). Complex examples like this one should be kept for a notebook dedicated on structured output.
We could also say the same of the images examples, I think only the example with one image is sufficient for this notebook. lucianommartins commented on 2025-02-22T02:38:00Z I really don't like the cookie recipes example tbh (especially because we use it everywhere). I would prefer a practical use case (ie. using a PDF file). |
|
View / edit / reply to this conversation on ReviewNB markmcd commented on 2025-02-17T04:19:30Z Line #6. You will be given unstructured text from a research paper and should convert it into the given structure.
This appears to be an OpenAI prompt too. lucianommartins commented on 2025-02-22T02:54:18Z fixed. |
|
View / edit / reply to this conversation on ReviewNB markmcd commented on 2025-02-17T04:19:31Z Line #20. print(json.dumps(dict(completion.choices[0].message.parsed), indent=2))
Since this is a pydantic object you can just dump the model:
print(completion.choices[0].message.parsed.model_dump_json(indent=2)) lucianommartins commented on 2025-02-22T02:54:56Z done. |
|
View / edit / reply to this conversation on ReviewNB markmcd commented on 2025-02-17T04:19:31Z Remove the code block from
Watch the colons too - the text preceding should be a standalone sentence (ending with "is" leaves the sentence incomplete).
And I don't think you need too much exposition on function calling. You can link to the existing docs on the site or in the cookbook. lucianommartins commented on 2025-02-22T02:58:34Z done. |
|
View / edit / reply to this conversation on ReviewNB markmcd commented on 2025-02-17T04:19:32Z Line #12. print(len(response.data[0].embedding))
Remove the whole embedding dump in the output. You can just print the
e.g.
print(len(embedding)) print(embedding[:4], '...') lucianommartins commented on 2025-02-22T02:59:55Z done. |
|
View / edit / reply to this conversation on ReviewNB markmcd commented on 2025-02-17T04:19:33Z Line #1. cos_sim_array = cosine_similarity(list(df.embeddings.values))
Any reason not to just use lucianommartins commented on 2025-02-22T03:03:58Z because I'm performing a cosine similarity to use as basis to distance estimation - df@df.T gives me a dot product without normalization which is not exactly what the sklearn cosine similarity does. |
|
View / edit / reply to this conversation on ReviewNB markmcd commented on 2025-02-17T04:19:34Z Check out the Google GenAI SDK for more details on the new SDK.
Drop "new" for timelessness. Add something to explain that there's a more fully-featured first-party SDK that they should check out.
check the Gemini 2.0 folder of the cookbook
Most of the content here just redirects users elsewhere.
Related examples
If we want to call them "related" WDYT about pointing out that they are using the 1p sdk? not the SDK from this guide. lucianommartins commented on 2025-02-22T03:06:09Z done. |
markmcd
added
status:awaiting response
|
I also think that quickstarts should focus on the features we want to show in the most simple way with simple examples (and bonus points, it will be easier to reuse in the doc). View entire conversation on ReviewNB |
|
I think it should also be a level 2 title. View entire conversation on ReviewNB |
I think it's OK either way as long as we keep the naming convention (get started guides are named Get_Started_something and feature guides are named after the features). |
|
View / edit / reply to this conversation on ReviewNB patrickloeber commented on 2025-02-20T10:57:55Z my 2 cents: I'd move the imports below to when they are used except for openai and maybe some of the core imports. 15 imports and also the install line for lucianommartins commented on 2025-02-22T03:08:50Z fixed on previous comments. thanks. |
|
fixed, thanks View entire conversation on ReviewNB |
|
done. View entire conversation on ReviewNB |
|
done. View entire conversation on ReviewNB |
|
fixed. View entire conversation on ReviewNB |
|
done. I replaced by a C program. View entire conversation on ReviewNB |
|
fixed. View entire conversation on ReviewNB |
|
fixed, thanks. View entire conversation on ReviewNB |
|
I really don't like the cookie recipes example tbh (especially because we use it everywhere). I would prefer a practical use case (ie. using a PDF file). View entire conversation on ReviewNB |
|
fixed, thanks @markmcd. View entire conversation on ReviewNB |
|
done. View entire conversation on ReviewNB |
|
done. View entire conversation on ReviewNB |
|
it is way uglier now :) but done. View entire conversation on ReviewNB |
|
done. View entire conversation on ReviewNB |
|
done. View entire conversation on ReviewNB |
|
fixed. View entire conversation on ReviewNB |
|
done. View entire conversation on ReviewNB |
|
done. View entire conversation on ReviewNB |
|
done. View entire conversation on ReviewNB |
|
because I'm performing a cosine similarity to use as basis to distance estimation - df@df.T gives me a dot product without normalization which is not exactly what the sklearn cosine similarity does. View entire conversation on ReviewNB |
|
done. View entire conversation on ReviewNB |
|
fixed. View entire conversation on ReviewNB |
|
It is explained on the get started notebook and in the documentation. this one is about how to use Gemini models with the OpenAI SDK. View entire conversation on ReviewNB |
|
fixed on previous comments. thanks. View entire conversation on ReviewNB |
* adding OpenAI compatibility quickstart * fixing notebook formatting * addressing feedbacks * fixing lint issues bypassing & merging as it was previously reviewed. * fixing lint issues
Including one quickstart for the OpenAI compatibility.