Add unified provider interface with string-based initialization #1490
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This commit adds a new string-based provider initialization interface that makes it easier to work with different LLM providers using a consistent API. - Add auto_client.py with from_provider function - Update __init__.py to expose from_provider - Add tests for the new interface - Update documentation including README and integrations docs - Add blog post explaining the feature
Deploying instructor-py with
|
| Latest commit: |
d7d0d7a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://c0300b30.instructor-py.pages.dev |
| Branch Preview URL: | https://automodel.instructor-py.pages.dev |
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to ba0294b in 1 minute and 42 seconds
More details
- Looked at
1188lines of code in16files - Skipped
0files when reviewing. - Skipped posting
8drafted comments based on config settings.
1. tests/test_auto_client.py:84
- Draft comment:
Bug: In test_async_parameter, the assertion usesexpected_type.__class__which always evaluates totype. Instead, useisinstance(mock_instructor, expected_type)to properly check the type of the returned instructor instance. - Reason this comment was not posted:
Marked as duplicate.
2. instructor/client_fireworks.py:46
- Draft comment:
Review the async_wrapper implementation. In the branch where 'stream' is True, the function returns the coroutine without awaiting, whereas in the else branch it awaits the result. Confirm that this behavior is intentional and correctly handles streaming responses. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
3. docs/blog/index.md:30
- Draft comment:
There appears to be a typographical inconsistency in the 'Fine-tuning and Distillation in AI Models' link. The display text uses “Distillation” while the linked file is named ‘posts/distilation-part1.md’. Consider updating the file name or the link text to use the correct spelling 'distillation'. - Reason this comment was not posted:
Comment was on unchanged code.
4. docs/blog/posts/string-based-init.md:33
- Draft comment:
In the usage example (line 33) you use 'google/gemini-pro', but in the Supported Providers section (line 50) the naming is given as 'google/model-name'. Please clarify which format should be used to maintain consistency. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 85%
The inconsistency is real but not problematic. The "model-name" format in the providers section is clearly a placeholder showing the general format, while the example shows an actual implementation. This is a common documentation pattern where you show the generic format in one section and concrete examples in another. The meaning is clear to readers.
The comment raises a valid point about consistency in documentation. Inconsistent formatting could potentially confuse readers.
The apparent inconsistency is actually helpful - it shows both the generic format and a concrete example, which is good documentation practice.
This comment should be deleted as it points out a non-issue. The documentation is clear and follows standard practices of showing both generic formats and specific examples.
5. docs/blog/posts/string-based-init.md:34
- Draft comment:
In the usage example (line 34) you use 'mistral/mistral-large', whereas the Supported Providers section (line 51) specifies 'mistral/model-name'. Please update one of these to ensure consistency in the documentation. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 85%
The comment identifies a real inconsistency in the documentation. In the examples section, a specific model name is used (mistral-large) while in the Supported Providers section, a generic placeholder (model-name) is used. However, this is actually the expected pattern - examples should show real model names while the provider list shows the generic format. This is consistent with how other providers are documented in the file.
Maybe the inconsistency could be confusing to readers who are new to LLM APIs and don't understand the difference between specific model names and format placeholders?
The pattern of showing specific examples followed by generic formats is a common documentation practice and is used consistently throughout the file for all providers. The format is also explicitly explained in the text.
The comment should be deleted as it flags a non-issue. The documentation correctly uses specific model names in examples and generic placeholders in the format description.
6. instructor/client_gemini.py:42
- Draft comment:
There is a typographical inconsistency in the error message for the isinstance check. The error message at line 43 reads 'genai.generativemodel', but the actual class name is 'genai.GenerativeModel'. It is recommended to correct the casing for consistency. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
7. instructor/reask.py:27
- Draft comment:
Typographical issue: please change "Response must be a Anthropic Message" to "Response must be an Anthropic Message". - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
8. instructor/reask.py:74
- Draft comment:
Typographical issue: please update "Response must be a Anthropic Message" to "Response must be an Anthropic Message". - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_nZCBo2jaDwop5lBz
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
- Add auto_client.py with from_provider function to initialize any provider - Add comprehensive error handling with installation instructions - Update blog post with improved examples and troubleshooting - Add automodel example that demonstrates multi-provider usage - Bump version to 1.8.0 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed 58c0a8d in 1 minute and 34 seconds. Click for details.
- Reviewed
689lines of code in5files - Skipped
0files when reviewing. - Skipped posting
8draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. instructor/auto_client.py:76
- Draft comment:
Default mode assignment has been removed. If default modes are meant to be applied automatically, ensure that this behavior is clearly documented. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. instructor/auto_client.py:90
- Draft comment:
Similar try/except blocks for each provider lead to repetitive code. Consider refactoring to reduce duplication. - Reason this comment was not posted:
Confidence changes required:80%<= threshold85%None
3. docs/blog/posts/string-based-init.md:45
- Draft comment:
Ensure the provider model examples match actual supported models. For instance, verify examples like 'openai/gpt-4o' and others are correct. - Reason this comment was not posted:
Confidence changes required:80%<= threshold85%None
4. docs/blog/posts/string-based-init.md:163
- Draft comment:
Validate that the error code snippet in the troubleshooting section accurately reflects real 'model not found' responses from providers. - Reason this comment was not posted:
Confidence changes required:70%<= threshold85%None
5. instructor/auto_client.py:36
- Draft comment:
The 'mode' parameter is defined but never used. Consider removing it from the signature to avoid confusion. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
6. instructor/auto_client.py:160
- Draft comment:
Format the supported providers list as a comma‐separated string in the error message for better readability. - Reason this comment was not posted:
Confidence changes required:70%<= threshold85%None
7. examples/automodel/run.py:97
- Draft comment:
The Mistral client example uses 'mistral/mistral-small' while the docs list 'mistral/mistral-small-latest'. Align the example with documentation. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 70% vs. threshold = 85% Since this is a new example file being added to demonstrate library usage, consistency with documentation is important. However, I don't have access to the actual documentation to verify if there really is a discrepancy. The comment could be wrong about what's in the docs. Additionally, both model names might work, making this a non-critical issue. I don't have access to the documentation to verify if this discrepancy actually exists. The comment could be incorrect about what's in the docs. While I can't verify the docs, consistency in examples is important enough that this potential discrepancy should be investigated. Keep the comment since consistency between examples and documentation is important for library code, but with reduced confidence due to inability to verify the docs.
8. uv.lock:529
- Draft comment:
Review the updated platform markers (e.g., using 'sys_platform == "win32"') to ensure cross-platform compatibility. - Reason this comment was not posted:
Confidence changes required:60%<= threshold85%None
Workflow ID: wflow_GSxsne01QaUf7lzz
You can customize
10000
instructor/auto_client.py
Outdated
| def from_provider( | ||
| model: str, | ||
| async_client: bool = False, | ||
| mode: instructor.Mode | None = None, # noqa: ARG001 |
There was a problem hiding this comment.
The 'mode' parameter is accepted but no longer passed to provider-specific functions. Consider either removing it or documenting its intended use.
- Fix function signature for from_gemini, from_mistral, and from_genai - Add proper type handling for optional dependencies - Change reportMissingImports to warning in pyright config 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 881132a in 1 minute and 28 seconds. Click for details.
- Reviewed
70lines of code in2files - Skipped
0files when reviewing. - Skipped posting
12draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. instructor/auto_client.py:111
- Draft comment:
Using 'model_name' instead of 'model' when instantiating genai.GenerativeModel is a good update. Confirm that the upstream API expects 'model_name'. - Reason this comment was not posted:
Confidence changes required:80%<= threshold85%None
2. instructor/auto_client.py:115
- Draft comment:
Async client handling is duplicated (if/else branches) across multiple providers. Consider extracting a helper to reduce repetition. - Reason this comment was not posted:
Confidence changes required:70%<= threshold85%None
3. instructor/auto_client.py:128
- Draft comment:
The mistral branch now conditionally calls from_mistral with use_async flag. Consider unifying the async condition check to reduce code duplication. - Reason this comment was not posted:
Confidence changes required:70%<= threshold85%None
4. instructor/auto_client.py:246
- Draft comment:
For vertexai provider, 'GenerativeModel' now uses 'model_name'. Ensure that this aligns with the updated API from the vertexai.generative_models module. - Reason this comment was not posted:
Confidence changes required:80%<= threshold85%None
5. instructor/auto_client.py:260
- Draft comment:
In the 'genai' branch, async_client handling is repeated. Consolidating the if/else logic (possibly via a small wrapper) could improve maintainability. - Reason this comment was not posted:
Confidence changes required:70%<= threshold85%None
6. pyproject.toml:43
- Draft comment:
Changing reportMissingImports from 'error' to 'warning' can suppress important type issues. Confirm this change is intentional. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 85% This is a configuration change that reflects a deliberate choice in how strictly to enforce import checking. While making type checking more strict can be good, this is clearly an intentional choice to make it less strict. The comment violates the rule about not asking for confirmation of intentional changes. Configuration changes like this are typically well-considered. The comment could be valid if this change would hide serious type issues that could lead to runtime errors. Missing imports could indicate real problems. While type safety is important, this is clearly a deliberate configuration choice. The comment explicitly asks to "confirm this change is intentional" which violates our rules about not asking for confirmation. Delete the comment. It violates the rule about asking for confirmation of intentional changes, and configuration changes like this are typically deliberate choices about development workflow.
7. instructor/auto_client.py:111
- Draft comment:
Updated google provider instantiation: using 'model_name' instead of 'model' aligns with the provider's API. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold85%This comment is purely informative, explaining a change in the code without suggesting any action or asking for clarification. It doesn't align with the rules as it doesn't provide a specific suggestion or request for confirmation.
8. instructor/auto_client.py:115
- Draft comment:
Async handling for the Google provider now conditionally passes 'use_async' only when needed, avoiding extra parameters for sync clients. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold85%This comment is purely informative and does not provide any actionable feedback or suggestions for improvement. It does not align with the rules for useful comments, as it does not address any specific issues or improvements needed in the code.
9. instructor/auto_client.py:131
- Draft comment:
For the Mistral provider, the async branch now calls from_mistral with the use_async flag only if async_client is True, which prevents passing an unsupported parameter in sync mode. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold85%This comment is purely informative, explaining a change in the code logic without suggesting any action or asking for clarification. It doesn't align with the rules for useful comments, as it doesn't provide a suggestion, ask for confirmation, or point out a potential issue.
10. instructor/auto_client.py:243
- Draft comment:
For the VertexAI provider, the parameter name is updated to 'model_name', ensuring correct API usage. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold85%This comment is purely informative and does not provide any actionable feedback or suggestions. It simply states a change that was made without offering any guidance or asking for confirmation. According to the rules, purely informative comments should be removed.
11. instructor/auto_client.py:261
- Draft comment:
In the Google GenAI provider block, async handling is made consistent: use_async is passed only when async_client is True. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold85%This comment is purely informative and does not provide any actionable feedback or suggestions for improvement. It simply states that something is good, which violates the rule against making purely informative comments.
12. pyproject.toml:40
- Draft comment:
Changed 'reportMissingImports' from 'error' to 'warning'. Ensure this less strict setting is intentional for your type checking strategy. - Reason this comment was not posted:
Comment looked like it was already resolved.
Workflow ID: wflow_88OgEgwZiCOa2g6x
You can customize
- Fix remaining type errors related to missing imports 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 3381be3 in 1 minute and 8 seconds. Click for details.
- Reviewed
17lines of code in1files - Skipped
0files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. instructor/auto_client.py:128
- Draft comment:
Consider adding a brief comment explaining why type checking is ignored for the Mistral provider import to aid future maintainers. - Reason this comment was not posted:
Confidence changes required:70%<= threshold85%None
2. instructor/auto_client.py:131
- Draft comment:
Include a comment clarifying why type checking is bypassed on client instantiation for Mistral, for clarity in case of future type updates. - Reason this comment was not posted:
Confidence changes required:70%<= threshold85%None
3. instructor/auto_client.py:127
- Draft comment:
Consider specifying a more targeted ignore code (e.g.,# type: ignore[import]) for the mistralai import. This can help avoid inadvertently ignoring other type issues. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 80% vs. threshold = 85% The comment is about a real code change in the diff. It's a reasonable suggestion to be more specific with type ignore annotations as it helps maintain better type safety. The suggestion is clear and actionable. However, we need to be sure this is actually the correct type ignore code for this case. We don't have visibility into the actual type error being suppressed. Maybe [import] isn't the correct ignore code for this situation. While we don't have full context, being more specific with type ignores is generally good practice and the suggestion follows Python typing best practices. Keep the comment as it suggests a concrete improvement to type safety while following Python best practices.
4. instructor/auto_client.py:131
- Draft comment:
Likewise, consider a more specific ignore (e.g.,# type: ignore[call-overload]) on the instantiation of the client if possible, to limit the ignore scope. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 70% vs. threshold = 85% Being more specific with type ignores is generally good practice as it makes the intention clearer and limits the scope of what type checks are being ignored. However, without seeing the actual type error being suppressed or the mistralai package types, I can't be 100% certain that call-overload is the specific error. The comment seems reasonable but requires assumptions about the underlying type issue. I don't have visibility into the actual type error being suppressed, so I can't verify that call-overload is the correct specific ignore to use. The suggestion could be wrong if the type error is something else. While I can't be 100% certain about the specific error, suggesting more specific type ignores is a good practice that improves code quality and maintainability by making the intention clearer. Keep the comment as it promotes better typing practices by suggesting a more specific type ignore, even though we can't be completely certain about the exact type error.
Workflow ID: wflow_JRVLqU8g4fa3Rvko
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed a81ebcd in 30 seconds. Click for details.
- Reviewed
38lines of code in1files - Skipped
0files when reviewing. - Skipped posting
6draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. tests/dsl/test_simple_type_fix.py:3
- Draft comment:
Good removal of unused import for List; using modern type hinting with list is clear. - Reason this comment was not posted:
Confidence changes required:0%<= threshold85%None
2. tests/dsl/test_simple_type_fix.py:19
- Draft comment:
Docstring updated to match new syntax. Confirm that all documentation aligns with these changes. - Reason this comment was not posted:
Confidence changes required:0%<= threshold85%None
3. tests/dsl/test_simple_type_fix.py:13
- Draft comment:
Assertion formatting looks good; clear error messages with Python version included. - Reason this comment was not posted:
Confidence changes required:0%<= threshold85%None
4. tests/dsl/test_simple_type_fix.py:3
- Draft comment:
Good removal of the unused 'List' import. Using the built-in generic type (list) improves consistency with Python 3.10+. - Reason this comment was not posted:
Confidence changes required:0%<= threshold85%None
5. tests/dsl/test_simple_type_fix.py:19
- Draft comment:
Updated the docstring in the alternative syntax test to correctly reflect the use of the built-in 'list' generic alias instead of 'List'. This enhances consistency. - Reason this comment was not posted:
Confidence changes required:0%<= threshold85%None
6. tests/dsl/test_simple_type_fix.py:13
- Draft comment:
Assertion messages are clear and provide Python version context. Good use of multi-line formatting for clarity. - Reason this comment was not posted:
Confidence changes required:0%<= threshold85%None
Workflow ID: wflow_7GL3ZhaBUWrZ0h5J
You can customize
- Improve is_simple_type to better handle list[int | str] and list[Union[int, str]] - Fix auto_client test mocks to correctly patch from_openai - Update missing_dependency test to properly trigger ImportError 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 1bdb493 in 1 minute and 15 seconds. Click for details.
- Reviewed
100lines of code in2files - Skipped
0files when reviewing. - Skipped posting
10draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. tests/test_auto_client.py:83
- Draft comment:
In the async parameter test, the instance check usingexpected_type.__class__is incorrect. It likely should verify that the returned result is an instance ofexpected_type(i.e. useisinstance(result, expected_type)) rather than checking the mock instructor’s class. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. tests/test_auto_client.py:124
- Draft comment:
Patching 'instructor.auto_client.importlib' may be too broad and may not effectively simulate the missing dependency scenario. Consider patching 'importlib.util.find_spec' to more accurately mimic the behavior when the required package is missing. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 85% The test is already effectively testing the missing dependency case by directly forcing an ImportError through the openai patch. The current implementation actually seems more direct and clearer than the suggested approach. The suggestion to use find_spec would test the dependency check at a lower level, but that's not necessarily better. The comment raises a valid point about being more specific with patches. However, the current implementation might actually be more maintainable and clearer in its intent. The current implementation directly tests the outcome we care about (ImportError for missing dependency) in a clear and maintainable way. The suggested change would make the test more complex without clear benefits. Delete the comment. The current implementation is effective and clear, and the suggested change doesn't provide significant benefits.
3. instructor/dsl/simple_type.py:68
- Draft comment:
Good update! The addedlistin the origins set and union-check with__or__improves compatibility with Python 3.10 union types. Consider adding a note on the rationale for differentiating between streaming types (Iterable) and regular lists. - Reason this comment was not posted:
Confidence changes required:0%<= threshold85%None
4. tests/test_auto_client.py:21
- Draft comment:
Ensure patching the correct location now that the function is accessed via 'instructor.from_openai'. Confirm that the new patch path reflects the actual import path in auto_client. - Reason this comment was not posted:
Confidence changes required:0%<= threshold85%None
5. tests/test_auto_client.py:63
- Draft comment:
Same as above: verify that patching 'instructor.auto_client.instructor.from_openai' is correct throughout the tests. - Reason this comment was not posted:
Confidence changes required:0%<= threshold85%None
6. tests/test_auto_client.py:89
- Draft comment:
Ensure the updated patch path 'instructor.auto_client.instructor.from_openai' is intended here for mode_parameter tests. - Reason this comment was not posted:
Confidence changes required:0%<= threshold85%None
7. tests/test_auto_client.py:100
- Draft comment:
Verify that the default mode patching is correct with the new import path. - Reason this comment was not posted:
Confidence changes required:0%<= threshold85%None
8. tests/test_auto_client.py:112
- Draft comment:
Check that additional kwargs are properly forwarded with the new patch path. Looks consistent. - Reason this comment was not posted:
Confidence changes required:0%<= threshold85%None
9. tests/test_auto_client.py:124
- Draft comment:
Patching 'instructor.auto_client.importlib' with return_value=None might be too broad. Consider patching 'instructor.auto_client.importlib.util.find_spec' directly to simulate missing dependencies without affecting the entire module. - Reason this comment was not posted:
Confidence changes required:80%<= threshold85%None
10. tests/test_auto_client.py:84
- Draft comment:
The type-check assertion usingisinstance(mock_instructor, expected_type.__class__)is not validating correctness (since expected_type.class is always 'type'). Likely intended to checkisinstance(result, expected_type), which would properly verify the returned client instance type. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_hGsV6wb3rjzRyRw4
You can customize
- Skip pipe type operator tests on Python <3.10 - Improve handling of Union types in simple_type - Simplify auto_client tests to focus on core functionality 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 5b0308c in 1 minute and 16 seconds. Click for details.
- Reviewed
258lines of code in4files - Skipped
0files when reviewing. - Skipped posting
9draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. instructor/dsl/simple_type.py:70
- Draft comment:
Returning True immediately if origin is list may bypass additional union argument checks. Confirm this is the intended behavior for both plain lists and lists with union types. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. instructor/dsl/simple_type.py:78
- Draft comment:
Consider adding a comment that differentiates between detection of a union type vs. Python 3.10+ pipe syntax on inner_arg. - Reason this comment was not posted:
Confidence changes required:70%<= threshold85%None
3. tests/dsl/test_simple_type_fix.py:12
- Draft comment:
Good usage of sys.version_info check to ensure compatibility. Ensure tests cover both pipe syntax and Union syntax in environments where supported. - Reason this comment was not posted:
Confidence changes required:0%<= threshold85%None
4. tests/test_auto_client.py:18
- Draft comment:
The test_provider_parsing verifies only string splitting. Consider extending tests to validate the full parsing logic of from_provider if possible. - Reason this comment was not posted:
Confidence changes required:80%<= threshold85%None
5. tests/test_fizzbuzz_fix.py:12
- Draft comment:
Using Union syntax here for compatibility is clear. Good message in assert to indicate the expected type format. - Reason this comment was not posted:
Confidence changes required:0%<= threshold85%None
6. instructor/dsl/simple_type.py:68
- Draft comment:
Refactored union type check for list types improves clarity by avoiding duplicate checks. Consider adding a comment to explain checking for the or attribute for Python 3.10+. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 85% The code already has a clear comment explaining what the or check is for. The existing comment is concise but informative. The suggestion to add more documentation seems redundant since the code is already well-documented. The comment doesn't point out any actual missing information that would be valuable to add. Perhaps there could be more detailed explanation about why this check is necessary or what specific Python versions it applies to. The existing comment is sufficient for developers who need to maintain this code - it clearly indicates the purpose (Python 3.10+ pipe syntax) and the mechanism (checking or attribute). More detail would likely be unnecessary verbosity. The comment should be deleted since the code already has appropriate documentation about the or attribute check.
7. tests/dsl/test_simple_type_fix.py:11
- Draft comment:
Using skipTest for Python versions without pipe syntax is acceptable. Consider also testing alternative syntax in environments that support it. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =80%<= threshold85%The comment suggests testing alternative syntax in environments that support it, which is a specific suggestion related to testing. It doesn't ask the author to ensure or verify anything, but rather provides a constructive suggestion for improvement.
8. tests/test_auto_client.py:18
- Draft comment:
The test now focuses on parsing the provider string rather than full provider instantiation. Ensure that integration tests exist elsewhere for provider-specific behaviors. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold85%The comment is asking the PR author to ensure that integration tests exist elsewhere, which violates the rule against asking the author to ensure testing. It doesn't provide a specific suggestion or point out a specific issue with the code.
9. tests/test_fizzbuzz_fix.py:12
- Draft comment:
The test now uses Union syntax for compatibility instead of pipe syntax. If Union types are supported in earlier Python versions, consider removing the skip to expand coverage. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =80%<= threshold85%The comment is suggesting a change to the test to expand coverage if certain conditions are met. It is not asking the author to confirm their intention or to ensure behavior, but rather suggesting a specific action to improve the test coverage. This aligns with the rules for making code suggestions.
Workflow ID: wflow_ZkbXpuDLc0UpRfWb
You can customize
- Add special case handling for list[Union[int, str]] in Python 3.9 - Update tests to use version-specific syntax instead of skipping - Fix is_simple_type to better handle Union types across Python versions 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed d7d0d7a in 35 seconds. Click for details.
- Reviewed
180lines of code in3files - Skipped
0files when reviewing. - Skipped posting
8draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. instructor/dsl/simple_type.py:56
- Draft comment:
The Python 3.9 special case using string checks (e.g., 'list[typing.Union[') is brittle. Consider using explicit type introspection instead of string matching. - Reason this comment was not posted:
Confidence changes required:70%<= threshold85%None
2. instructor/dsl/simple_type.py:90
- Draft comment:
Union type detection logic is duplicated and complex. Refactor or extract helper to improve readability and maintainability. - Reason this comment was not posted:
Confidence changes required:80%<= threshold85%None
3. instructor/dsl/simple_type.py:53
- Draft comment:
The special case for Python 3.9 relies on string-matching the type representation (e.g. 'list[typing.Union['). This workaround could be brittle. Consider adding a note on potential limitations or exploring a more robust detection method. - Reason this comment was not posted:
Confidence changes required:80%<= threshold85%None
4. instructor/dsl/simple_type.py:90
- Draft comment:
The union type detection conditions are repeated (checking inner_origin against typing.Union in multiple ways). Consider refactoring this logic into a helper function to improve clarity and maintainability. - Reason this comment was not posted:
Confidence changes required:80%<= threshold85%None
5. instructor/dsl/simple_type.py:100
- Draft comment:
The check for pipe syntax with 'hasattr(inner_arg, "or")' is clever. Please add an inline comment explaining why this check reliably detects Python 3.10+ pipe syntax, for future maintainers. - Reason this comment was not posted:
Confidence changes required:70%<= threshold85%None
6. instructor/dsl/simple_type.py:54
- Draft comment:
The 'sys' module is imported inside the function. For consistency and possibly slight performance improvements, consider moving 'import sys' to the top level of the module. - Reason this comment was not posted:
Confidence changes required:60%<= threshold85%None
7. tests/dsl/test_simple_type_fix.py:12
- Draft comment:
Good use of conditional logic to test both pipe and Union syntax based on the Python version. This ensures compatibility across versions. - Reason this comment was not posted:
Confidence changes required:20%<= threshold85%None
8. tests/test_fizzbuzz_fix.py:12
- Draft comment:
The test handles Python version differences effectively by conditionally using pipe or Union syntax. The assertions are comprehensive and clear. - Reason this comment was not posted:
Confidence changes required:20%<= threshold85%None
Workflow ID: wflow_0oA7xHoAtjSHX8Z5
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed ab1c61e in 58 seconds. Click for details.
- Reviewed
18lines of code in1files - Skipped
0files when reviewing. - Skipped posting
5draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. tests/test_fizzbuzz_fix.py:1
- Draft comment:
Good removal of unused import 'Union'. - Reason this comment was not posted:
Confidence changes required:0%<= threshold85%None
2. tests/test_fizzbuzz_fix.py:27
- Draft comment:
Ensure file ends with a newline for POSIX compatibility. - Reason this comment was not posted:
Confidence changes required:33%<= threshold85%None
3. t
F438
ests/test_fizzbuzz_fix.py:3
- Draft comment:
Good removal of the unused import 'Union'. This cleans up the code without affecting functionality. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold85%This comment is purely informative, as it only praises the removal of an unused import without providing any actionable feedback or suggestions. According to the rules, purely informative comments should be removed.
4. tests/test_fizzbuzz_fix.py:27
- Draft comment:
Ensure the file ends with a newline for consistency and POSIX compatibility. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
5. tests/test_fizzbuzz_fix.py:27
- Draft comment:
There is no newline at the end of the file. It's a minor formatting issue but adding a newline at the end of the file is considered good practice. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_6csNctF8Q8VwYObt
You can customize
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed 9b71261 in 3 minutes and 45 seconds. Click for details.
- Reviewed
285lines of code in2files - Skipped
0files when reviewing. - Skipped posting
21draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. instructor/auto_client.py:12
- Draft comment:
Change provider name from 'genai' to 'generative-ai' is inconsistent with some older docs. Ensure documentation updates match this change. - Reason this comment was not posted:
Confidence changes required:80%<= threshold85%None
2. instructor/auto_client.py:87
- Draft comment:
In the OpenAI branch, passing 'mode' with a fallback to 'instructor.Mode.TOOLS' is good; ensure that new 'Mode' enum values are documented for users. - Reason this comment was not posted:
Confidence changes required:30%<= threshold85%None
3. instructor/auto_client.py:105
- Draft comment:
Anthropic branch now conditionally sets max_tokens. Consider caching kwargs.get('max_tokens') to DRY the logic. - Reason this comment was not posted:
Confidence changes required:50%<= threshold85%None
4. instructor/auto_client.py:125
- Draft comment:
Google provider branch now imports from 'google.genai'. Confirm that client initialization with 'vertexai' kwarg works as expected. - Reason this comment was not posted:
Confidence changes required:30%<= threshold85%None
5. instructor/auto_client.py:151
- Draft comment:
In the Mistral branch, reading MISTRAL_API_KEY directly from os.environ is repeated. Consider caching the API key for readability. - Reason this comment was not posted:
Confidence changes required:40%<= threshold85%None
6. instructor/auto_client.py:190
- Draft comment:
For the Perplexity branch, consider storing the result of kwargs.get('api_key') in a variable to avoid duplicate lookups. - Reason this comment was not posted:
Confidence changes required:40%<= threshold85%None
7. instructor/auto_client.py:301
- Draft comment:
The 'generative-ai' branch imports from 'google.generativeai'. Verify that error messages reflect the correct package name consistently. - Reason this comment was not posted:
Confidence changes required:70%<= threshold85%None
8. tests/test_auto_client.py:20
- Draft comment:
Test providers list includes many providers; ensure that tests remain maintainable/DRY as more providers are added. Possibly factor out common test setup. - Reason this comment was not posted:
Confidence changes required:30%<= threshold85%None
9. tests/test_auto_client.py:36
- Draft comment:
Tests use try/except with pytest.skip; consider using proper mocking for more robust testing of provider initialization in the future. - Reason this comment was not posted:
Confidence changes required:50%<= threshold85%None
10. instructor/auto_client.py:12
- Draft comment:
Renamed provider key from 'genai' to 'generative-ai'. Ensure that all documentation and client references are updated to use the new key. - Reason this comment was not posted:
Comment looked like it was already resolved.
11. instructor/auto_client.py:87
- Draft comment:
Passing a default mode (instructor.Mode.TOOLS) for the OpenAI provider improves consistency. Confirm that this default is appropriate and documented. - Reason this comment was not posted:
Confidence changes required:80%<= threshold85%None
12. instructor/auto_client.py:108
- Draft comment:
For the Anthropic provider, a default mode and a max_tokens value (4096) are now added. Consider documenting the rationale for using 4096 as the default max_tokens. - Reason this comment was not posted:
Confidence changes required:80%<= threshold85%None
13. instructor/auto_client.py:125
- Draft comment:
The Google provider now imports from 'google.genai' and instantiates a Client with a 'vertexai' parameter. Verify that switching from from_gemini to from_genai aligns with the intended client API and update related docs if needed. - Reason this comment was not posted:
Confidence changes required:70%<= threshold85%None
14. instructor/auto_client.py:186
- Draft comment:
Improved initialization for the Perplexity provider by checking PERPLEXITY_API_KEY from environment or kwargs. Consider refactoring this API key retrieval into a helper if similar logic recurs. - Reason this comment was not posted:
Confidence changes required:80%<= threshold85%None
15. instructor/auto_client.py:222
- Draft comment:
For the Groq, Writer, Cerebras, and Fireworks providers, the model parameter is now consistently passed. Confirm that all underlying conversion functions expect and properly handle the model parameter. - Reason this comment was not posted:
Confidence changes required:70%<= threshold85%None
16. instructor/auto_client.py:302
- Draft comment:
The 'generative-ai' branch now uses an import from 'google.generativeai' and calls from_gemini. The mix of from_genai (in the Google branch) and from_gemini here might lead to confusion—verify the naming consistency and behavior. - Reason this comment was not posted:
Confidence changes required:80%<= threshold85%None
17. tests/test_auto_client.py:20
- Draft comment:
The list of provider strings in tests now reflects the updated naming (e.g., 'generative-ai'). Ensure these changes are consistent with the provider keys in auto_client. - Reason this comment was not posted:
Comment looked like it was already resolved.
18. tests/test_auto_client.py:39
- Draft comment:
Using try/except to skip tests when a provider isn’t available is practical for integration tests. Consider adding a comment explaining this approach for clarity. - Reason this comment was not posted:
Confidence changes required:70%<= threshold85%None
19. tests/test_auto_client.py:16
- Draft comment:
Typo: In the prompt, 'Ivan is 28 and strays in Singapore' might be incorrect. Consider using 'stays' if the intent is to describe his residence. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 85% Since this is a test file and the typo doesn't affect the test's functionality (it only checks name="ivan" and age=28), the typo is not critical. The prompt still successfully communicates the test's intent. Comments about typos that don't affect functionality fall into the "not clearly requiring a code change" category. The typo could potentially confuse other developers reading the test. It might also indicate a lack of attention to detail that we should fix. While clarity is good, this is a minor cosmetic issue in test data that doesn't impact functionality. The rules explicitly state not to make purely informative comments that don't clearly require code changes. Delete the comment as it points out a cosmetic issue that doesn't affect test functionality and doesn't clearly require a code change.
20. tests/test_auto_client.py:24
- Draft comment:
Typo: The provider string 'mistral/ministral-8b-latest' appears to contain a misspelling. Verify if 'ministral' should be 'mistral' for consistency. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 85% This appears to be a legitimate typo since "ministral" is not a known model name and other instances in the file use "mistral". However, this is in a test file and the provider string might be intentionally wrong to test error handling. The test is wrapped in a try-except that skips on failure, suggesting these are integration tests that may not always work. We don't have enough context about whether this is an actual error. The provider string could be intentionally misspelled to test error handling. We also don't know if "ministral" might be a valid variant name for some models. While those are valid concerns, the consistent use of "mistral" elsewhere in the file and the fact that "ministral" is not a known model name suggests this is likely a genuine typo. Given the uncertainty about whether this is an intentional test case and following the principle of only keeping comments we're very confident about, we should remove this comment.
21. tests/test_auto_client.py:23
- Draft comment:
Typo: The provider string 'openai/gpt-4o-mini' may contain an extra 'o'. Confirm if it should be 'gpt-4-mini' instead. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 85% While the string does look unusual with the 'o', we don't have strong evidence that this is actually incorrect. This could be a valid model name we're not familiar with. The tests are specifically designed to work with these providers, and the author likely chose these specific strings intentionally. The test file even includes error handling for provider failures. The model name could be a legitimate variant or internal testing model. Without access to OpenAI's full model list or the author's intent, we can't be certain this is a typo. Given the uncertainty and the fact that this is test code specifically designed to work with these providers, we should err on the side of not making assumptions about model names. Delete the comment as we don't have strong evidence that the model name is incorrect, and speculating about potential typos violates our review rules about making speculative comments.
Workflow ID: wflow_PwsDQaSEiPaWHvkR
You can customize
|
Tests are passing here in CI |
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed a3bd29d in 47 seconds. Click for details.
- Reviewed
24lines of code in1files - Skipped
0files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/test.yml:30
- Draft comment:
Good addition of CI env var; ensure similar vars are updated if used in other test steps. - Reason this comment was not posted:
Confidence changes required:0%<= threshold85%None
2. .github/workflows/test.yml:47
- Draft comment:
Removed 'Validate Auto Client' step; confirm its removal doesn't reduce test coverage. - Reason this comment was not posted:
Confidence changes required:80%<= threshold85%None
3. .github/workflows/test.yml:29
- Draft comment:
Added 'CI: True' env var. Ensure consistent use of CI flag across all steps if needed. - Reason this comment was not posted:
Confidence changes required:80%<= threshold85%None
4. .github/workflows/test.yml:46
- Draft comment:
Removed 'Validate Auto Client' step. Confirm that auto_client functionality is tested elsewhere to avoid regression. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_EJra9XAAS5sIunVl
You can customize
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed 9d193be in 2 minutes and 30 seconds. Click for details.
- Reviewed
213lines of code in2files - Skipped
0files when reviewing. - Skipped posting
7draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/test.yml:50
- Draft comment:
Ensure that the additional 'Validate Auto Client' step doesn't conflict with other test runs. Also verify that required ENV variables are set consistently for all providers. - Reason this comment was not posted:
Confidence changes required:50%<= threshold85%None
2. tests/test_auto_client.py:107
- Draft comment:
Error message assertion in test_mode_parameter seems tightly coupled to internal error wording. Consider checking a more stable substring or using a custom error for better resilience. - Reason this comment was not posted:
Confidence changes required:80%<= threshold85%None
3. tests/test_auto_client.py:147
- Draft comment:
As above, consider using a more robust check when comparing the 'CI' environment variable in test_additional_kwargs_passed. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 80% vs. threshold = 85% The comment is technically correct - comparing os.getenv() to True is not ideal since os.getenv() returns a string or None. The suggested change would handle both "true", "True" and "1" values correctly. This is a real code quality issue that could cause bugs. The suggestion is clear and actionable. The current code might actually be intentionally strict, wanting to only match when CI=true exactly. The suggested change would make the check more permissive. While being strict could be intentional, it's more likely this is just a common Python gotcha. Environment variables are conventionally checked this way in Python, and being more permissive is usually better for CI variables. Keep the comment as it suggests a valid improvement to make the CI environment variable check more robust and follow Python conventions.
4. .github/workflows/test.yml:50
- Draft comment:
New 'Validate Auto Client' step is added. Note that the CI env variable is set as a string ('True'), so tests checking 'os.getenv("CI") == True' may not work as expected. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold85%This comment is informative and highlights a potential issue with the CI environment variable being set as a string. It does not ask for confirmation or suggest a specific code change. It is purely informative, which violates the rules.
5. tests/test_auto_client.py:38
- Draft comment:
Avoid comparing os.getenv('CI') == True since os.getenv returns a string. Consider using a truthy check (e.g. if os.getenv('CI')) or comparing to 'True'. - Reason this comment was not posted:
Marked as duplicate.
6. tests/test_auto_client.py:147
- Draft comment:
For consistency and DRY, consider reusing the helper function for checking the CI environment instead of using 'if os.getenv("CI") == True:' directly. - Reason this comment was not posted:
Confidence changes required:80%<= threshold85%None
7. tests/test_auto_client.py:124
- Draft comment:
The assertion checking the exception message substring is brittle. Consider a more robust way to validate the error (e.g. specific exception attributes) to avoid breakage if the message format changes. - Reason this comment was not posted:
Confidence changes required:70%<= threshold85%None
Workflow ID: wflow_HOzb0DM0FNNZ7Yqm
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 3bfea50 in 37 seconds. Click for details.
- Reviewed
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/test.yml:30
- Draft comment:
Using lowercase 'true' aligns with YAML boolean convention. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold85%This comment is purely informative and does not provide any actionable feedback or suggestions for improvement. It simply states a convention without indicating any issue or suggesting a change.
2. .github/workflows/test.yml:30
- Draft comment:
Changing 'CI: True' to 'CI: true' is stylistically consistent with YAML's common lower-case boolean notation. Confirm this is intentional and doesn't affect any dependency expecting a specific format. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =30%<= threshold85%The comment is asking for confirmation on the intention behind a stylistic change, which violates the rule against asking the PR author to confirm their intention. However, it does provide a suggestion about stylistic consistency, which could be useful. Overall, the comment leans more towards asking for confirmation rather than providing a specific suggestion or improvement.
Workflow ID: wflow_LEgtywMGLIGhWwJ3
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 6614c97 in 44 seconds. Click for details.
- Reviewed
35lines of code in2files - Skipped
0files when reviewing. - Skipped posting
6draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/test.yml:30
- Draft comment:
Using INSTRUCTOR_ENV instead of CI for consistency with environment checks. - Reason this comment was not posted:
Confidence changes required:0%<= threshold85%None
2. tests/test_auto_client.py:38
- Draft comment:
Fixed environment variable check to properly compare string values ("CI"). - Reason this comment was not posted:
Confidence changes required:0%<= threshold85%None
3. tests/test_auto_client.py:147
- Draft comment:
Updated environment check to use INSTRUCTOR_ENV for consistency. - Reason this comment was not posted:
Confidence changes required:0%<= threshold85%None
4. .github/workflows/test.yml:29
- Draft comment:
Using INSTRUCTOR_ENV: CI instead of CI is a good move for clarity. Ensure that all tools/tests referencing this environment variable are updated consistently. - Reason this comment was not posted:
Confidence changes required:0%<= threshold85%None
5. tests/test_auto_client.py:38
- Draft comment:
Comparing os.getenv('INSTRUCTOR_ENV') to the string 'CI' is more accurate than a boolean check. Ensure consistency with similar changes (e.g., line 147). - Reason this comment was not posted:
Confidence changes required:0%<= threshold85%None
6. tests/test_auto_client.py:16
- Draft comment:
Typographical error: The phrase 'Ivan is 28 and strays in Singapore' may contain a typo. It looks like 'strays' might be intended to be 'stays'. Please verify and update if necessary. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_Gf1MU8oGkSrNHObG
You can customize
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 805dd3d in 1 minute and 36 seconds. Click for details.
- Reviewed
20lines of code in1files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .github/workflows/test.yml:41
- Draft comment:
Consider extracting common env variables (INSTRUCTOR_ENV, OPENAI_API_KEY, GOOGLE_API_KEY, COHERE_API_KEY) into a job-level setting to reduce repetition. - Reason this comment was not posted:
Confidence changes required:66%<= threshold85%None
2. .github/workflows/test.yml:41
- Draft comment:
Ensure the conditionif: matrix.python-version != '3.11'for the 'Run Auto Client' step is intentional. If auto client tests should run on all Python versions, adjust accordingly. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 85% The comment asks to "ensure" the condition is intentional, which violates our rule about not asking for confirmations. The condition appears to be part of a deliberate test matrix strategy where different types of tests run on different Python versions. The pattern is clear and consistent throughout the file. I could be missing some technical reason why auto client tests specifically should run on Python 3.11. There might be dependencies or features that work better with 3.11. Even if there were technical reasons to run on 3.11, the current setup shows a clear intentional pattern. The comment is asking for confirmation rather than pointing out a clear issue. Delete the comment. It violates the rule about asking for confirmations and doesn't point out any clear issues. The condition appears to be part of an intentional test strategy.
3. .github/workflows/test.yml:45
- Draft comment:
Consider consolidating common environment variable definitions (e.g. OPENAI_API_KEY, GOOGLE_API_KEY, COHERE_API_KEY, INSTRUCTOR_ENV) at the job level to reduce repetition across steps. - Reason this comment was not posted:
Confidence changes required:50%<= threshold85%None
Workflow ID: wflow_nB7EeW4a6An0DT7k
You can customize
ivanleomk
approved these changes
May 6, 2025
There was a problem hiding this comment.
A few things just to consider
-
I think we should implement a set of Literals that match known models. This makes it easy for people to get started and minimise their issues using this string based initialisation
-
I'd like to spend some time rewriting some of the docs for this but the tests and implementation look ok to me out of the box.
|
Only do literals if it's also unions with the default string. We don't want to make people upgrade instructor just to use a new model. |
|
Let's publish first and then fix docs afterwards. This is blocking some stuff I want to do in instructor-classify |
Added in provider literals
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
from_providerfunction for simplified initialization of LLM clientsTODOs before merging
Test plan
🤖 Generated with Claude Code
Important
Introduces
from_providerfor unified string-based LLM client initialization with comprehensive error handling, documentation, and tests.from_providerfunction inauto_client.pyfor string-based LLM client initialization.README.mdwith examples of the new initialization method.string-based-init.mddetailing the unified interface.integrations/index.mdwith provider initialization instructions.test_auto_client.pyfor testingfrom_providerfunction with various providers.test.ymlto includetest_auto_client.pyin CI.1.8.0inpyproject.toml.run.pyexample inexamples/automodeldemonstrating the new interface.This description was created by
for 805dd3d. You can customize this summary. It will automatically update as commits are pushed.