Implement start_pos per query for batch interface
#344
Conversation
jan-wassenberg
left a comment
jan-wassenberg
left a comment
There was a problem hiding this comment.
Very nice, I like that you also used a typedef. Looks good.
Good catch that prompt() is unused. We do have some internal code that uses it, though. Would you mind reverting that change? We can then redo the removal once it's no longer referenced.
gemma/gemma-inl.h
Outdated
| size_t& min_prompt_size, | ||
| size_t& max_prompt_size) { | ||
| // Count the minimum/maximum size of prompts for interleave queries. | ||
| static void InterleaveQueries(const MultiplePromptsTokens& queries, |
There was a problem hiding this comment.
Please keep the std::vector return for now until we can remove the last reference to it.
There was a problem hiding this comment.
OK, let me drop that commit. :)
ufownl
force-pushed
the
feature/start_pos_per_query
branch
from
f3ef9f6 to
7823829
Compare
|
I've dropped that commit. :) |
ufownl
force-pushed
the
feature/start_pos_per_query
branch
from
7823829 to
e02b1e9
Compare
jan-wassenberg
added
the
copybara-import
|
hm, we seem to have change markers (<<<<<) inside the argument list of TransformerLayer in gemma-inl.h, not in the pull request but in the code that copybara imported. Maybe the force-push clobbered history? Can you rebase and double-check the diff is clean? Worst case, create a new pull request? |
|
OK, let me have a see. |
|
I saw the newest commit of the dev branch was just added some test cases about gemma2 and small fixes, there shouldn't be any conflict in gemma-inl.h? I have rebased my local branch to the newest dev branch, and there was no conflict. Is the latest code not in the dev branch? |
|
I agree there isn't actually a conflict, I think the issue is just that the branch is out of date. A |
ufownl
force-pushed
the
feature/start_pos_per_query
branch
from
e02b1e9 to
5c98189
Compare
|
I've updated this branch. See if the problem still exists? |
jan-wassenberg
added
copybara-import
|
Thanks, rerunning the import :) |
|
Sorry, still seeing the conflict :( Would you mind re-creating the pull request? |
|
OK, I close this PR first. |
2 participants
Refs #338
I noticed that the original padding code is not used, and the current implementation truncates the prompts to align to the shortest length to avoid accessing out-of-range. So before calling the batch interface, users should insert
<pad>tokens in the front of the prompts to align them to the maximum length.