feat(instructor): add middleware and update package #551
Conversation
ellipsis-dev
bot
changed the title
Deploying instructor with
|
| Latest commit: |
20bb86d
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://b0c6733c.instructor.pages.dev |
| Branch Preview URL: | https://messages-middleware.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.
❌ Changes requested.
- Reviewed the entire pull request up to 6e50514
- Looked at
328lines of code in5files - Took 2 minutes and 12 seconds to review
More info
- Skipped
0files when reviewing. - Skipped posting
3additional comments because they didn't meet confidence threshold of85%.
1. examples/middleware/readme.md:1:
- Assessed confidence :
100% - Grade:
0% - Comment:
The newmdfile created indocsis not added tomkdocs.yml. Please add it to ensure the documentation is correctly generated and the new content is accessible. - Reasoning:
The newmdfile created indocsis not added tomkdocs.yml. This is necessary to ensure the documentation is correctly generated and the new content is accessible.
2. instructor/client.py:64:
- Assessed confidence :
100% - Grade:
30% - Comment:
Thewith_middlewaremethod does not check the type of the middleware passed to it. This could lead to runtime errors if the middleware does not have a__call__method. Consider checking the type of the middleware and raising a clear error if it is not a function or an instance ofMessageMiddleware. - Reasoning:
Thewith_middlewaremethod in theclient.pyfile does not check the type of the middleware passed to it. This could lead to runtime errors if the middleware does not have a__call__method. It would be better to check the type of the middleware and raise a clear error if it is not a function or an instance ofMessageMiddleware.
3. instructor/messages_middleware.py:25:
- Assessed confidence :
100% - Grade:
0% - Comment:
Themessages_middlewarefunction returns an instance of_Middlewarewithout any arguments. This could be a problem if the middleware function requires arguments. Consider returning a function that creates a new instance of_Middlewarewith the given arguments when called. - Reasoning:
Themessages_middlewarefunction in themessages_middleware.pyfile returns an instance of_Middlewarewithout any arguments. This could be a problem if the middleware function requires arguments. It would be better to return a function that creates a new instance of_Middlewarewith the given arguments when called.
Workflow ID: wflow_1m3ZAZRPd66jbqmK
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. We'll respond in a few minutes. Learn more here.
There was a problem hiding this comment.
👍 Looks good to me!
- Performed an incremental review on 8a28d7e
- Looked at
90lines of code in5files - Took 1 minute and 16 seconds to review
More info
- Skipped
0files when reviewing. - Skipped posting
1additional comments because they didn't meet confidence threshold of85%.
1. examples/middleware/run.py:8:
- Assessed confidence :
50% - Comment:
There are unnecessary blank lines in your code. Please remove them for better readability. This applies to lines 7, 20, 31, 37, 43, 44. - Reasoning:
The PR introduces middleware to the Instructor library, allowing for the modification of messages sent to the language model before they are processed. The changes include updates to theinstructorpackage and examples demonstrating middleware usage. The changes seem to be well implemented and there are no clear violations of best practices, logical bugs, performance bugs, or security bugs. However, there are some unnecessary blank lines in the code that could be removed for better readability.
Workflow ID: wflow_0w4cJpFUaEZWuvGG
Not what you expected? You can customize the content of the reviews using rules. Learn more here.
There was a problem hiding this comment.
❌ Changes requested.
- Performed an incremental review on 20bb86d
- Looked at
21lines of code in1files - Took 1 minute and 15 seconds to review
More info
- Skipped
0files when reviewing. - Skipped posting
0additional comments because they didn't meet confidence threshold of85%.
Workflow ID: wflow_JFCFwVEsbWMR96tt
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. We'll respond in a few minutes. Learn more here.
| try: | ||
|
|
||
| @instructor.messages_middleware | ||
| def dumb_rag(x): |
There was a problem hiding this comment.
The function dumb_rag is defined twice in this file. If the second definition is meant for testing, consider moving it to a dedicated test function or file. If it's not for testing, please remove the duplicate.
There was a problem hiding this comment.
❌ Changes requested.
- Performed an incremental review on 4560ae9
- Looked at
21lines of code in1files - Took 2 minutes and 31 seconds to review
More info
- Skipped
0files when reviewing. - Skipped posting
0additional comments because they didn't meet confidence threshold of85%.
Workflow ID: wflow_ZBx4GrCuG3suf89X
Want Ellipsis to fix these issues? Tag @ellipsis-dev in a comment. We'll respond in a few minutes. Learn more here.
|
|
||
|
|
||
| def messages_middleware(func: Callable) -> MessageMiddleware: | ||
| import inspect |
There was a problem hiding this comment.
Consider moving the import statement for the inspect module to the top of the file. This aligns with Python's best practices for import statements.
|
Is this still in the works? I'd like to use something like this for caching request / responses from the LLM. I currently have custom code to do this just for openai but wanted to do the same for anthropic. Given the new client refactor, it'd be nice to just be able to do something like: I know there is this for caching: https://jxnl.github.io/instructor/concepts/caching/#2-redis-caching-decorator-for-distributed-systems but in this case, I wanted to log the raw request / responses in a db so I can use them later. |
|
Definitely this is mostly a sketch right now because I don't know if this is something people would actually want |
Summary:
This PR introduces middleware to the Instructor library, updates the
instructorpackage, provides an example of middleware implementation, and makes minor changes in other files.Key points:
MessageMiddlewareandAsyncMessageMiddlewareclasses ininstructor/messages_middleware.py.instructor/client.pyto include amessage_middlewarelist and awith_middlewaremethod.examples/middleware/run.py.instructor/__init__.pyandtests/llm/test_openai/test_validators.py.Generated with ❤️ by ellipsis.dev