-
-
Notifications
You must be signed in to change notification settings - Fork 729
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: implement hooks #1065
feat: implement hooks #1065
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.
❌ Changes requested. Reviewed everything up to b95235b in 45 seconds
More details
- Looked at
981
lines of code in7
files - Skipped
0
files when reviewing. - Skipped posting
8
drafted comments based on config settings.
1. instructor/hooks.py:1
- Draft comment:
Typo in PR title: 'implimennt' should be 'implement'. - Reason this comment was not posted:
Confidence changes required:10%
The PR introduces a new Hooks system, but there is a minor typo in the PR title.
2. instructor/hooks.py:109
- Draft comment:
Consider using thelogging
module instead ofwarnings.warn
for better control over logging levels and consistency with other logging practices. - Reason this comment was not posted:
Confidence changes required:30%
TheHooks
class ininstructor/hooks.py
useswarnings.warn
to notify about exceptions in handlers. This is a good practice for debugging, but it might be better to log these warnings using thelogging
module for consistency and better control over logging levels.
3. instructor/hooks.py:118
- Draft comment:
Consider using thelogging
module instead ofwarnings.warn
for better control over logging levels and consistency with other logging practices. - Reason this comment was not posted:
Confidence changes required:30%
TheHooks
class ininstructor/hooks.py
useswarnings.warn
to notify about exceptions in handlers. This is a good practice for debugging, but it might be better to log these warnings using thelogging
module for consistency and better control over logging levels.
4. instructor/hooks.py:127
- Draft comment:
Consider using thelogging
module instead ofwarnings.warn
for better control over logging levels and consistency with other logging practices. - Reason this comment was not posted:
Confidence changes required:30%
TheHooks
class ininstructor/hooks.py
useswarnings.warn
to notify about exceptions in handlers. This is a good practice for debugging, but it might be better to log these warnings using thelogging
module for consistency and better control over logging levels.
5. instructor/hooks.py:136
- Draft comment:
Consider using thelogging
module instead ofwarnings.warn
for better control over logging levels and consistency with other logging practices. - Reason this comment was not posted:
Confidence changes required:30%
TheHooks
class ininstructor/hooks.py
useswarnings.warn
to notify about exceptions in handlers. This is a good practice for debugging, but it might be better to log these warnings using thelogging
module for consistency and better control over logging levels.
6. instructor/hooks.py:145
- Draft comment:
Consider using thelogging
module instead ofwarnings.warn
for better control over logging levels and consistency with other logging practices. - Reason this comment was not posted:
Confidence changes required:30%
TheHooks
class ininstructor/hooks.py
useswarnings.warn
to notify about exceptions in handlers. This is a good practice for debugging, but it might be better to log these warnings using thelogging
module for consistency and better control over logging levels.
7. instructor/retry.py:235
- Draft comment:
Consider using thelogging
module instead ofwarnings.warn
for better control over logging levels and consistency with other logging practices. - Reason this comment was not posted:
Confidence changes required:30%
TheHooks
class ininstructor/hooks.py
useswarnings.warn
to notify about exceptions in handlers. This is a good practice for debugging, but it might be better to log these warnings using thelogging
module for consistency and better control over logging levels.
8. instructor/retry.py:324
- Draft comment:
Consider using thelogging
module instead ofwarnings.warn
for better control over logging levels and consistency with other logging practices. - Reason this comment was not posted:
Confidence changes required:30%
TheHooks
class ininstructor/hooks.py
useswarnings.warn
to notify about exceptions in handlers. This is a good practice for debugging, but it might be better to log these warnings using thelogging
module for consistency and better control over logging levels.
Workflow ID: wflow_v5gdRFnu5txoyZci
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.
@@ -0,0 +1,378 @@ | |||
# Hooks |
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 new docs/concepts/hooks.md
file should be added to the mkdocs.yml
to ensure it is included in the documentation.
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 4582b71 in 14 seconds
More details
- Looked at
144
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
4
drafted comments based on config settings.
1. instructor/hooks.py:131
- Draft comment:
Usetry-except
to handle the case where the handler is not found in the list to avoidValueError
. - Reason this comment was not posted:
Confidence changes required:80%
Theoff
method should handle the case where the handler is not found in the list to avoid aValueError
.
2. instructor/hooks.py:81
- Draft comment:
Add comments to explain the error handling mechanism when exceptions occur in handlers. This will improve code readability and maintainability. - Reason this comment was not posted:
Confidence changes required:50%
The code is missing comments for complex sections, especially where exceptions are handled and warnings are issued. This would help in understanding the error handling mechanism better.
3. instructor/hooks.py:29
- Draft comment:
Use more descriptive variable names for_handlers
to improve code readability. This applies to other similar instances as well. - Reason this comment was not posted:
Confidence changes required:50%
The code uses a consistent naming pattern for methods, which is good. However, the variable names could be more descriptive in some places.
4. instructor/hooks.py:77
- Draft comment:
Consider refactoring the repeated exception handling logic in emit methods to adhere to the DRY principle. This applies to other emit methods as well. - Reason this comment was not posted:
Confidence changes required:50%
The code should be DRY, and there are repeated patterns in the exception handling across different emit methods.
Workflow ID: wflow_DyIy8rC6BAFsG1bd
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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 f364bce in 26 seconds
More details
- Looked at
206
lines of code in4
files - Skipped
0
files when reviewing. - Skipped posting
4
drafted comments based on config settings.
1. README.md:83
- Draft comment:
The hook event names used here (e.g.,completion:kwargs
,completion:error
) should be consistent with the naming conventions used in theinstructor/hooks.py
file and documentation. Ensure consistency across all examples and documentation. - Reason this comment was not posted:
Confidence changes required:50%
The PR introduces a new Hooks system, but the README and documentation examples use inconsistent naming for the hook events. This could lead to confusion for users trying to implement the hooks.
2. docs/concepts/hooks.md:134
- Draft comment:
Ensure that the hook event names (e.g.,completion:kwargs
,completion:error
) are consistent with those used in theinstructor/hooks.py
file and other documentation. Consistency is key to avoid confusion. - Reason this comment was not posted:
Confidence changes required:50%
The PR introduces a new Hooks system, but the README and documentation examples use inconsistent naming for the hook events. This could lead to confusion for users trying to implement the hooks.
3. docs/index.md:193
- Draft comment:
The hook event names used here (e.g.,completion:kwargs
,completion:error
) should be consistent with the naming conventions used in theinstructor/hooks.py
file and documentation. Ensure consistency across all examples and documentation. - Reason this comment was not posted:
Confidence changes required:50%
The PR introduces a new Hooks system, but the README and documentation examples use inconsistent naming for the hook events. This could lead to confusion for users trying to implement the hooks.
4. mkdocs.yml:169
- Draft comment:
The newdocs/concepts/hooks.md
file is correctly added to themkdocs.yml
file, ensuring it is included in the documentation. - Reason this comment was not posted:
Confidence changes required:0%
The PR introduces a new Hooks system, and the documentation for it is added indocs/concepts/hooks.md
. This new documentation file is correctly added to themkdocs.yml
file under the Concepts section, ensuring it is included in the documentation build.
Workflow ID: wflow_19w1K5jcpDqk3o8c
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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 c5c5642 in 17 seconds
More details
- Looked at
16
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
1
drafted comments based on config settings.
1. docs/concepts/hooks.md:48
- Draft comment:
Ensurehooks.md
is added tomkdocs.yml
for documentation. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_NMwvWQSmJCTG0CFb
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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 a33bb16 in 13 seconds
More details
- Looked at
12
lines of code in1
files - Skipped
0
files when reviewing. - Skipped posting
2
drafted comments based on config settings.
1. pyproject.toml:3
- Draft comment:
The version update to 1.6.0 is appropriate for the new feature addition, following semantic versioning guidelines. - Reason this comment was not posted:
Confidence changes required:0%
The version update in thepyproject.toml
file is consistent with the new feature addition described in the PR. The change from version 1.5.2 to 1.6.0 indicates a minor version update, which is appropriate for adding new functionality without breaking changes.
2. pyproject.toml:3
- Draft comment:
If the library code has changed, ensure that the documentation and tests are updated accordingly. This includes updating any relevantmd
files indocs
and ensuring they are added tomkdocs.yml
. - Reason this comment was not posted:
Confidence changes required:80%
The version update in thepyproject.toml
file indicates a change in the library code, which should be accompanied by updates in documentation and tests.
Workflow ID: wflow_vdUdt37Bwftj9wiM
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
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 d31ae49 in 21 seconds
More details
- Looked at
205
lines of code in3
files - Skipped
0
files when reviewing. - Skipped posting
4
drafted comments based on config settings.
1. instructor/patch.py:20
- Draft comment:
handle_templating
is imported but not used in this file. Consider removing the import statement to clean up the code. - Reason this comment was not posted:
Confidence changes required:10%
The import statement forhandle_templating
ininstructor/patch.py
is unnecessary after the refactor, as the function is no longer used in this file.
2. tests/llm/test_cohere/test_json_schema.py:121
- Draft comment:
The# type: ignore
comment is used here. Ensure this is necessary and not suppressing important type errors. - Reason this comment was not posted:
Confidence changes required:33%
Intests/llm/test_cohere/test_json_schema.py
, the# type: ignore
comment is used to suppress type checking. It's important to ensure that this is necessary and not hiding potential issues.
3. tests/dsl/test_partial.py:79
- Draft comment:
Assertions should include error messages for clarity and debugging purposes. This applies to all assertions in this file. - Reason this comment was not posted:
Confidence changes required:80%
The assertion statements in the test files lack error messages, which is a violation of the rule that assertions should always have an error message.
4. tests/llm/test_cohere/test_json_schema.py:118
- Draft comment:
Assertions should include error messages for clarity and debugging purposes. This applies to all assertions in this file. - Reason this comment was not posted:
Confidence changes required:80%
The assertion statements in the test files lack error messages, which is a violation of the rule that assertions should always have an error message.
Workflow ID: wflow_6dDIwfdJcrsbU1M2
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet
mode, and more.
Add Hooks System to Instructor Library
This PR introduces a new Hooks system to the Instructor library, providing a flexible way to intercept and handle events during the completion and parsing process.
Changes in
instructor/hooks.py
:Hooks
class with the following features:HookName
enum)on
), unregister (off
), and clear hooksChanges in
docs/concepts/hooks.md
:Key Features:
Impact:
This addition will allow users of the Instructor library to:
Important
Introduces a Hooks system in the Instructor library for managing event hooks during the completion process, with integration, documentation, and tests.
Hooks
class ininstructor/hooks.py
for managing event hooks.completion:kwargs
,completion:response
,completion:error
,completion:last_attempt
,parse:error
.on
), unregister (off
), and emit events.hooks
parameter toInstructor
andAsyncInstructor
classes ininstructor/client.py
.retry_sync
andretry_async
ininstructor/retry.py
to emit hooks during retries.docs/concepts/hooks.md
with detailed usage and examples.test_concepts.py
for testing hook examples in documentation.This description was created by
for d31ae49. It will automatically update as commits are pushed.