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

Update chain docs #962

Closed
wants to merge 3 commits into from
Closed

Update chain docs #962

wants to merge 3 commits into from

Conversation

vshulman
Copy link
Contributor

@vshulman vshulman commented Jun 6, 2024

Clarifying LLM section of chains guide

@vshulman vshulman requested a review from marius-baseten June 6, 2024 22:51
It still works at runtime, because we constructed `FakeMistralLLM` to
have the same *procotol*. To also get this "clean" from a typing perspective,
we can actually use a `Protocol` in the type annotation:
The argument mistral_llm is of type Mistral LLM that can be substituted
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks a lot for contributing to making the docs better!
Overall this is all fine, just one discussion point for this section here:

I assume your intent for the change here was to make it less "technical bulky" and more accessible. But in this case it waters down the message that it really is a type error - unless you add the protocol annotation.

If someone has type checking set-up in their CI, they might be surprised by getting a type error (or think that our example code is messy). So I think writing "can be substituted" is a bit to lenient. And please keep the ` backticks around identifiers from code examples.

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.

3 participants