Add Echo parameter to llama runner, jni+java layer, and demo app #5011
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/5011
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit d081b02 with merge base c83fd2e ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
facebook-github-bot
added
the
CLA Signed
|
linter 😶🌫️ |
|
@larryliu0820 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Summary: Echo flag is added in the runner side via pytorch#5011 by Chirag. Now, we update the app side to leverage the new echo flag, so that we don't display the user prompt in response. Differential Revision: D62250116
cmodi-meta
changed the title
|
@larryliu0820 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
| if (echo) { | ||
| wrapped_callback(ET_UNWRAP(tokenizer_->decode(cur_token, cur_token))); | ||
| } |
There was a problem hiding this comment.
I think we should always call wrapped_callback, otherwise we are missing the first generated token.
There was a problem hiding this comment.
from my testing, this callback would just return a newline. We also tested this and the output works well.
There was a problem hiding this comment.
Hmm I'm saying it doesn't matter what that token is. Semantically it is the first token the model generates, by prefilling the prompt, so we should trigger callback on this one.
There was a problem hiding this comment.
got it! @larryliu0820, I've removed the echo around the first token generation.
| public void onResult(String result) { | ||
| mResultMessage.appendText(result); | ||
| run(); | ||
| if(result.equals("\n\n")) { |
There was a problem hiding this comment.
@Riandy fyi with this check on the app side.
|
@larryliu0820 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Summary: Echo flag is added in the runner side via #5011 by Chirag. Now, we update the app side to leverage the new echo flag, so that we don't display the user prompt in response. Differential Revision: D62250116
cmodi-meta
force-pushed
the
echo-parameter
branch
from
2b7730d to
a4b63e0
Compare
|
@larryliu0820 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
|
@larryliu0820 has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
5 participants
To allow the developer to set whether the response should echo (i.e. include) the input prompt.
We'll default to true since that will maintain the existing flow.