Adding Structured Outputs for Mistral #1391
Merged
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
Deploying instructor-py with
|
| Latest commit: |
49b7214
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://4a1c140e.instructor-py.pages.dev |
| Branch Preview URL: | https://add-mistral-structured-outpu.instructor-py.pages.dev |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 5c64079 in 41 seconds
More details
- Looked at
10lines of code in1files - Skipped
0files when reviewing. - Skipped posting
3drafted comments based on config settings.
1. tests/llm/test_mistral/conftest.py:1
- Draft comment:
Good removal of unused import. Make sure this change is consistent with the rest of the module. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold85%
The comment is asking the author to ensure consistency in the module, which is similar to asking them to double-check their work. This violates the rule against asking the PR author to confirm or ensure things. The comment does not provide a specific suggestion or point out a specific issue, so it should be removed.
2. tests/llm/test_mistral/conftest.py:1
- Draft comment:
Unused import removed: from openai import AsyncOpenAI, OpenAI is no longer used. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =0%<= threshold85%
This comment is purely informative, indicating that an unused import was removed. It does not provide any actionable feedback or suggestions for improvement.
3. tests/llm/test_mistral/conftest.py:7
- Draft comment:
The 'client' and 'aclient' fixtures are identical. If intentional, please add a comment clarifying their distinct use cases or consider refactoring. - Reason this comment was not posted:
Confidence changes required:66%<= threshold85%
None
Workflow ID: wflow_obsfomCg3TKJbIId
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
❌ Changes requested. Reviewed everything up to 0aea217 in 2 minutes and 28 seconds
More details
- Looked at
983lines of code in12files - Skipped
0files when reviewing. - Skipped posting
13drafted comments based on config settings.
1. .github/workflows/test.yml:29
- Draft comment:
Excluding mistralai tests from the CI is intentional, but please confirm that this aligns with the new mistral structured outputs support. - Reason this comment was not posted:
Confidence changes required:0%<= threshold85%
None
2. instructor/client_mistral.py:41
- Draft comment:
The assertion error message lists only MISTRAL_TOOLS even though the allowed modes now include MISTRAL_STRUCTURED_OUTPUTS. Please update the error message to reflect all accepted modes. - 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. instructor/function_calls.py:235
- Draft comment:
Adding a branch for MISTRAL_STRUCTURED_OUTPUTS is clear; however, consider adding a brief comment explaining why parse_mistral_structured_outputs is used instead of standard parsing. - Reason this comment was not posted:
Confidence changes required:33%<= threshold85%
None
4. pyproject.toml:55
- Draft comment:
Dependency version for mistralai has been bumped. Please ensure that all related documentation and tests reflect this version update. - Reason this comment was not posted:
Confidence changes required:0%<= threshold85%
None
5. tests/llm/test_mistral/test_modes.py:1
- Draft comment:
The tests for both asynchronous and synchronous extraction of structured outputs are comprehensive. Ensure that these tests cover edge cases for multiple and single user extraction properly. - Reason this comment was not posted:
Confidence changes required:0%<= threshold85%
None
6. instructor/function_calls.py:481
- Draft comment:
Using an assert for type validation in parse_mistral_structured_outputs may be bypassed in optimized runs. Consider replacing it with an explicit type check and raising a ValueError with a descriptive 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.
7. .github/workflows/test.yml:28
- Draft comment:
Typographical error: The filter criteria uses 'mistralai', but based on the pull request title it seems it should be 'mistral'. Please confirm and update if necessary. - 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 appears to be a speculative comment asking for confirmation about naming. Without seeing the actual test files or knowing the exact package naming convention being used, we can't be certain that 'mistralai' is incorrect. The comment is asking the author to verify something rather than pointing out a clear issue.
I might be missing some context about the standard naming conventions used in the test files. There could be a good reason why 'mistral' would be more appropriate.
Even if there might be a naming convention, the comment violates our rules by asking for verification rather than pointing out a clear issue. We should not ask authors to "confirm and update if necessary".
Delete the comment. It's speculative and asks for verification rather than pointing out a definitive issue that needs fixing.
8. .github/workflows/test.yml:43
- Draft comment:
Typographical error: The filter string in the coverage report command uses 'mistralai', which might be a typo if the intended service name is 'mistral'. Please verify this spelling. - 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 consistent use of 'mistralai' in both places suggests this is intentional. 2. This is likely referring to the Mistral AI company/service. 3. Many AI companies use their full name in package names (like 'anthropic' is used here too). 4. The comment is asking for verification rather than pointing out a definite issue. 5. This falls under the rule about not asking authors to verify things.
I could be wrong about whether Mistral AI's official package name is 'mistralai' vs 'mistral'. Maybe there's a real typo here.
Even if I'm not 100% sure about the package name, the comment violates the rule about not asking authors to verify things. If it was definitely wrong, a correction would be appropriate, but asking for verification is not useful.
Delete the comment because it asks for verification rather than pointing out a definite issue, and the consistent usage suggests this is intentional.
9. instructor/client_mistral.py:37
- Draft comment:
Typo: The assert error message on line 37 reads "Mode be one of {instructor.Mode.MISTRAL_TOOLS}". It might be clearer to say "Mode must be one of {instructor.Mode.MISTRAL_TOOLS, instructor.Mode.MISTRAL_STRUCTURED_OUTPUTS}" to correctly indicate which modes are acceptable. - Reason this comment was not posted:
Marked as duplicate.
10. instructor/function_calls.py:107
- Draft comment:
Typo in the docstring: "Its important to add a docstring..." should be "It's important to add a docstring...". - 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.
11. instructor/process_response.py:104
- Draft comment:
Typo: The debug message reads 'Returning takes from IterableBase' but should be 'Returning tasks from IterableBase'. Please correct the misspelling. - 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.
12. instructor/process_response.py:387
- Draft comment:
Typo: The variable 'implict_forced_tool_message' is misspelled. Consider renaming it to 'implicit_forced_tool_message' for clarity. - 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.
13. tests/llm/test_mistral/util.py:3
- Draft comment:
Typo detected: 'ministral-8b-latest' on line 3. It is likely meant to be 'mistral-8b-latest' to match the naming convention and context (e.g., file name and mode names). Please correct this typo. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_KxrrIKnKialdpbmf
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.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 913a03c in 1 minute and 1 seconds
More details
- Looked at
17 lines of code in 1 files
- Skipped
0 files when reviewing.
- Skipped posting
3 drafted comments based on config settings.
1. .github/workflows/test.yml:27
- Draft comment:
Removed Gemini tests block.
- 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. .github/workflows/test.yml:27
- Draft comment:
Confirm removal of Gemini tests aligns with project requirements.
- 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. .github/workflows/test.yml:31
- Draft comment:
Ensure that the removal of the 'Run Gemini Tests' step is intentional. Verify that Gemini tests are no longer required, as this deletion excludes them from the test runs and coverage reports.
- Reason this comment was not posted:
Comment did not seem useful. Confidence is useful = 30% <= threshold 85%
The comment is asking the PR author to confirm their intention regarding the removal of a test step. This violates the rule against asking for confirmation of intention. However, it does point out a specific change that could have significant implications, which might be useful for the author to consider. Despite this, the comment still primarily focuses on asking for confirmation, which is not allowed.
Workflow ID: wflow_ZiS51c05NREjoro6
17 lines of code in 1 files0 files when reviewing.3 drafted comments based on config settings.1. .github/workflows/test.yml:27
- Draft comment:
Removed Gemini tests block. - 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. .github/workflows/test.yml:27
- Draft comment:
Confirm removal of Gemini tests aligns with project requirements. - 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. .github/workflows/test.yml:31
- Draft comment:
Ensure that the removal of the 'Run Gemini Tests' step is intentional. Verify that Gemini tests are no longer required, as this deletion excludes them from the test runs and coverage reports. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =30%<= threshold85%
The comment is asking the PR author to confirm their intention regarding the removal of a test step. This violates the rule against asking for confirmation of intention. However, it does point out a specific change that could have significant implications, which might be useful for the author to consider. Despite this, the comment still primarily focuses on asking for confirmation, which is not allowed.
wflow_ZiS51c05NREjoro6You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on a1a4405 in 1 minute and 33 seconds
More details
- Looked at
71lines of code in1files - Skipped
0files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. docs/integrations/mistral.md:56
- Draft comment:
The snippet uses Mode but doesn't import it. Consider adding 'from instructor import Mode' to keep the example self-contained. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. docs/integrations/mistral.md:87
- Draft comment:
The sync example now moves the 'model' parameter from from_mistral to completions.create. Update the accompanying documentation to clarify this change. - Reason this comment was not posted:
Comment looked like it was already resolved.
3. docs/integrations/mistral.md:130
- Draft comment:
The async example similarly moves the 'model' parameter to completions.create. Consider noting this change in the docs for consistency with the sync example. - 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.
4. docs/integrations/mistral.md:59
- Draft comment:
Add the missing import for 'Mode' (e.g., from instructor import Mode) in this snippet to avoid a NameError. - Reason this comment was not posted:
Comment looked like it was already resolved.
5. docs/integrations/mistral.md:83
- Draft comment:
Good refactoring: moving the 'model' parameter from the client initialization to the completions.create call improves clarity and decouples configuration from request parameters. - Reason this comment was not posted:
Confidence changes required:0%<= threshold85%
None
6. docs/integrations/mistral.md:40
- Draft comment:
Since the guide introduces two modes, consider adding an example for using Mode.MISTRAL_STRUCTURED_OUTPUTS to showcase both functionalities. - Reason this comment was not posted:
Confidence changes required:33%<= threshold85%
None
Workflow ID: wflow_Xew5UUBMdDxDT2vd
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on e5ac07e in 1 minute and 10 seconds
More details
- Looked at
44lines of code in2files - Skipped
0files when reviewing. - Skipped posting
5drafted comments based on config settings.
1. .github/workflows/test.yml:28
- Draft comment:
The filter now correctly excludes tests for mistral. Consider ensuring that any references to 'mistral' in tests are consistent across the suite, e.g. in all '-k' expressions. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. instructor/client_mistral.py:34
- Draft comment:
Improve assert error message: use a consistent ordered structure (like a tuple) to list allowed modes for clarity. - Reason this comment was not posted:
Comment looked like it was already resolved.
3. .github/workflows/test.yml:28
- Draft comment:
Update test filter: Changed 'mistralai' to 'mistral' for consistency with the new naming. Verify that this pattern is used everywhere. - Reason this comment was not posted:
Confidence changes required:0%<= threshold85%
None
4. .github/workflows/test.yml:37
- Draft comment:
Ensure coverage filter update: Changed 'mistralai' to 'mistral' in the coverage report command for consistency. - Reason this comment was not posted:
Confidence changes required:0%<= threshold85%
None
5. instructor/client_mistral.py:34
- Draft comment:
Consider enhancing the assertion error: Include the invalid mode value for clearer debugging (e.g., f'Invalid mode {mode}; must be one of ...'). - Reason this comment was not posted:
Confidence changes required:33%<= threshold85%
None
Workflow ID: wflow_9vdi2Aw45ScRaqhl
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 99f7579 in 1 minute and 22 seconds
More details
- Looked at
12lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. docs/integrations/mistral.md:177
- Draft comment:
Add the required 'model' parameter for structured outputs with Mistral. This ensures consistency with other examples and clarifies the expected model version. - Reason this comment was not posted:
Comment looked like it was already resolved.
2. docs/integrations/mistral.md:178
- Draft comment:
Consider reordering parameters for consistency. In the simple example, 'model' appears immediately after 'response_model', while here it's after 'messages'. Align them to improve readability. - Reason this comment was not posted:
Confidence changes required:33%<= threshold85%
None
Workflow ID: wflow_75DI0WPZ5OlYipyf
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
ivanleomk
commented
Mar 11, 2025
| @@ -111,7 +111,7 @@ test-docs = [ | |||
| "groq<0.14.0,>=0.4.2", | |||
| "phonenumbers<9.0.0,>=8.13.33", | |||
| "cohere<6.0.0,>=5.1.8", | |||
| "mistralai<2.0.0,>=1.0.3", | |||
| "mistralai<2.0.0,>=1.5.1", | |||
There was a problem hiding this comment.
Mistral doesn't release changelings, so I just updated to the latest version just to be safe
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on eb78c08 in 1 minute and 47 seconds
More details
- Looked at
222lines of code in6files - Skipped
0files when reviewing. - Skipped posting
19drafted comments based on config settings.
1. instructor/client_mistral.py:47
- Draft comment:
Removed conditional branch for MISTRAL_STRUCTURED_OUTPUTS; ensure this is intentional since specialized parsing may be needed. - Reason this comment was not posted:
Confidence changes required:80%<= threshold85%
None
2. instructor/client_mistral.py:59
- Draft comment:
Similarly, async branch now always calls 'client.chat.complete_async'. Verify if specialized async handling for structured outputs is not required. - Reason this comment was not posted:
Confidence changes required:80%<= threshold85%
None
3. instructor/function_calls.py:492
- Draft comment:
Updated parse_mistral_structured_outputs now leverages model_validate_json. Ensure that removing explicit type-checks on 'message.parsed' is acceptable. - Reason this comment was not posted:
Confidence changes required:80%<= threshold85%
None
4. instructor/process_response.py:265
- Draft comment:
handle_mistral_structured_outputs sets response_format using response_format_from_pydantic_model; verify that popping 'tools' and 'response_model' is safe. - Reason this comment was not posted:
Confidence changes required:80%<= threshold85%
None
5. instructor/reask.py:373
- Draft comment:
New functions reask_mistral_structured_outputs and reask_mistral_tools added; ensure their consistency with existing reask patterns. - Reason this comment was not posted:
Confidence changes required:80%<= threshold85%
None
6. tests/llm/test_mistral/test_modes.py:12
- Draft comment:
Test cases now validate structured outputs for lists and single objects; ensure the models provided reflect expected parsing. - Reason this comment was not posted:
Confidence changes required:70%<= threshold85%
None
7. tests/llm/test_mistral/test_retries.py:7
- Draft comment:
Tests now expect retried responses with negative ages; confirm that field_validator logic aligns with this behavior. - Reason this comment was not posted:
Confidence changes required:70%<= threshold85%
None
8. instructor/client_mistral.py:47
- Draft comment:
Removed mode-specific logic (using client.chat.parse) for MISTRAL_STRUCTURED_OUTPUTS. Confirm that defaulting to client.chat.complete is intentional. - Reason this comment was not posted:
Confidence changes required:80%<= threshold85%
None
9. instructor/client_mistral.py:59
- Draft comment:
Async branch now always uses client.chat.complete_async; verify that this change aligns with new structured outputs expectations. - Reason this comment was not posted:
Confidence changes required:80%<= threshold85%
None
10. instructor/function_calls.py:236
- Draft comment:
Passing validation_context and strict to parse_mistral_structured_outputs; ensure downstream JSON validation handles edge cases correctly. - Reason this comment was not posted:
Confidence changes required:70%<= threshold85%
None
11. instructor/function_calls.py:490
- Draft comment:
Removed check on message.parsed; now directly using message.content with model_validate_json. Verify that content always contains valid JSON. - Reason this comment was not posted:
Confidence changes required:80%<= threshold85%
None
12. instructor/process_response.py:265
- Draft comment:
handle_mistral_structured_outputs now uses response_format_from_pydantic_model; ensure mistralai.extra is available and docs updated. - Reason this comment was not posted:
Confidence changes required:70%<= threshold85%
None
13. instructor/reask.py:19
- Draft comment:
New reask functions added for MISTRAL modes (structured outputs and tools). Consider adding detailed logging for debugging reask flows. - Reason this comment was not posted:
Confidence changes required:70%<= threshold85%
None
14. tests/llm/test_mistral/test_modes.py:12
- Draft comment:
Tests for structured outputs (both sync and async) are comprehensive; they validate multiple and single user responses appropriately. - Reason this comment was not posted:
Confidence changes required:0%<= threshold85%
None
15. tests/llm/test_mistral/test_retries.py:20
- Draft comment:
Retry tests correctly trigger the validator (ensuring age becomes negative). Tests appear robust for both sync and async calls. - Reason this comment was not posted:
Confidence changes required:0%<= threshold85%
None
16. instructor/function_calls.py:107
- Draft comment:
Typo: In the docstring for openai_schema, consider changing 'Its important to add a docstring...' to "It's important to add a docstring..." for grammatical correctness. - 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:104
- Draft comment:
Typo: In the debug log message, 'Returning takes from IterableBase' should be 'Returning tasks from IterableBase'. This typo appears here (and similarly in the non-async version) and should be corrected for clarity. - 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. instructor/process_response.py:389
- Draft comment:
Typo: The variable name 'implict_forced_tool_message' seems to be misspelled. Consider renaming it to 'implicit_forced_tool_message' for accuracy and clarity. - 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. tests/llm/test_mistral/test_retries.py:15
- Draft comment:
There is an extra space before the closing parenthesis in the error message on line 15. Consider changing "(Eg. 25 is -25 )" to "(Eg. 25 is -25)" for better 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 consistency in formatting is good, this is an extremely minor stylistic issue in a test file's error message. The space doesn't affect functionality or readability in any meaningful way. The comment feels overly pedantic and doesn't align with the principle of only making important, actionable comments.
Perhaps consistent formatting of error messages is more important than I'm giving it credit for, especially in a library codebase where error messages are part of the user experience.
While consistency is valuable, this particular formatting issue is so minor that bringing it up creates more noise than value in the review process. The error message is already clear and readable.
Delete this comment as it focuses on an extremely minor formatting issue that doesn't meaningfully impact code quality or user experience.
Workflow ID: wflow_YDVcElt0E9WAcQy8
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
ivanleomk
commented
Mar 11, 2025
| @@ -262,6 +262,17 @@ def handle_mistral_tools( | |||
| return response_model, new_kwargs | |||
|
|
|||
|
|
|||
| def handle_mistral_structured_outputs( | |||
There was a problem hiding this comment.
In the mistral SDK, they implement it as
def parse(
self, response_format: Type[CustomPydanticModel], **kwargs: Any
) -> ParsedChatCompletionResponse[CustomPydanticModel]:
"""
Parse the response using the provided response format.
:param Type[CustomPydanticModel] response_format: The Pydantic model to parse the response into
:param Any **kwargs Additional keyword arguments to pass to the .complete method
:return: The parsed response
"""
# Convert the input Pydantic Model to a strict JSON ready to be passed to chat.complete
json_response_format = response_format_from_pydantic_model(response_format)
# Run the inference
response = self.complete(**kwargs, response_format=json_response_format)
# Parse response back to the input pydantic model
parsed_response = convert_to_parsed_chat_completion_response(
response, response_format
)
return parsed_responseso what I did was to make sure we did the parsing ourselves instead of executing the third validation portion there
All tests for mistral are passing now |
jxnl
approved these changes
Mar 12, 2025
eb78c08 to
49b7214
Compare
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.
This PR adds support for the mistral structured outputs mode
Important
Adds structured outputs support for Mistral with new modes, documentation, and tests.
from_mistral()inclient_mistral.py.Mode.MISTRAL_STRUCTURED_OUTPUTSinmode.py.function_calls.pyto handle Mistral structured outputs.process_response.pyandreask.pyto support Mistral structured outputs.mistral.mdwith installation, usage examples, and mode descriptions.ivanleomktomistral.md.test.yml.test_mistraldirectory with tests for structured outputs and retries.mistralaiversion inpyproject.tomlanduv.lock.This description was created by
for eb78c08. It will automatically update as commits are pushed.