Add audio to multimodal runner #13662
Conversation
[ghstack-poisoned]
jackzhxng
requested review from
larryliu0820,
mergennachin and
swolchok
as code owners
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/13662
Note: Links to docs will display an error until the docs builds have been completed. ❌ 1 New FailureAs of commit fb87bbf with merge base 99e6349 ( NEW FAILURE - The following job has failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
meta-cla
bot
added
the
CLA Signed
[ghstack-poisoned]
extension/llm/runner/constants.h
Outdated
| inline constexpr auto kTokenEmbeddingMethod = "token_embeddings"; | ||
| inline constexpr auto kTextModelMethod = "decoder"; |
There was a problem hiding this comment.
Make it backwards compatible...
Keep token_embedding, not token_embeddings
And keep text_model instead of decoder
There was a problem hiding this comment.
There's nothing that needs to be kept backwards compatible, this isn't used anywhere atm. I'd like to match this to Optimum
| // 2. Run decoder model for prefill. | ||
| // `cache_position` goes from start_pos to start_pos + encoder_output.size(1). | ||
| // e.g. if start_pos = 2 and encoder_output.size(1) = 5, | ||
| // cache_position_tensor should be [2, 3, 4, 5, 6]. | ||
| int64_t seq_len = encoder_output.toTensor().size(1); | ||
| std::vector<int64_t> cache_positions(seq_len); | ||
| for (int64_t i = 0; i < seq_len; ++i) { | ||
| cache_positions[i] = start_pos + i; | ||
| } | ||
| auto cache_position_tensor = ::executorch::extension::from_blob( | ||
| cache_positions.data(), {seq_len}, executorch::aten::ScalarType::Long); | ||
| auto prefill_result = module_->execute( | ||
| kTextModelMethod, {cache_position_tensor, encoder_output}); | ||
| if (prefill_result.error() != ::executorch::runtime::Error::Ok) { | ||
| return ::executorch::runtime::Error::Internal; | ||
| } | ||
| auto prefill_outputs = prefill_result.get(); | ||
| auto outputs_res = prefill_outputs[0].toTensor(); |
There was a problem hiding this comment.
cc @kimishpatel @JacobSzwejbka is this correct way to manage KV Cache indices?
| return prefill_result.error(); | ||
| } | ||
| auto prefill_outputs = prefill_result.get(); | ||
| auto outputs_res = prefill_outputs[0].toTensor(); |
There was a problem hiding this comment.
validate if outputs_res.numel() == 0
There was a problem hiding this comment.
Why? I think adding so many validations for extremely unlikely outcomes makes things too long and hard to read. I think letting this one naturally error out below and returning that error directly is good enough
[ghstack-poisoned]
|
consider adding unit tests similar to what mengwei added in vision-text version |
|
Also, CI failures look legit |
|
Yeah I'm fixing the CI issue |
[ghstack-poisoned]
| Audio& get_audio() & { | ||
| return std::get<Audio>(data_); | ||
| } |
There was a problem hiding this comment.
is this needed? like do we ever return mutable Audio?
There was a problem hiding this comment.
Yeah I was thinking the same too, this is following already established pattern, I'm thinking of getting rid of all of these get_ variants later
| // 2. Run decoder model for prefill. | ||
| // `cache_position` goes from start_pos to start_pos + encoder_output.size(1). | ||
| // e.g. if start_pos = 2 and encoder_output.size(1) = 5, | ||
| // cache_position_tensor should be [2, 3, 4, 5, 6]. | ||
| int64_t seq_len = encoder_output.toTensor().size(1); |
There was a problem hiding this comment.
Didnt vision based multimodal need exactly the same thing?
There was a problem hiding this comment.
Vision also takes this path
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
jackzhxng
temporarily deployed
to
cherry-pick-bot
GitHub Actions
Inactive
(Messed up the merge for the original stack, this is reland. Original PR with comments here - #13662) Differential Revision: [D81498750](https://our.internmc.facebook.com/intern/diff/D81498750) [ghstack-poisoned]
(Messed up the merge for the original stack, this is reland. Original PR with comments here - #13662) Differential Revision: [D81498750](https://our.internmc.facebook.com/intern/diff/D81498750) [ghstack-poisoned]
(Messed up the merge for the original stack, this is reland. Original PR with comments here - #13662) Differential Revision: [D81498750](https://our.internmc.facebook.com/intern/diff/D81498750) [ghstack-poisoned]
(Messed up the merge for the original stack, this is reland. Original PR with comments here - #13662) Differential Revision: [D81498750](https://our.internmc.facebook.com/intern/diff/D81498750) [ghstack-poisoned]
3 participants
Stack from ghstack (oldest at bottom):