Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add EventsMixin #984

Merged
merged 6 commits into from
Jul 23, 2024
Merged

Add EventsMixin #984

merged 6 commits into from
Jul 23, 2024

Conversation

collindutter
Copy link
Member

@collindutter collindutter commented Jul 15, 2024 •
edited
Loading

Describe your changes

Issue ticket number and link

Closes #914
Closes #643

@codecov Codecov
Copy link

codecov bot commented Jul 15, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 96.47059% with 3 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
griptape/tasks/base_task.py 0.00% 0 Missing and 2 partials ⚠️
griptape/config/base_structure_config.py 95.23% 0 Missing and 1 partial ⚠️

📢 Thoughts on this report? Let us know!

@collindutter collindutter mentioned this pull request Jul 15, 2024
Closed
@collindutter collindutter force-pushed the feature/events-mixin branch 6 times, most recently from 4a705be to 3622d31 Compare July 17, 2024 23:00
@collindutter collindutter marked this pull request as ready for review July 17, 2024 23:06
@collindutter
Copy link
Member Author

Still need to write up a changelog

dylanholmes
dylanholmes reviewed Jul 17, 2024
View reviewed changes
Copy link
Contributor

@dylanholmes dylanholmes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, I like a lot of the change, just a couple of questions

griptape/mixins/events_mixin.py Outdated Show resolved Hide resolved
griptape/drivers/vector/base_vector_store_driver.py Show resolved Hide resolved
griptape/tasks/prompt_task.py Outdated Show resolved Hide resolved
griptape/utils/stream.py Outdated Show resolved Hide resolved
@collindutter collindutter force-pushed the feature/events-mixin branch from cef72ee to a9ab80d Compare July 18, 2024 20:33
@dylanholmes dylanholmes self-requested a review July 19, 2024 13:50
dylanholmes
dylanholmes reviewed Jul 19, 2024
View reviewed changes
Copy link
Contributor

@dylanholmes dylanholmes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Give me a minute to think about this one. Won't take too long

griptape/utils/stream.py Outdated Show resolved Hide resolved
griptape/tasks/prompt_task.py Outdated Show resolved Hide resolved
@dylanholmes dylanholmes self-requested a review July 19, 2024 14:01
@collindutter collindutter force-pushed the feature/events-mixin branch 3 times, most recently from c5d8ee5 to 0df4872 Compare July 19, 2024 15:15
dylanholmes
dylanholmes approved these changes Jul 22, 2024
View reviewed changes
Copy link
Contributor

@dylanholmes dylanholmes left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels like the right direction, but I just want to make sure we define the expected behavior when mutating structure/structure config/drivers

(Accidentally approved this one)

griptape/tasks/base_task.py Show resolved Hide resolved
griptape/config/base_structure_config.py Outdated Show resolved Hide resolved
griptape/config/base_structure_config.py Outdated Show resolved Hide resolved
griptape/config/base_structure_config.py Outdated Show resolved Hide resolved
griptape/config/base_structure_config.py Show resolved Hide resolved
dylanholmes
dylanholmes previously requested changes Jul 22, 2024
View reviewed changes
Copy link
Contributor

@dylanholmes dylanholmes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(that approval was accidental)

@dylanholmes dylanholmes dismissed their stale review July 22, 2024 13:21

accident

@collindutter collindutter requested a review from dylanholmes July 22, 2024 21:22
@collindutter collindutter force-pushed the feature/events-mixin branch from 42ea678 to 891715c Compare July 22, 2024 21:26
@collindutter collindutter force-pushed the feature/events-mixin branch from 891715c to 38c74b4 Compare July 23, 2024 16:06
dylanholmes
dylanholmes approved these changes Jul 23, 2024
View reviewed changes
Copy link
Contributor

@dylanholmes dylanholmes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So much better!

@collindutter collindutter merged commit 1f690c6 into dev Jul 23, 2024
13 checks passed
@collindutter collindutter deleted the feature/events-mixin branch July 23, 2024 18:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dylanholmes dylanholmes dylanholmes approved these changes

@emjay07 emjay07 Awaiting requested review from emjay07

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Prompt Drivers Crash When Streaming Without A Structure
2 participants
@collindutter @dylanholmes