refactor: organize client files into dedicated subdirectory #1632
+1,485
−105
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
- Move all client_*.py files to instructor/clients/ subdirectory - Update all imports to reference new location - Maintain backward compatibility for all existing imports - Add comprehensive README for contributing new clients - Fix conditional imports to avoid dependency errors This refactoring improves code organization and makes it easier for contributors to add support for new LLM providers. 🤖 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 everything up to 1e8ac61 in 1 minute and 34 seconds. Click for details.
- Reviewed
640lines of code in19files - 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. instructor/__init__.py:54
- Draft comment:
Updated conditional imports now point to the new subdirectory. This is clean and maintains backward compatibility. - 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 what has been done without offering any guidance or questions for the PR author.
2. instructor/auto_client.py:102
- Draft comment:
Import paths adjusted to use 'instructor.clients'. Verify consistency across all provider cases. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold85%The comment is asking the PR author to verify consistency across all provider cases, which violates the rule against asking the author to double-check things. It does not provide a specific suggestion or point out a specific issue, making it not useful according to the guidelines.
3. instructor/clients/README.md:80
- Draft comment:
Comprehensive contributor guide is added. Consider updating mkdocs.yml to include this new 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% This file is in instructor/clients/README.md, not in the docs directory. The rule about mkdocs.yml only applies to files in the docs directory. This is a standard README in a code directory meant for developers working directly with the code, not part of the public documentation that would be built with mkdocs. The comment correctly identifies that this is new documentation being added. Maybe this content should actually be moved to the docs directory? While the content could potentially be moved to docs, that would be a separate suggestion. As it stands, this is a README in a code directory which doesn't need to be in mkdocs.yml. The comment should be deleted since it incorrectly suggests updating mkdocs.yml for a README that is not in the docs directory.
4. instructor/clients/__init__.py:14
- Draft comment:
The clients submodule initializer is clear; it serves as a package marker and exposes all factory functions. - 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 provided, which prohibit purely informative comments.
5. instructor/process_response.py:708
- Draft comment:
Vertex AI import update is consistent with the new directory structure. Nice work on keeping the client code separated. - 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 praises the update for being consistent with the new directory structure, which is not aligned with the rules provided.
6. tests/llm/test_vertexai/test_deprecated_async.py:13
- Draft comment:
Patch target updated to new module path; tests correctly reflect the refactor. - 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 the patch target has been updated and that tests reflect the refactor, which does not align with the rules provided.
7. tests/llm/test_vertexai/test_message_parser.py:3
- Draft comment:
Import for vertexai_message_parser is updated to the new path. Test cases cover valid and error scenarios well. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold85%This comment is purely informative, stating that the import path for vertexai_message_parser is updated and that test cases cover scenarios well. It does not provide any actionable feedback or suggestions for improvement.
Workflow ID: wflow_iksn7Qi5cjZtNsGP
You can customize 
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
instructor | 74f30c7 | Commit Preview URL | Jun 27 2025, 06:14 PM |
Adds google-genai>=1.5.0 and jsonref<2.0.0,>=1.1.0 to the test-docs dependency group to fix CI test failures for Google GenAI tests.
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed 6df777c in 49 seconds. Click for details.
- Reviewed
39lines of code in2files - 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. pyproject.toml:155
- Draft comment:
Consider specifying an upper bound for the 'google-genai' dependency to prevent unintended breaking changes in future major releases. - Reason this comment was not posted:
Confidence changes required:66%<= threshold85%None
2. uv.lock:1860
- Reason this comment was not posted:
Confidence changes required:66%<= threshold85%None
Verify that 'google-genai' and 'jsonref' are intended as optional dependencies. Consider using extras if they are not required by core functionality.
Workflow ID: wflow_6KwSsBob5KFa5n67
You can customize
- Move all client conditional imports to clients/__init__.py - Remove duplicate conditional imports from main __init__.py - Fix reask.py import paths to use clients subdirectory - Remove unused importlib.util import - Fix unused parameter warning in provider_openai.py - Resolves import symbol errors in auto_client.py and clients module
There was a problem hiding this comment.
Caution
Changes requested ❌
Reviewed 74f30c7 in 1 minute and 53 seconds. Click for details.
- Reviewed
1368lines of code in9files - 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. instructor/__init__.py:55
- Draft comment:
The loop that dynamically adds client functions (skipping 'from_openai') is clever. Consider adding an inline comment explaining why 'from_openai' is explicitly skipped to help future maintainers. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. instructor/providers/provider_openai.py:265
- Draft comment:
In the _handle_json_modes method, the handling of different message content types (string vs list) is a bit brittle. Consider strengthening type checks or adding comments explaining expected message formats to improve maintainability. - 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 code is actually reasonably well structured with clear type checks and error handling. While adding comments about message formats could help, the code itself is not particularly brittle - it handles the known valid formats and explicitly errors on invalid ones. The comment seems to be suggesting improvements that aren't clearly necessary. The code could benefit from documentation about expected message formats, but the handling itself seems robust rather than brittle. The existing type checks and error handling make the code more robust than the comment suggests. While documentation could help, calling the code "brittle" seems inaccurate. The comment should be removed as it incorrectly characterizes the code as brittle when it has appropriate type checking and error handling.
3. instructor/reask.py:176
- Draft comment:
The updated import paths for vertexai (e.g. in reask_vertexai_tools and reask_vertexai_json) correctly reflect the new subdirectory organization. Ensure that documentation and any other references are updated accordingly. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold85%This comment is asking the PR author to ensure that documentation and other references are updated, which violates the rule against asking the author to update documentation or confirm intentions. The comment is not making a specific suggestion or pointing out a specific issue with the code itself.
4. tests/providers/test_base.py:97
- Draft comment:
The test_from_client in the MockProvider assumes that kwargs passed to from_client are not modified. Since it simply returns the kwargs, consider adding a comment in the mock implementation about expected behavior for clarity. - Reason this comment was not posted:
Confidence changes required:33%<= threshold85%None
5. instructor/providers/provider_openai.py:251
- Draft comment:
Typo: The message "Return the correct JSON response within ajson codeblock. not the JSON_SCHEMA" appears to have a punctuation mistake. Consider replacing the period with a comma (or rephrasing) for clarity, for example: "Return the correct JSON response within ajson``` code block, not the JSON_SCHEMA". - 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% While this is a user-facing message, the suggested changes are quite minor and don't materially impact functionality. The current message is understandable and gets the point across. The backticks formatting suggestion is stylistic and the punctuation change is very minor. This seems like an unnecessary nitpick that doesn't warrant a PR comment. The message is user-facing and part of the API, so clarity and professionalism could be important. Poor formatting might confuse users. While professional formatting is good, this change is too minor to justify a PR comment. The current message works fine and users will understand it. We should focus PR comments on more substantial issues. Delete this comment as it suggests only minor stylistic changes that don't materially impact functionality or user understanding.
Workflow ID: wflow_8uKP3rTT3KGwadF1
You can customize
|
|
||
| __all__ = ["from_openai"] | ||
|
|
||
| # Conditional imports based on available packages |
There was a problem hiding this comment.
The organization of conditional client imports using importlib.util.find_spec is clear. For future DRY improvements, consider abstracting the repeated pattern for conditional imports into a helper function.
|
|
||
| assert client == mock_client | ||
| mock_openai.OpenAI.assert_called_once_with(api_key="test-key") | ||
| """Test creating sync OpenAI client.""" |
There was a problem hiding this comment.
There appears to be a duplicate block in the test_create_sync_client method (after the with patch block). Consider removing the repeated test code to avoid redundancy.
| def create_client( | ||
| self, model_name: str, async_client: bool = False, **kwargs | ||
| ) -> Any: | ||
| """Create the underlying client for from_provider().""" |
There was a problem hiding this comment.
Typo: The docstring for create_client states "Create the underlying client for from_provider()." However, there is no from_provider method; consider revising this description, perhaps to reference from_client or clarify the intended meaning.
Suggested change
| """Create the underlying client for from_provider().""" | |
| """Create the underlying client for use with from_client().""" |
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
client_*.pyfiles into a dedicatedinstructor/clients/subdirectoryChanges
instructor/clients/directory: All 12 client files moved to a dedicated subdirectory__init__.py,auto_client.py,process_response.py, and test filesinstructor/clients/README.mdwith detailed instructions for adding new provider supportBenefits
Test plan
from instructor import from_anthropicetc. still work🤖 Generated with Claude Code
Important
Reorganized client files into
instructor/clients/subdirectory, updated imports, and added contributor documentation.client_*.pyfiles toinstructor/clients/subdirectory.instructor/clients/__init__.pyfor managing imports.__init__.py,auto_client.py, andprocess_response.pyto reflect new file locations.__init__.pyandclients/__init__.pyto prevent errors when optional 8000 dependencies are missing.instructor/clients/README.mdwith guidelines for adding new client integrations.This description was created by
for 74f30c7. You can customize this summary. It will automatically update as commits are pushed.