-
-
Notifications
You must be signed in to change notification settings - Fork 751
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
feat: Adding support for Open Router #1386
base: main
Are you sure you want to change the base?
Conversation
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.
👍 Looks good to me! Reviewed everything up to f643b10 in 2 minutes and 2 seconds
More details
- Looked at
449
lines of code in8
files - Skipped
0
files when reviewing. - Skipped posting
20
drafted comments based on config settings.
1. docs/integrations/index.md:33
- Draft comment:
Consider grouping similar integrations consistently. The new OpenRouter link appears correct, but double-check ordering with other routing providers. - Reason this comment was not posted:
Confidence changes required:70%
<= threshold85%
None
2. docs/integrations/openrouter.md:1
- Draft comment:
The examples in the new OpenRouter guide are clear. Ensure to mention explicitly any limitations when using tools on models lacking native support. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold85%
None
3. instructor/client.py:516
- Draft comment:
The added assertion for Provider.OPENROUTER is good. Ensure documentation highlights that only TOOLS, OPENROUTER_STRUCTURED_OUTPUTS, and JSON modes are supported with OpenRouter. - Reason this comment was not posted:
Confidence changes required:60%
<= threshold85%
None
4. instructor/function_calls.py:221
- Draft comment:
The added check for no completion choices is useful. Consider including additional context in the error message to aid debugging. - Reason this comment was not posted:
Confidence changes required:80%
<= threshold85%
None
5. instructor/mode.py:59
- Draft comment:
The new OPENROUTER_STRUCTURED_OUTPUTS mode is consistently added. Confirm that naming conventions match existing modes and update any related documentation. - Reason this comment was not posted:
Confidence changes required:70%
<= threshold85%
None
6. instructor/process_response.py:845
- Draft comment:
The mode handler mapping now includes OPENROUTER_STRUCTURED_OUTPUTS. Verify that the handling function (handle_openrouter_structured_outputs) is fully tested against edge cases. - Reason this comment was not posted:
Confidence changes required:70%
<= threshold85%
None
7. mkdocs.yml:247
- Draft comment:
The navigation includes duplicate entries for several providers. Review to remove potential duplicates (e.g., AWS Bedrock, Azure OpenAI are repeated) to keep the docs navigation clean. - 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. docs/integrations/openrouter.md:12
- Draft comment:
Consider adding a brief note that explicitly warns users that using models which do not support tool calling or structured outputs may lead to unforeseen behavior. This extra guidance will help prevent confusion. - Reason this comment was not posted:
Comment looked like it was already resolved.
9. instructor/client.py:516
- Draft comment:
Instead of using a bare assert to check the mode for OPENROUTER, consider raising a more descriptive exception. This is more robust in production where asserts may be disabled. - Reason this comment was not posted:
Confidence changes required:80%
<= threshold85%
None
10. instructor/function_calls.py:245
- Draft comment:
Addition of 'Mode.OPENROUTER_STRUCTURED_OUTPUTS' into the JSON parsing group is consistent. Ensure all similar modes follow this pattern. - Reason this comment was not posted:
Confidence changes required:70%
<= threshold85%
None
11. instructor/mode.py:59
- Draft comment:
New enum member OPENROUTER_STRUCTURED_OUTPUTS is added. It’s good to see it included in both tool_modes and json_modes. Verify documentation and examples reference this mode correctly. - Reason this comment was not posted:
Confidence changes required:70%
<= threshold85%
None
12. instructor/process_response.py:748
- Draft comment:
The new handler 'handle_openrouter_structured_outputs' is clearly implemented with appropriate setup of the JSON schema. Good job ensuring strict validation. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold85%
None
13. instructor/utils.py:91
- Draft comment:
In the get_provider function, the addition for 'openrouter' is straightforward. Consider if matching should be case-insensitive to avoid edge cases. - Reason this comment was not posted:
Confidence changes required:60%
<= threshold85%
None
14. mkdocs.yml:248
- Draft comment:
The navigation now includes the OpenRouter integration. Double-check for duplicate entries in integrations to avoid potential user confusion. - Reason this comment was not posted:
Confidence changes required:50%
<= threshold85%
None
15. instructor/function_calls.py:107
- Draft comment:
Typo: In the docstring for openai_schema, consider changing 'Its important' to "It's important" for proper grammar. - 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.
16. instructor/process_response.py:104
- Draft comment:
Typo: The debug message 'Returning takes from IterableBase' should be corrected to 'Returning tasks from IterableBase' for clarity and 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.
17. instructor/process_response.py:378
- Draft comment:
Typo: The variable name 'implict_forced_tool_message' should be renamed to 'implicit_forced_tool_message' to correct the spelling. - 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.
18. mkdocs.yml:187
- Draft comment:
Consider removing the extra space in 'Multimodal :' for consistency with other entries (e.g. 'Overview:' and 'Prompting:'). - 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.
19. mkdocs.yml:267
- Draft comment:
Please double-check the file name 'simtom.md' associated with 'Simulate A Perspective'. It might be intended to be something like 'simulate.md' or 'simptom.md' if it is a typo. - 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.
20. mkdocs.yml:317
- Draft comment:
Please verify if 'Recurs.-of-Thought' is spelled correctly. It might be a typo; perhaps it was meant to be 'Recursive-of-Thought' or similar. - 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_bEMYNbb6UxRRLBRY
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
@@ -245,6 +252,7 @@ def from_response( | |||
Mode.CEREBRAS_JSON, | |||
Mode.FIREWORKS_JSON, | |||
Mode.PERPLEXITY_JSON, | |||
Mode.OPENROUTER_STRUCTURED_OUTPUTS, |
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 weird thing is that when we use OpenRouter's structured outputs implementation, the completion comes back as this
This is for Gemini
choices=[Choice(finish_reason='stop', index=0, logprobs=None, message=ChatCompletionMessage(content='{"age": 25, "name": "Jason"}', refusal=None, role='assistant', audio=None, function_call=None, tool_calls=None),
This is for OpenAI
choices=[Choice(finish_reason='stop', index=0, logprobs=None, message=ChatCompletionMessage(content='{"name":"Jason","age":25}', refusal=None, role='assistant', audio=None, function_call=None, tool_calls=None)
But the same use of OpenAI's structured outputs with the default OpenAI api returns it as a function call instead.
choices=[Choice(finish_reason='stop', index=0, logprobs=None, message=ChatCompletionMessage(content=None, refusal=None, role='assistant', audio=None, function_call=None, tool_calls=[ChatCompletionMessageToolCall(id='call_nrxEG6W16RNL1UDpTVQsIxVr', function=Function(arguments='{"name":"Jason","age":25}', name='User'), type='function')]))]
Deploying instructor-py with
|
Latest commit: |
946c2b3
|
Status: | ✅ Deploy successful! |
Preview URL: | https://e9e65b43.instructor-py.pages.dev |
Branch Preview URL: | https://add-openrouter-so.instructor-py.pages.dev |
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.
👍 Looks good to me! Incremental review on 14bfba8 in 1 minute and 57 seconds
More details
- Looked at
53
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
8
drafted comments based on config settings.
1. docs/integrations/openrouter.md:10
- Draft comment:
Ensure this new documentation file is added in mkdocs.yml so it's included in the docs build. - 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. docs/integrations/openrouter.md:50
- Draft comment:
The extra_body parameter is repeated across examples. Consider adding a brief comment explaining its necessity for provider-specific configuration. - Reason this comment was not posted:
Confidence changes required:80%
<= threshold85%
None
3. docs/integrations/openrouter.md:88
- Draft comment:
Verify that the async example's use of extra_body mirrors the sync one. A short note might help clarify the practice across async and sync implementations. - Reason this comment was not posted:
Confidence changes required:80%
<= threshold85%
None
4. docs/integrations/openrouter.md:194
- Draft comment:
Double-check that the extra_body configuration is consistent with any recent changes to provider routing on the OpenAI client. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%
<= threshold85%
This comment is asking the PR author to double-check something, which violates the rule against asking for confirmation or double-checking. It doesn't provide a specific suggestion or point out a specific issue.
5. docs/integrations/openrouter.md:10
- Draft comment:
Consider using 'provider-specific' with a hyphen for clarity. This line explains that extra_body is used to pass custom provider configurations. - Reason this comment was not posted:
Confidence changes required:80%
<= threshold85%
None
6. docs/integrations/openrouter.md:50
- Draft comment:
The extra_body parameter is now consistently added to the completion calls. Consider adding a brief inline note or a link to documentation explaining its purpose, especially for users needing advanced provider routing options. - 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 file already has a clear explanation at line 10. The comment suggests adding redundant information. The usage of extra_body is also demonstrated consistently in multiple code examples. The current documentation is sufficient and adding more would be redundant.
I might be overlooking the value of having more detailed documentation about advanced routing options. Some users might benefit from more explicit guidance.
While more documentation could help some users, the current explanation is clear and concise, and the multiple examples demonstrate proper usage. Adding inline comments would make the documentation more cluttered.
The comment should be deleted as it suggests adding redundant documentation when the file already has a clear explanation and multiple examples of extra_body usage.
7. docs/integrations/openrouter.md:17
- Draft comment:
The removed note on additional configuration options (manual request for advanced provider routing) might be useful for advanced users. Consider linking to the official provider routing docs to avoid confusion. - Reason this comment was not posted:
Confidence changes required:80%
<= threshold85%
None
8. docs/integrations/openrouter.md:10
- Draft comment:
Consider changing 'Provider specific configuration' to 'provider-specific configuration' (with a hyphen) for improved clarity and 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%
While technically correct from a style perspective, this is an extremely minor change that doesn't affect functionality or understanding. The current wording is perfectly clear and understandable. The rules state not to make purely informative comments or comments about obvious/unimportant things.
The hyphenation could improve readability slightly, and technical documentation should strive for consistency in style.
While style consistency is good, this change is too minor to warrant a PR comment. It doesn't meaningfully impact the documentation's clarity or usefulness.
Delete this comment as it's too minor and violates the rule about not making purely informative or unimportant comments.
Workflow ID: wflow_VMaoh0JnzS81VLr7
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
This PR adds examples for how we can support OpenRouter and experimental support for Structured Outputs with Open Router.
I tried to add explicit callouts too that you must make sure that your model supports tool calling or structured outputs, or else you will get unforeseen behaviour. This happens because we cannot set open router specific configurations using the OpenAI client.
So users can for instance make tool calls against models.. that don't support structured outputs at all for some reason.
But otherwise, the way I've verified this is by making sure that OpenAI throws an error when we try to pass in a regex pattern ( as written in the docs )
Important
Adds support for OpenRouter, including structured outputs and tool calling, with documentation and examples.
from_openai()
inclient.py
, allowing modesTOOLS
,OPENROUTER_STRUCTURED_OUTPUTS
, andJSON
.handle_openrouter_structured_outputs()
inprocess_response.py
to manage structured outputs.from_response()
infunction_calls.py
to handle OpenRouter errors and structured outputs.openrouter.md
with examples for using OpenRouter with Instructor.index.md
to include OpenRouter in the list of integrations.mkdocs.yml
to include OpenRouter in the navigation.OPENROUTER
toProvider
enum inutils.py
.OPENROUTER_STRUCTURED_OUTPUTS
toMode
enum inmode.py
.This description was created by
for 14bfba8. It will automatically update as commits are pushed.