feat: implement jinja templating and rename kwarg to context
#1011
Conversation
This file contains 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 with
|
| Latest commit: |
d889b15
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ff02e471.instructor.pages.dev |
| Branch Preview URL: | https://jinja-support.instructor.pages.dev |
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Reviewed everything up to a03109c in 57 seconds
More details
- Looked at
1168lines of code in9files - Skipped
0files when reviewing. - Skipped posting
10drafted comments based on config settings.
1. instructor/function_calls.py:108
- Draft comment:
The parametervalidation_contextis still being used here without handling its deprecation. Consider using thehandle_contextfunction to manage the transition fromvalidation_contexttocontext. - Reason this comment was not posted:
Confidence changes required:80%
The PR introduces a newcontextparameter and deprecatesvalidation_context. However, there are still instances wherevalidation_contextis used without a deprecation warning or handling. This should be addressed to ensure consistency and proper deprecation handling.
2. instructor/process_response.py:106
- Draft comment:
The parametervalidation_contextis still being used here without handling its deprecation. Consider using thehandle_contextfunction to manage the transition fromvalidation_contexttocontext. - Reason this comment was not posted:
Confidence changes required:80%
The PR introduces a newcontextparameter and deprecatesvalidation_context. However, there are still instances wherevalidation_contextis used without a deprecation warning or handling. This should be addressed to ensure consistency and proper deprecation handling.
3. instructor/retry.py:187
- Draft comment:
The parametervalidation_contextis still being used here without handling its deprecation. Consider using thehandle_contextfunction to manage the transition fromvalidation_contexttocontext. - Reason this comment was not posted:
Confidence changes required:80%
The PR introduces a newcontextparameter and deprecatesvalidation_context. However, there are still instances wherevalidation_contextis used without a deprecation warning or handling. This should be addressed to ensure consistency and proper deprecation handling.
4. instructor/retry.py:266
- Draft comment:
The parametervalidation_contextis still being used here without handling its deprecation. Consider using thehandle_contextfunction to manage the transition fromvalidation_contexttocontext. - Reason this comment was not posted:
Confidence changes required:80%
The PR introduces a newcontextparameter and deprecatesvalidation_context. However, there are still instances wherevalidation_contextis used without a deprecation warning or handling. This should be addressed to ensure consistency and proper deprecation handling.
5. docs/blog/posts/jinja-proposal.md:1
- Draft comment:
The newjinja-proposal.mdfile should be added to themkdocs.ymlto ensure it is included in the documentation. - Reason this comment was not posted:
Confidence changes required:80%
The newjinja-proposal.mdfile should be added to themkdocs.ymlto ensure it is included in the documentation.
6. instructor/client.py:65
- Draft comment:
Thevalidation_contextparameter is deprecated. Consider updating to usecontextinstead. This applies to other overloads in this file as well. - Reason this comment was not posted:
Confidence changes required:80%
Thecreatemethod ininstructor/client.pyhas multiple overloads with thevalidation_contextparameter, which is deprecated. It should be updated to usecontextinstead.
7. instructor/function_calls.py:108
- Draft comment:
Thevalidation_contextparameter is deprecated. Consider updating to usecontextinstead. - Reason this comment was not posted:
Confidence changes required:80%
Thevalidation_contextparameter is deprecated and should be updated to usecontextinstead in thefrom_responsemethod.
8. instructor/patch.py:55
- Draft comment:
Ensure that thehandle_contextfunction reflects the deprecation ofvalidation_context. - Reason this comment was not posted:
Confidence changes required:80%
Thehandle_contextfunction ininstructor/patch.pyis handling bothcontextandvalidation_context. Sincevalidation_contextis deprecated, ensure that the function is updated to reflect this change.
9. instructor/process_response.py:106
- Draft comment:
Thevalidation_contextparameter is deprecated. Consider updating to usecontextinstead. This applies to other functions in this file as well. - Reason this comment was not posted:
Confidence changes required:80%
Theprocess_responsefunction ininstructor/process_response.pyusesvalidation_context, which is deprecated. It should be updated to usecontextinstead.
10. instructor/retry.py:187
- Draft comment:
Thevalidation_contextparameter is deprecated. Consider updating to usecontextinstead. This applies to other functions in this file as well. - Reason this comment was not posted:
Confidence changes required:80%
Theretry_syncfunction ininstructor/retry.pyusesvalidation_context, which is deprecated. It should be updated to usecontextinstead.
Workflow ID: wflow_vHNotWBouDNKSyNt
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 6c85df2 in 35 seconds
More details
- Looked at
38lines of code in2files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. instructor/patch.py:163
- Draft comment:
validation_contextis deprecated. Consider updating to usecontextinstead, as per the PR description. - Reason this comment was not posted:
Confidence changes required:80%
The PR description mentions deprecatingvalidation_contextin favor ofcontext. However, theprocess_response_asyncandprocess_responsefunctions still usevalidation_context. This should be updated to reflect the newcontextparameter.
2. instructor/process_response.py:22
- Draft comment:
Thevalidation_contextparameter is deprecated. Consider updating this to usecontextinstead, and handle the deprecation accordingly. - Reason this comment was not posted:
Confidence changes required:80%
Thevalidation_contextparameter is deprecated and replaced bycontext. However, theprocess_response_asyncandprocess_responsefunctions still usevalidation_contextwithout any deprecation warning or handling. This should be updated to reflect the changes made in the PR.
Workflow ID: wflow_KAxgf7KCOfDN24nj
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
ivanleomk
reviewed
Sep 20, 2024
|
@ivanleomk oh man can you help take a look at some of these errors? |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 69cec4d in 22 seconds
More details
- Looked at
10lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. .github/workflows/pyright.yml:47
- Draft comment:
Consider specifying the version forjakebailey/pyright-actionto ensure consistent behavior across runs. - Reason this comment was not posted:
Confidence changes required:80%
The removal of the 'version' key in the pyright-action step might lead to using the latest version by default, which could introduce unexpected changes or incompatibilities. It's generally a good practice to specify the version to ensure consistent behavior.
2. .github/workflows/pyright.yml:47
- Draft comment:
Consider specifying a version forjakebailey/pyright-actionto ensure consistent behavior. - Reason this comment was not posted:
Confidence changes required:50%
The version specification for the pyright-action has been removed, which might affect the consistency of the action's behavior.
Workflow ID: wflow_aZBYjWcEJp4ME82j
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 2f967ff in 22 seconds
More details
- Looked at
10lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. .github/workflows/pyright.yml:49
- Draft comment:
Ensure that the specified Pyright version (1.1.373) is compatible with the current setup and dependencies. This is crucial for maintaining consistency and avoiding potential issues. - Reason this comment was not posted:
Confidence changes required:50%
The version of Pyright is being set in the workflow file, which is a good practice for consistency. However, it's important to ensure that this version is compatible with the rest of the setup.
2. .github/workflows/pyright.yml:49
- Draft comment:
If the pyright-action version is updated, ensure the documentation reflects this change. - Reason this comment was not posted:
Confidence changes required:80%
The version update for the pyright-action should be documented.
Workflow ID: wflow_Kqfa5ZbtcP8G8YNN
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 d7e7baa in 27 seconds
More details
- Looked at
34lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. instructor/client_cohere.py:64
- Draft comment:
Thevalidation_contextparameter is deprecated and should be removed from the function signature. Update the function to rely solely on thecontextparameter. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_V3uJzjSWdQ20yZw8
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 a274ef2 in 27 seconds
More details
- Looked at
18lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. instructor/patch.py:123
- Draft comment:
Avoid using# type:ignoreunless necessary. It can hide potential type errors. This comment is also applicable on lines 124 and 126. - Reason this comment was not posted:
Confidence changes required:50%
The use of# type:ignoreis unnecessary and should be avoided unless absolutely necessary. It can hide potential type errors.
Workflow ID: wflow_FtGMHaYENG821DZ5
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 65d178f in 21 seconds
More details
- Looked at
13lines of code in1files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. instructor/patch.py:245
- Draft comment:
Avoid using# type: ignoreunless absolutely necessary. It can hide potential type issues. Consider addressing the root cause of the type warning instead. - Reason this comment was not posted:
Confidence changes required:80%
The use of# type: ignoreshould be minimized as it can hide potential type issues. It's better to address the root cause of the type warning.
2. instructor/patch.py:246
- Draft comment:
The# type: ignorecomment on the return statement might indicate a potential issue. Ensure this is necessary and document why it's ignored. - Reason this comment was not posted:
Confidence changes required:50%
The return statement innew_create_synchas a# type: ignorecomment, which might indicate a potential issue that should be addressed or documented properly.
Workflow ID: wflow_QlIygFyLipo4i9G7
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 b55e0db in 40 seconds
More details
- Looked at
138lines of code in3files - Skipped
0files when reviewing. - Skipped posting
4drafted comments based on config settings.
1. instructor/client_cohere.py:13
- Draft comment:
handle_cohere_templatingis imported but not used in the visible changes. Ensure that its usage is correctly implemented or remove the import if unnecessary. - Reason this comment was not posted:
Confidence changes required:50%
The import statement forhandle_cohere_templatingwas added, but the function is not used in the visible diff. This might indicate an incomplete change or a missing usage.
2. instructor/patch.py:80
- Draft comment:
Consider creating a copy ofnew_kwargsbefore modifying it to avoid side effects:
new_kwargs = new_kwargs.copy()
- Reason this comment was not posted:
Confidence changes required:50%
Thehandle_cohere_templatingfunction is modifyingnew_kwargsdirectly, which could lead to unexpected side effects ifnew_kwargsis used elsewhere. It's better to create a copy ofnew_kwargsbefore modifying it.
3. instructor/patch.py:98
- Draft comment:
Consider creating a copy ofmessagesbefore modifying it to avoid side effects:
messages = [message.copy() for message in messages]
- Reason this comment was not posted:
Confidence changes required:50%
Thehandle_templatingfunction is modifying themessageslist in place, which could lead to unexpected side effects ifmessagesis used elsewhere. It's better to create a copy ofmessagesbefore modifying it.
4. tests/llm/test_cohere/test_json_schema.py:100
- Draft comment:
Assertions should include error messages for clarity. This applies to all assertions in this file. - Reason this comment was not posted:
Confidence changes required:80%
The assertion statements in the test cases lack error messages, which are important for debugging when tests fail.
Workflow ID: wflow_hbFDuSTOwnXiKBuE
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 d889b15 in 33 seconds
More details
- Looked at
36lines of code in2files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. tests/test_formatting.py:3
- Draft comment:
Consider importingpytestat the top of the file to maintain consistency and avoid potential issues ifpytestis used in the future. - Reason this comment was not posted:
Confidence changes required:50%
The test filetest_formatting.pyis missing an import statement forpytest, which is used in the test filetest_validation_context.py. This could lead to issues ifpytestis used intest_formatting.pyin the future.
2. tests/test_formatting.py:1
- Draft comment:
Addimport pytestat the top of the file to ensure the tests can be run using pytest. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment suggests addingimport pytest, but the test functions do not seem to use any pytest-specific features. The removal ofimport pytestmight be intentional if the tests can run without it. The comment does not provide strong evidence that addingimport pytestis necessary.
I might be missing the context of how these tests are executed. If they are part of a larger suite that uses pytest, the import might be necessary for consistency or future use.
The tests do not currently use pytest-specific features, so the import is not strictly necessary. If future changes require pytest, the import can be added then.
The comment should be deleted because there is no strong evidence thatimport pytestis necessary for the current test functions.
Workflow ID: wflow_zrUoQR3LtF8lcVmg
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
ivanleomk
reviewed
Sep 24, 2024
|
|
||
| if "messages" in new_kwargs: | ||
| new_kwargs["messages"] = handle_templating(new_kwargs["messages"], context) | ||
| elif "message" in new_kwargs and "chat_history" in new_kwargs: |
There was a problem hiding this comment.
@jxnl , Cohere client seems to have a separate async method in its definition, that's why we only modify the sync here.
Thoughts on this approach to help support cohere messages?
ivanleomk
reviewed
Sep 24, 2024
| @@ -30,6 +30,25 @@ def test_parse_user_sync(client, mode): | |||
| assert resp.age == 27 | |||
|
|
|||
|
|
|||
| @pytest.mark.parametrize("mode", modes) | |||
| def test_parse_user_sync_jinja(client, mode): | |||
There was a problem hiding this comment.
Added some jinja tests to validate that the parsing is working too
ivanleomk
approved these changes
Sep 25, 2024
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
Integrates Jinja templating, adds
contextparameter, deprecatesvalidation_context, and adds new tests.client.pyandpatch.pyfor prompt formatting. Addscontextparameter to methods inclient.py. Deprecatesvalidation_context.handle_contextandhandle_templatinginpatch.py. Updatesfrom_responseinfunction_calls.pyto usecontext.test_validation_context.pyfor context handling andtest_formatting.pyfor Jinja templating.This description was created by
for d889b15. It will automatically update as commits are pushed.