Skip to content

Conversation

@MichaelDoyle
Copy link
Member

I think this will help folks that want to contribute to the plugins with better type safety. I've seen a few errors go by (or almost go by) related to this.

@MichaelDoyle MichaelDoyle requested a review from pavelgj June 12, 2024 17:42
Copy link
Member Author

@MichaelDoyle MichaelDoyle Jun 12, 2024

Choose a reason for hiding this comment

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

This seemed a little funky to me. Should we just make this ChromaRetrieverOptionsSchema?

It looks like we're possibly double-wrapping in an Optional? https://github.com/firebase/genkit/blob/main/js/ai/src/retriever.ts#L164-L168

Copy link
Contributor

Choose a reason for hiding this comment

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

When I first made these abstractions in js/ai, I wanted to make as few assumptions as possible. That meant that the options should not be ZodOptional since we do not know what kind of retriever they are building and what options are required vs optional.

IMO, the /ai abstractions should not add a .optional() on the schema and the providers can add them in the custom schema they define (ChromaRetrieverOptionsSchema in this case).

That being said, this code has through many changes and its evident that it no longer holds to that thought. The problem here does not seem to be double wrapping though -- there is no Optional on the ChromaRetrieverOptionsSchema object and it is added by RetrieverAction.

Copy link
Member Author

@MichaelDoyle MichaelDoyle Jun 12, 2024

Choose a reason for hiding this comment

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

This seemed a little funky to me. Should we just make this PineconeIndexerOptionsSchema?

It looks like we're possibly double-wrapping in an Optional? https://github.com/firebase/genkit/blob/main/js/ai/src/embedder.ts#L74-L78

@MichaelDoyle MichaelDoyle force-pushed the model-config-type-safety branch from fd6c532 to e916bc4 Compare June 12, 2024 17:46
@MichaelDoyle MichaelDoyle force-pushed the model-config-type-safety branch from e916bc4 to d449594 Compare June 12, 2024 20:40
@MichaelDoyle
Copy link
Member Author

@pavelgj friendly ping on this one

@MichaelDoyle MichaelDoyle deleted the model-config-type-safety branch February 25, 2025 02:27
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.

2 participants