Clarify ownership of runner components #10338
Open
+26
−4
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
larryliu0820
requested review from
jackzhxng,
iseeyuan,
swolchok and
lucylq
as code owners
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/10338
Note: Links to docs will display an error until the docs builds have been completed. ⏳ 3 Pending, 1 Unrelated FailureAs of commit 8305e49 with merge base cfd1be3 ( BROKEN TRUNK - The following job failed but were present on the merge base:👉 Rebase onto the `viable/strict` branch to avoid these failures
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
facebook-github-bot
added
the
CLA Signed
|
This pull request was exported from Phabricator. Differential Revision: D73399600 |
larryliu0820
added
the
release notes: api
kirklandsign
approved these changes
Apr 21, 2025
facebook-github-bot
pushed a commit
that referenced
this pull request
Apr 21, 2025
Summary: The ownership of these components need some clarification. * `Module` should be solely owned by `TextDecoderRunner` * `TextDecoderRunner` should be shared by the `TextPrefiller` and `TextTokenGenerator`. * `Tokenizer` should be owned by the `Runner` as well as `TextTokenGenerator` Reviewed By: kirklandsign Differential Revision: D73399600
facebook-github-bot
force-pushed
the
export-D73399600
branch
from
406a901 to
d94237a
Compare
|
This pull request was exported from Phabricator. Differential Revision: D73399600 |
facebook-github-bot
pushed a commit
that referenced
this pull request
Apr 21, 2025
Summary: The ownership of these components need some clarification. * `Module` should be solely owned by `TextDecoderRunner` * `TextDecoderRunner` should be shared by the `TextPrefiller` and `TextTokenGenerator`. * `Tokenizer` should be owned by the `Runner` as well as `TextTokenGenerator` Reviewed By: kirklandsign Differential Revision: D73399600
facebook-github-bot
force-pushed
the
export-D73399600
branch
from
d94237a to
ec95d29
Compare
|
This pull request was exported from Phabricator. Differential Revision: D73399600 |
facebook-github-bot
pushed a commit
that referenced
this pull request
Apr 22, 2025
Summary: The ownership of these components need some clarification. * `Module` should be shared by `TextDecoderRunner` and potentially `TextPrefiller` (or `ImagePrefiller` in multimodal runner). * `TextDecoderRunner` should be shared by the `TextPrefiller` and `TextTokenGenerator`. * `Tokenizer` should be owned by the `Runner` as well as `TextTokenGenerator`. Reviewed By: kirklandsign Differential Revision: D73399600
facebook-github-bot
force-pushed
the
export-D73399600
branch
from
ec95d29 to
4a79eaf
Compare
|
This pull request was exported from Phabricator. Differential Revision: D73399600 |
larryliu0820
added a commit
that referenced
this pull request
Apr 22, 2025
Summary: The ownership of these components need some clarification. * `Module` should be shared by `TextDecoderRunner` and potentially `TextPrefiller` (or `ImagePrefiller` in multimodal runner). * `TextDecoderRunner` should be shared by the `TextPrefiller` and `TextTokenGenerator`. * `Tokenizer` should be owned by the `Runner` as well as `TextTokenGenerator`. Reviewed By: kirklandsign Differential Revision: D73399600
larryliu0820
force-pushed
the
export-D73399600
branch
from
4a79eaf to
61c1b63
Compare
facebook-github-bot
pushed a commit
that referenced
this pull request
Apr 22, 2025
Summary: The ownership of these components need some clarification. * `Module` should be shared by `TextDecoderRunner` and potentially `TextPrefiller` (or `ImagePrefiller` in multimodal runner). * `TextDecoderRunner` should be shared by the `TextPrefiller` and `TextTokenGenerator`. * `Tokenizer` should be owned by the `Runner` as well as `TextTokenGenerator`. Reviewed By: kirklandsign Differential Revision: D73399600
facebook-github-bot
force-pushed
the
export-D73399600
branch
from
61c1b63 to
8aa32a8
Compare
|
This pull request was exported from Phabricator. Differential Revision: D73399600 |
1 similar comment
|
This pull request was exported from Phabricator. Differential Revision: D73399600 |
larryliu0820
added a commit
that referenced
this pull request
Apr 22, 2025
Summary: Pull Request resolved: #10338 The ownership of these components need some clarification. * `Module` should be shared by `TextDecoderRunner` and potentially `TextPrefiller` (or `ImagePrefiller` in multimodal runner). * `TextDecoderRunner` should be shared by the `TextPrefiller` and `TextTokenGenerator`. * `Tokenizer` should be owned by the `Runner` as well as `TextTokenGenerator`. Reviewed By: kirklandsign Differential Revision: D73399600
larryliu0820
force-pushed
the
export-D73399600
branch
from
8aa32a8 to
2afe009
Compare
facebook-github-bot
pushed a commit
that referenced
this pull request
Apr 22, 2025
Summary: The ownership of these components need some clarification. * `Module` should be shared by `TextDecoderRunner` and potentially `TextPrefiller` (or `ImagePrefiller` in multimodal runner). * `TextDecoderRunner` should be shared by the `TextPrefiller` and `TextTokenGenerator`. * `Tokenizer` should be owned by the `Runner` as well as `TextTokenGenerator`. Reviewed By: kirklandsign Differential Revision: D73399600
facebook-github-bot
force-pushed
the
export-D73399600
branch
from
2afe009 to
747120f
Compare
larryliu0820
added a commit
that referenced
this pull request
Apr 22, 2025
Summary: The ownership of these components need some clarification. * `Module` should be shared by `TextDecoderRunner` and potentially `TextPrefiller` (or `ImagePrefiller` in multimodal runner). * `TextDecoderRunner` should be shared by the `TextPrefiller` and `TextTokenGenerator`. * `Tokenizer` should be owned by the `Runner` as well as `TextTokenGenerator`. Reviewed By: kirklandsign Differential Revision: D73399600
larryliu0820
force-pushed
the
export-D73399600
branch
from
d0159d6 to
bb4938d
Compare
facebook-github-bot
pushed a commit
that referenced
this pull request
Apr 22, 2025
Summary: The ownership of these components need some clarification. * `Module` should be shared by `TextDecoderRunner` and potentially `TextPrefiller` (or `ImagePrefiller` in multimodal runner). * `TextDecoderRunner` should be shared by the `TextPrefiller` and `TextTokenGenerator`. * `Tokenizer` should be owned by the `Runner` as well as `TextTokenGenerator`. Reviewed By: kirklandsign Differential Revision: D73399600
facebook-github-bot
force-pushed
the
export-D73399600
branch
from
bb4938d to
79b8ac2
Compare
|
This pull request was exported from Phabricator. Differential Revision: D73399600 |
larryliu0820
added a commit
that referenced
this pull request
Apr 22, 2025
Summary: Pull Request resolved: #10338 The ownership of these components need some clarification. * `Module` should be shared by `TextDecoderRunner` and potentially `TextPrefiller` (or `ImagePrefiller` in multimodal runner). * `TextDecoderRunner` should be shared by the `TextPrefiller` and `TextTokenGenerator`. * `Tokenizer` should be owned by the `Runner` as well as `TextTokenGenerator`. Reviewed By: kirklandsign Differential Revision: D73399600
larryliu0820
force-pushed
the
export-D73399600
branch
from
79b8ac2 to
aa4d5fe
Compare
|
This pull request was exported from Phabricator. Differential Revision: D73399600 |
1 similar comment
|
This pull request was exported from Phabricator. Differential Revision: D73399600 |
larryliu0820
added a commit
that referenced
this pull request
Apr 22, 2025
Summary: Pull Request resolved: #10338 The ownership of these components need some clarification. * `Module` should be shared by `TextDecoderRunner` and potentially `TextPrefiller` (or `ImagePrefiller` in multimodal runner). * `TextDecoderRunner` should be shared by the `TextPrefiller` and `TextTokenGenerator`. * `Tokenizer` should be owned by the `Runner` as well as `TextTokenGenerator`. Reviewed By: kirklandsign Differential Revision: D73399600
larryliu0820
force-pushed
the
export-D73399600
branch
from
aa4d5fe to
f5623b1
Compare
facebook-github-bot
pushed a commit
that referenced
this pull request
Apr 22, 2025
Summary: The ownership of these components need some clarification. * `Module` should be shared by `TextDecoderRunner` and potentially `TextPrefiller` (or `ImagePrefiller` in multimodal runner). * `TextDecoderRunner` should be shared by the `TextPrefiller` and `TextTokenGenerator`. * `Tokenizer` should be owned by the `Runner` as well as `TextTokenGenerator`. Reviewed By: kirklandsign Differential Revision: D73399600
facebook-github-bot
force-pushed
the
export-D73399600
branch
from
f5623b1 to
5680dbf
Compare
|
This pull request was exported from Phabricator. Differential Revision: D73399600 |
swolchok
requested changes
Apr 22, 2025
| ::executorch::extension::llm::Stats stats_; | ||
| std::unique_ptr<::executorch::extension::llm::Stats> stats_; |
There was a problem hiding this comment.
why are we moving this to the heap?
There was a problem hiding this comment.
Ah that's because in my next PR TextTokenGenerator needs to share the same stats with Runner, TextTokenGenerator(stats) but also Runner constructor is going to take in a TextTokenGenerator. Runner(text_token_generator)
Therefore the only way I can think of is to:
auto stats = std::make_unique<Stats>();
auto text_token_generator = std::make_unique<TextTokenGenerator>(stats.get());
auto runner = std::make_unique<Runner>(std::move(stats), std::move(text_token_generator));There was a problem hiding this comment.
of course I can put this change into next PR.
There was a problem hiding this comment.
if you want to avoid the heap allocation, you can have the Runner hand a pointer to its Stats object to the TextTokenGenerator, or you can have the runner create the TextTokenGenerator and leave the constructor as-is.
facebook-github-bot
pushed a commit
that referenced
this pull request
Apr 22, 2025
Summary: The ownership of these components need some clarification. * `Module` should be shared by `TextDecoderRunner` and potentially `TextPrefiller` (or `ImagePrefiller` in multimodal runner). * `TextDecoderRunner` should be shared by the `TextPrefiller` and `TextTokenGenerator`. * `Tokenizer` should be owned by the `Runner` as well as `TextTokenGenerator`. Reviewed By: kirklandsign Differential Revision: D73399600
facebook-github-bot
force-pushed
the
export-D73399600
branch
from
5680dbf to
aba2bfe
Compare
|
This pull request was exported from Phabricator. Differential Revision: D73399600 |
larryliu0820
changed the title
facebook-github-bot
pushed a commit
that referenced
this pull request
Apr 22, 2025
Summary: The ownership of these components need some clarification. * `Module` should be shared by `TextDecoderRunner` and potentially `TextPrefiller` (or `ImagePrefiller` in multimodal runner). * `TextDecoderRunner` should be shared by the `TextPrefiller` and `TextTokenGenerator`. * `Tokenizer` should be owned by the `Runner` as well as `TextTokenGenerator`. Reviewed By: kirklandsign Differential Revision: D73399600
larryliu0820
added a commit
that referenced
this pull request
Apr 22, 2025
Summary: The ownership of these components need some clarification. * `Module` should be shared by `TextDecoderRunner` and potentially `TextPrefiller` (or `ImagePrefiller` in multimodal runner). * `TextDecoderRunner` should be shared by the `TextPrefiller` and `TextTokenGenerator`. * `Tokenizer` should be owned by the `Runner` as well as `TextTokenGenerator`. Reviewed By: kirklandsign Differential Revision: D73399600
facebook-github-bot
force-pushed
the
export-D73399600
branch
from
aba2bfe to
0df9ee9
Compare
larryliu0820
force-pushed
the
export-D73399600
branch
from
0df9ee9 to
eb2bd02
Compare
|
This pull request was exported from Phabricator. Differential Revision: D73399600 |
Summary: Pull Request resolved: #10338 The ownership of these components need some clarification. * `Module` should be shared by `TextDecoderRunner` and potentially `TextPrefiller` (or `ImagePrefiller` in multimodal runner). * `TextDecoderRunner` should be shared by the `TextPrefiller` and `TextTokenGenerator`. * `Tokenizer` should be owned by the `Runner` as well as `TextTokenGenerator`. Reviewed By: kirklandsign Differential Revision: D73399600
|
This pull request was exported from Phabricator. Differential Revision: D73399600 |
larryliu0820
force-pushed
the
export-D73399600
branch
from
eb2bd02 to
8305e49
Compare
larryliu0820
added a commit
that referenced
this pull request
Apr 22, 2025
Summary: The ownership of these components need some clarification. * `Module` should be shared by `TextDecoderRunner` and potentially `TextPrefiller` (or `ImagePrefiller` in multimodal runner). * `TextDecoderRunner` should be shared by the `TextPrefiller` and `TextTokenGenerator`. * `Tokenizer` should be owned by the `Runner` as well as `TextTokenGenerator`. Reviewed By: swolchok, kirklandsign Differential Revision: D73399600
larryliu0820
added a commit
that referenced
this pull request
Apr 22, 2025
Summary: Pull Request resolved: #10338 The ownership of these components need some clarification. * `Module` should be shared by `TextDecoderRunner` and potentially `TextPrefiller` (or `ImagePrefiller` in multimodal runner). * `TextDecoderRunner` should be shared by the `TextPrefiller` and `TextTokenGenerator`. * `Tokenizer` should be owned by the `Runner` as well as `TextTokenGenerator`. Differential Revision: D73399600 Reviewed By: kirklandsign
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
The ownership of these components need some clarification.
Moduleshould be shared byTextDecoderRunnerand potentiallyTextPrefiller(orImagePrefillerin multimodal runner).TextDecoderRunnershould be shared by theTextPrefillerandTextTokenGenerator.Tokenizershould be owned by theRunneras well asTextTokenGenerator.Differential Revision: D73399600