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

remove broken stop word detection, rely on server side implementation #63

Merged
merged 2 commits into from
Jul 9, 2024

Conversation

mattf
Copy link
Collaborator

@mattf mattf commented Jul 2, 2024

remove the client side stop word detection and rely on server side implementation

two issues with client side implementation -

  1. it was not being called in the v0.1 series (last worked in v0.0.20), effectively didn't work
  2. it would return part of the stop word (find(...) + 1)

this simplifies the output post-processing, which moves us toward using standard post-processing tools.

@mattf mattf requested review from dglogo and raspawar July 2, 2024 12:09
@mattf mattf self-assigned this Jul 2, 2024
Copy link
Collaborator

@raspawar raspawar left a comment

Choose a reason for hiding this comment

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

I see some logic involving is_stop in _aggregate_msgs() which can be removed as we are not making it part of postprocessing

Copy link
Collaborator

@dglogo dglogo left a comment

Choose a reason for hiding this comment

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

lgtm, thanks

@mattf mattf force-pushed the mattf/remove-stop-word-detection branch from e17aa91 to 9696e9d Compare July 5, 2024 15:21
@mattf
Copy link
Collaborator Author

mattf commented Jul 5, 2024

I see some logic involving is_stop in _aggregate_msgs() which can be removed as we are not making it part of postprocessing

@raspawar ptal.

i removed extraneous stop params. the is_stopped is used in streaming.

@mattf mattf force-pushed the mattf/remove-stop-word-detection branch from 9696e9d to f0c0b48 Compare July 5, 2024 15:24
@mattf mattf requested a review from raspawar July 9, 2024 08:27
msg, is_stopped = self._aggregate_msgs(msg_list)
msg, is_stopped = self._early_stop_msg(msg, is_stopped, stop=stop)
return msg, is_stopped
return self._aggregate_msgs(self._process_response(response))
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this logic needed in _aggredate_msgs?
is_stopped = msg.get("finish_reason", "") == "stop"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it's used in stream processing

Copy link
Collaborator

Choose a reason for hiding this comment

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

ok

@mattf mattf merged commit b04e754 into main Jul 9, 2024
12 checks passed
@raspawar raspawar mentioned this pull request Jul 15, 2024
@mattf mattf deleted the mattf/remove-stop-word-detection branch July 22, 2024 19:18
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.

3 participants