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 Schema type to reference itself in its child properties #222

Merged
merged 3 commits into from
Aug 12, 2024

Conversation

hsubox76
Copy link
Collaborator

@hsubox76 hsubox76 commented Aug 6, 2024

Note: There are a lot of docs changes, feel free to collapse the docs folder in the PR page while reviewing code and then look at them separately later.

Schema was incorrectly referencing FunctionDeclarationSchema for the value of its items array and properties map. This happened when we genericized Schema and forgot to do the same for all its properties.

As part of this change I also renamed FunctionDeclarationSchemaType to SchemaType, as it is also used for JSON response schema, and may be used for even more use cases going forward.

I've also made some changes to fix some docs errors regarding GoogleAIFileManager and SingleRequestOptions that were causing them to be left blank in some docs when called with a {@link}.

@hsubox76 hsubox76 marked this pull request as ready for review August 6, 2024 23:51
dlarocque
dlarocque previously approved these changes Aug 7, 2024
@@ -75,7 +75,7 @@ export class ChatSession {
*
* Fields set in the optional {@link SingleRequestOptions} parameter will
* take precedence over the {@link RequestOptions} values provided at the
* time of the {@link GoogleAIFileManager} initialization.
* time of the `GoogleAIFileManager` initialization.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why didn't the @link work here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

api-extractor/documenter doesn't really handle multiple entry points and there's 2 separate entry points from the main package and from the /server subpath. ChatSession is under the main entry point and GoogleAIFileManager is in the subpath. We run api-documenter separately on each and the two processes don't know about each other. That's why we have a fork of api-documenter in the firebase-js-sdk repo, we wrote custom code to handle multiple entry points. I don't want to use the fork if possible because it's not getting any of the bug fixes and improvements being made to the main branch of api-documenter.

However, now that I've written all that, I realize this is in ChatSession, and this was probably originally a copy-paste mistake, as GoogleAIFileManager doesn't have anything to do with ChatSession. It looks like all the bad GoogleAIFileManager links were typos in the non-server-only entry point, which shouldn't have any references to GoogleAIFileManager at all. I've fixed them to refer to the correct initialization location (GoogleGenerativeAI.getGenerativeModel()).

/** Optional. The items of the property. {@link FunctionDeclarationSchema} */
items?: FunctionDeclarationSchema;
/** Optional. The items of the property. */
items?: Schema;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems a little odd to me. So a Schema can have Schemas?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. It's always been that way in the proto: https://source.corp.google.com/piper///depot/google3/google/ai/generativelanguage/v1main/content.proto;l=489

We moved away from using the generic Schema type in the SDK for function calling because top-level Schemas had different requirements from Schemas nested in properties or items so we wanted to distinguish and provide more specific guidance for users. It's less clear what the difference is for the top-level responseSchema is so we should leave it as the generic Schema until we have any confirmation that it should be different.

As long as there is a generic Schema, its children should also be generic Schema and not extended versions of itself that other extended versions may not know about.

@dlarocque dlarocque self-requested a review August 9, 2024 13:51
Copy link
Collaborator

@dlarocque dlarocque left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops- I meant to approve the first time

@hsubox76 hsubox76 merged commit 3b5daae into main Aug 12, 2024
8 checks passed
@hsubox76 hsubox76 deleted the ch-schema-fix branch August 12, 2024 20:34
@newme616
Copy link

The renaming is not reflected in any documentation! See: https://ai.google.dev/gemini-api/docs/json-mode?lang=node

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