Skip to content

Conversation

@wreed4
Copy link
Contributor

@wreed4 wreed4 commented Jun 15, 2025

This change fixes the logic in the print handler allowing thinking events to print within the same panel. Without this logic, Marvin would make a new panel if thinking events were even a millisecond apart, which happens quite frequently 😉.

Copilot AI review requested due to automatic review settings June 15, 2025 04:37
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR reduces the frequency of agent prints during thinking by coarsening the timestamp resolution used in the stable event ID.

  • Changed timestamp granularity from full ISO format to hour-level only
  • Affects how event IDs are generated to prevent repeated prints within the same hour
Comments suppressed due to low confidence (2)

src/marvin/handlers/print_handler.py:233

  • Add a comment explaining why the timestamp is truncated to the hour (e.g., to batch prints within the same hour), so future readers understand the rationale.
msg_time = event.timestamp.isoformat(timespec='hour')

src/marvin/handlers/print_handler.py:233

  • Add a unit test to verify that events within the same hour produce the same event_id and that events in different hours produce distinct IDs.
msg_time = event.timestamp.isoformat(timespec='hour')

Copy link
Collaborator

@zzstoatzz zzstoatzz left a comment

Choose a reason for hiding this comment

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

thanks! @wreed4

@zzstoatzz
Copy link
Collaborator

actually @wreed4 can you share what model / setup you have here?

@wreed4
Copy link
Contributor Author

wreed4 commented Jun 23, 2025

I'm using just the regular sonnet models. They're coming through a litellm, but as far as I can tell that's not the issue. the issue only was that that code would make a new panel every time that "stable id" changes. And the stable id was "actor name" + "timestamp". but the time would be different on the millisecond, so the code would just.. essentially always make new panels for every event. because every token of "thought" back from the model is more than a millisecond away from the last one. maybe it's streaming them now and in the past it didn't so the whole thought would come back at once and this bug wasn't visible? 🤷

@zzstoatzz zzstoatzz closed this Jul 14, 2025
@wreed4
Copy link
Contributor Author

wreed4 commented Jul 14, 2025

@zzstoatzz did this get handled in a different change?

@zzstoatzz zzstoatzz reopened this Jul 14, 2025
@zzstoatzz
Copy link
Collaborator

i don't really understand why this change is necessary as i haven't reproduced but i should have explained that when closing. im fine with merging it and testing it out in main for a bit before releasing

@zzstoatzz zzstoatzz merged commit 1034eef into PrefectHQ:main Jul 14, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants