-
Notifications
You must be signed in to change notification settings - Fork 399
adding docstring documentation to backend module #1353
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Colin O'Sullivan <[email protected]>
Summary of ChangesHello @colinosullivan-ie, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances the documentation within the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request does an excellent job of adding comprehensive docstrings to the backend module, significantly improving the documentation. The new docstrings are clear, follow a consistent style, and include helpful examples.
I've identified a couple of areas for improvement:
- The examples for the
clonemethods inBackend,ChatModel, andEmbeddingModelcan be misleading. They imply a deep copy behavior that is not guaranteed by the base implementation, which could lead to user confusion. - The placeholder error messages in
NotImplementedErrorexceptions for abstract methods could be made more descriptive to improve the developer experience when subclassing.
These are minor points in an otherwise great contribution to the project's documentation.
| Example: | ||
| >>> original = Backend.from_provider("openai") | ||
| >>> cloned = await original.clone() | ||
| >>> # Modifications to cloned won't affect original |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The example for clone assumes that the underlying chat and embedding models implement a deep copy clone method. As noted in the ChatModel and EmbeddingModel base classes, their default clone implementation performs a shallow copy (returns self). If the specific models (e.g., from the "openai" provider) do not override this, the example is misleading as cloned will not be a deep copy. This could cause unexpected side effects for users. The documentation should be clear about this dependency or use a more robust example that doesn't rely on a specific provider's implementation.
| Example: | ||
| >>> original = ChatModel.from_name("openai:gpt-4") | ||
| >>> cloned = await original.clone() | ||
| >>> # Modifications to cloned won't affect original | ||
| >>> cloned.config(parameters=ChatModelParameters(temperature=0.9)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The example provided for the clone method can be misleading. It uses ChatModel.from_name("openai:gpt-4"), which returns an OpenAIChatModel instance. However, the base ChatModel.clone method returns self (a shallow copy) and logs a warning if not overridden. If OpenAIChatModel or its parents do not override clone to perform a deep copy, the example is incorrect because modifications to cloned will affect original. This could lead to subtle bugs for users who copy this example.
To avoid confusion, the example should either use a mock class that is known to implement clone correctly, or be rephrased to not rely on a specific implementation that may not have this feature.
| Example: | ||
| >>> original = EmbeddingModel.from_name("openai:text-embedding-3-small") | ||
| >>> cloned = await original.clone() | ||
| >>> # Modifications to cloned won't affect original |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the ChatModel.clone method, the example for EmbeddingModel.clone can be misleading. It uses EmbeddingModel.from_name("openai:text-embedding-3-small") and implies a deep copy is made. However, the base implementation returns self and warns if not overridden. If OpenAIEmbeddingModel does not implement a deep clone, this example is incorrect and could cause bugs for users.
Consider using a mock class in the example to demonstrate the intended behavior of a correctly implemented clone method, to avoid confusion.
| >>> print(len(chunks)) | ||
| >>> print(chunks[0]) # First chunk | ||
| """ | ||
| raise NotImplementedError("Implement me") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Raises: | ||
| NotImplementedError: If the subclass doesn't implement this method. | ||
| """ | ||
| raise NotImplementedError("Implement me") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| >>> ids = await vector_store.add_documents(documents) | ||
| >>> print(ids) # ['id1', 'id2'] | ||
| """ | ||
| raise NotImplementedError("Implement me") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ... filter={"source": "documentation"} | ||
| ... ) | ||
| """ | ||
| raise NotImplementedError("Implement me") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Raises: | ||
| NotImplementedError: If the subclass doesn't implement this method. | ||
| """ | ||
| raise NotImplementedError("Implement me") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Attributes: | ||
| tool_call_fallback_via_response_format: Enable fallback to response format for tool calls. | ||
| retry_on_empty_response: Automatically retry when the model returns an empty response. | ||
| model_supports_tool_calling: Whether the underlying model supports native tool calling. | ||
| allow_parallel_tool_calls: Allow the model to make multiple tool calls simultaneously. | ||
| ignore_parallel_tool_calls: Ignore all but the first tool call when multiple are returned. | ||
| use_strict_tool_schema: Use strict JSON schema validation for tool parameters. | ||
| use_strict_model_schema: Use strict JSON schema validation for structured outputs. | ||
| supports_top_level_unions: Whether the model supports union types at the top level. | ||
| parameters: Default parameters for model generation (temperature, max_tokens, etc.). | ||
| cache: Cache implementation for storing and retrieving model outputs. | ||
| settings: Additional provider-specific settings. | ||
| middlewares: List of middleware to apply during model execution. | ||
| tool_choice_support: Set of supported tool choice modes (required, none, single, auto). | ||
| fix_invalid_tool_calls: Automatically attempt to fix malformed tool calls. | ||
| """ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you do comments on each attribute instead? Like it is done in AgentOptions.
Tomas2D
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution. Just please update the way comments are done for Pydantic Models / Kwargs. Prefer an inline description instead of the top-level one, as it improves readability.
Which issue(s) does this pull-request address?
#1228
Closes: #
Description
Adding docstring documentation to the Backend module
Checklist
General
/ TypeScript
Pythonfor Python changes,TypeScriptfor TypeScript changesCode quality checks
mise check(mise fixto auto-fix)Testing
mise test:unitmise test:e2eDocumentation
mise docs:fix