-
Notifications
You must be signed in to change notification settings - Fork 143
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
Feature/better events #768
Conversation
9a76b01
to
6d148ee
Compare
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.
Open questions.
- **BREAKING** Bumped `pinecone` from `^2` to `^3`. | ||
- **BREAKING**: Removed `workdir`, `loaders`, `default_loader`, and `save_file_encoding` fields from `FileManager` and added `file_manager_driver`. | ||
- **BREADKING**: Removed `mime_type` field from `ImageArtifact`. `mime_type` is now a property constructed using the Artifact type and `format` field. | ||
- Improved RAG performance in `VectorQueryEngine`. | ||
- Moved [Griptape Docs](https://github.com/griptape-ai/griptape-docs) to this repository. | ||
- The return value of `EventListener.handler` will be passed to the `EventListenerDriver.try_publish_event_payload`'s' `event_payload` parameter. | ||
|
||
### Fixed | ||
- Type hint for parameter `azure_ad_token_provider` on Azure OpenAI drivers to `Optional[Callable[[], str]]`. |
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.
Maybe "CHANGED" type hint? Can we clarify what this fixed?
Ditto for line 45 below. Did we SUPPLY the missing params?
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.
This didn't fix anything, it updated the behavior of the handler
function to use the user's return value in the Driver. Previously this return value was thrown away.
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.
Ditto for line 45 below.
Assuming you're talking about Missing parameters azure_ad_token and azure_ad_token_provider
, this was a fix because the type hints were incorrect.
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.
Yes, it's the missing parameters here, I was advocating for including a verb in the sentence, or at least clarifying what was wrong and what was done to fix it.
@@ -9,12 +9,17 @@ | |||
|
|||
@define | |||
class EventListener: | |||
handler: Callable[[BaseEvent], Optional[dict]] = field(default=Factory(lambda: lambda event: event.to_dict())) |
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.
no action, but good god this is brutal to parse
99e5d94
to
dde96fc
Compare
0f6514e
to
097d4e0
Compare
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.
One comment
- **BREAKING** Bumped `pinecone` from `^2` to `^3`. | ||
- **BREAKING**: Removed `workdir`, `loaders`, `default_loader`, and `save_file_encoding` fields from `FileManager` and added `file_manager_driver`. | ||
- **BREADKING**: Removed `mime_type` field from `ImageArtifact`. `mime_type` is now a property constructed using the Artifact type and `format` field. | ||
- Improved RAG performance in `VectorQueryEngine`. | ||
- Moved [Griptape Docs](https://github.com/griptape-ai/griptape-docs) to this repository. | ||
- The return value of `EventListener.handler` will be passed to the `EventListenerDriver.try_publish_event_payload`'s' `event_payload` parameter. | ||
|
||
### Fixed | ||
- Type hint for parameter `azure_ad_token_provider` on Azure OpenAI drivers to `Optional[Callable[[], str]]`. |
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.
Yes, it's the missing parameters here, I was advocating for including a verb in the sentence, or at least clarifying what was wrong and what was done to fix it.
@@ -1,6 +1,7 @@ | |||
## Overview | |||
|
|||
You can use [EventListener](../../reference/griptape/events/event_listener.md)s to listen for events during a Structure's execution. | |||
See [Event Listener Drivers](../drivers/event-listener-drivers.md) for forwarding events to external services. |
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.
for examples of? guidance on?
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.
Nice work!
Overall, I really like the decision to add back EventListener.handler
. It avoids a breaking change, keeps the local usage simple, and at the same time enables clients to customize events sent to the drivers. So cool.
Requesting changes for minor typos; everything else is optional and I'll leave to your discretion.
3b2d7d8
to
c44366c
Compare
c44366c
to
6391b61
Compare
EventListener.handler
EventListener.handler
will be passed to theEventListenerDriver.try_publish_event_payload
's'event_payload
parameter.LocalEventListenerDriver
https://griptape--768.org.readthedocs.build/768/