Repository navigation
feat: update encode signature - #2274
Merged
Merged
Conversation
* adds `EncodeHandle` return type to pave the way for async
* takes `impl Into<Inputs>` as an input
* pairs are not supported atm
Signed-off-by: Luc Georges <luc.sydney.georges@gmail.com>
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
ArthurZucker
approved these changes
Aug 4, 2026
Comment on lines
+84
to
+87
| .encode(chunk, true) | ||
| .wait() | ||
| .unwrap() | ||
| .first() |
Collaborator
There was a problem hiding this comment.
we could have a _encode vs encode as you said or something that does this for us when no using any async?
Collaborator
There was a problem hiding this comment.
which is what happens for sequential input unbatch AFAIK
Member
Author
There was a problem hiding this comment.
imo we should keep a consistent API for both, this is sync but allows us to build future/async on top
ArthurZucker
added a commit
that referenced
this pull request
Aug 5, 2026
Conflict resolutions: - models/bpe/model.rs (modify/delete): kept the deletion. This branch split model.rs into bpe_model/bpe_scratch/merge_* and moved its tests to tests.rs. The only thing the base added to it was the reused_scratch_matches_fresh test, which tests.rs already carries verbatim, so nothing is lost. - utils/parallelism.rs: took the base version. The fork-safe pool (#2268) is deliberate upstream work; this branch's "cleanup" commit had dropped it as apparently dead code. Restoring it also means restoring libc, which the cleanup removed from tk-encode/Cargo.toml and pthread_atfork needs. Semantic fixups the textual merge could not do: - pipeline.rs a_reused_scratch_keeps_its_grown_buffers asserted on BpeScratch::word, which no longer exists. The buffer that now grows with input length is the merge entry arena (convert_queue reserves one entry per byte); symbols only holds the final merged ids, so it stays at its initial 64 and the assertion has to target merge.entries. Dropped Word::capacity, added by the base only for the old form of this assertion. - tests/bpe_pipeline_oracle.rs used the pre-#2274 encode signature. encode now returns EncodeHandle, so wait()/first() as in tests/pipeline_oracle.rs. Verified: 312 lib tests, 4 bpe_pipeline_oracle, 20 doctests pass. clippy --workspace --all-targets --all-features is byte-identical to the base branch (both fail only on the 12 pre-existing dead-code lints for the not-yet-wired pool).
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
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.
EncodeHandlereturn type to pave the way for asyncimpl Into<Inputs>as an input