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.

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

Handle invalid utf-8 bytes in engine class instead of model class #1094

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

JC1DA
Copy link
Collaborator

@JC1DA JC1DA commented Jan 9, 2025

This PR removes unnecessary invalid utf-8 bytes handling code in Model class as we can better handle it in Engine class.

@codecov-commenter
Copy link

codecov-commenter commented Jan 9, 2025

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 84.61538% with 2 lines in your changes missing coverage. Please review.

Project coverage is 60.27%. Comparing base (aaa5f00) to head (190b3aa).

Files with missing lines Patch % Lines
guidance/models/_model.py 84.61% 2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #1094       +/-   ##
===========================================
+ Coverage   49.79%   60.27%   +10.47%     
===========================================
  Files          71       71               
  Lines        5880     5875        -5     
===========================================
+ Hits         2928     3541      +613     
+ Misses       2952     2334      -618     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@JC1DA
Copy link
Collaborator Author

JC1DA commented Jan 15, 2025

@hudson-ai can you help check this PR?

@JC1DA JC1DA requested a review from hudson-ai January 15, 2025 22:55
@@ -543,13 +543,19 @@ def __call__(
delayed_engine_outputs = []
elif not echo and engine_response.new_bytes:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is it that we only delay bytes if not echo?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

there is another logic to process delayed_bytes if echo is True above.

@hudson-ai
Copy link
Collaborator

Seems reasonable to me, but I'm not sure I have a strong opinion on whether this should be done in the engine vs in the model. Can you help me understand why you like this approach more?

@JC1DA
Copy link
Collaborator Author

JC1DA commented Jan 16, 2025

Seems reasonable to me, but I'm not sure I have a strong opinion on whether this should be done in the engine vs in the model. Can you help me understand why you like this approach more?

First, I think it does not make sense for the engine to generate bytes that the model could not even use, and have to do post-processing on the result.
Second, we need to process delayed_bytes in the engine right now anyway, so why not keeping it there :)

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.

4 participants