new parser for json + thinking output + tests using deepseek r1 on groq #195
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
…using deepseek r1 on groq
🦋 Changeset detectedLatest commit: e78b419 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to ffd8b76 in 1 minute and 37 seconds
More details
- Looked at
275lines of code in7files - Skipped
0files when reviewing. - Skipped posting
6drafted comments based on config settings.
1. src/constants/providers.ts:12
- Draft comment:
Consider adding a default parser toMODE_TO_RESPONSE_PARSERto handle modes that don't have a specific parser defined. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
While having a default parser might seem like a good practice, we don't have enough context to know if other modes need parsers or if they're handled differently elsewhere. The code specifically adds only thinkingJsonParser for THINKING_MD_JSON mode, which seems intentional. Adding a default parser could mask errors if a mode actually needs specific parsing.
The comment might be identifying a legitimate architectural concern - what happens when other modes don't have parsers? There could be potential runtime errors.
Without seeing how this code is used elsewhere, we can't be certain that a default parser is needed or even appropriate. The explicit mapping might be intentional to catch missing parser errors early.
The comment should be deleted as it makes assumptions about the architecture without sufficient context and may suggest a change that goes against the intentional design.
2. src/instructor.ts:192
- Draft comment:
Ensure that the default parser used inchatCompletionStandardis appropriate for all modes or consider adding a warning if a mode-specific parser is not found. - Reason this comment was not posted:
Confidence changes required:70%
ThechatCompletionStandardmethod usesOAIResponseParseras a default parser, which might not be suitable for all modes. This could lead to incorrect parsing if a mode-specific parser is not defined.
3. src/types/index.ts:64
- Draft comment:
Ensure thethinkingproperty inCompletionMetais consistently used across the codebase. - Reason this comment was not posted:
Confidence changes required:50%
Thethinkingproperty is added toCompletionMetain multiple places. This should be consistent across the codebase to avoid potential issues.
4. tests/deepseek.test.ts:58
- Draft comment:
Consider using a configuration or environment variable for the model name to improve flexibility and maintainability. - Reason this comment was not posted:
Confidence changes required:60%
The test case forGROQprovider indeepseek.test.tsuses a hardcoded model name. This could lead to issues if the model name changes or is not available.
5. tests/deepseek.test.ts:50
- Draft comment:
The logic for creating anInstructorinstance and making a call toclient.chat.completions.createis repeated here and intests/mode.test.ts. Consider refactoring to avoid code duplication. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
While DRY is an important principle, I can't verify the duplication without seeing mode.test.ts. The setup code here seems like fairly standard test setup that might need to vary between tests. Test files often need similar but slightly different setups.
I might be too quick to dismiss potential duplication. If the setup is exactly the same, having a shared test utility function could make the tests more maintainable.
Without seeing the other file, I can't be confident enough that this is problematic duplication versus necessary test setup variation. The comment requires more context to be actionable.
Delete the comment since we can't verify the duplication claim without seeing the other file, and test setup code often needs to vary between test cases.
6. tests/mode.test.ts:128
- Draft comment:
The logic for creating anInstructorinstance and making a call toclient.chat.completions.createis repeated here and intests/deepseek.test.ts. Consider refactoring to avoid code duplication. - Reason this comment was not posted:
Marked as duplicate.
Workflow ID: wflow_ZMnFuo7QM9X6Nxgm
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Deploying instructor-js with
|
| Latest commit: |
718d093
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://7b1946a7.instructor-js.pages.dev |
| Branch Preview URL: | https://thinking-mode.instructor-js.pages.dev |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 6e2de78 in 12 seconds
More details
- Looked at
25lines of code in2files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. .github/workflows/test-pr.yml:26
- Draft comment:
Ensure that theGROQ_API_KEYis correctly set in the repository secrets to avoid runtime errors. - Reason this comment was not posted:
Confidence changes required:50%
The PR adds a new environment variableGROQ_API_KEYto the GitHub Actions workflows. This is consistent across bothtest-pr.ymlandtest.yml. The change seems correct and necessary for the new functionality.
2. .github/workflows/test-pr.yml:26
- Draft comment:
Ensure that documentation and tests are updated to reflect the addition of GROQ_API_KEY. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_IlBl15huF80s6ZYi
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 718d093 in 16 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. tests/deepseek.test.ts:43
- Draft comment:
If the library code or functionality has changed, ensure that the documentation and tests are updated accordingly. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_Ey3tgEL4eB9E7LWu
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 e78b419 in 20 seconds
More details
- Looked at
11lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. .changeset/little-cougars-bow.md:5
- Draft comment:
Ensure the description is clear and concise, explaining the purpose of the new mode. - Reason this comment was not posted:
Confidence changes required:0%
The changeset file is correctly formatted and provides a clear description of the change.
2. .changeset/little-cougars-bow.md:1
- Draft comment:
Ensure this new markdown file is added to mkdocs.yml. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_rcMJqHSSM3MYftRa
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Merged
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.
Important
Adds
THINKING_MD_JSONmode for parsing JSON with thinking blocks, updates dependencies, and includes tests forGROQprovider.THINKING_MD_JSONmode to parse thinking blocks from markdown JSON responses inproviders.tsandinstructor.ts.MODE_TO_RESPONSE_PARSERto includethinkingJsonParserforTHINKING_MD_JSONmode.THINKING_MD_JSONmode forGROQprovider with modeldeepseek-r1-distill-llama-70b.deepseek.test.tsto testTHINKING_MD_JSONmode withGROQprovider.mode.test.tsto includeTHINKING_MD_JSONmode in test cases.zod-streamto3.0.0inpackage.json.GROQ_API_KEYtotest-pr.ymlandtest.yml.Dimitri Kennedyto contributors inpackage.json.This description was created by
for e78b419. It will automatically update as commits are pushed.