fix(hi3): carry per-sample cond_vit metadata and the full batch through the chat template - #300
Draft
heguangxin wants to merge 3 commits into
Draft
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
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
Three fixes to how HunyuanImage-3 conditioning survives a batch.
types/conditions/image.pyspatial_shapesbecomes CONCAT; annotation corrected toList[torch.Tensor]modes/it2i.pysamples_per_prompt <= 2guard; gatetokenizer_outputmodes/t2i.py,modes/t2ti.pytokenizer_outputtext_embed.pyexamples/unified_model/hi3_it2i.yaml1.
cond_vit.spatial_shapeswas declared batch-shared but is per-sample. Its own docstring onmaincalls it "a per-sample list", yet it is the oneshared_fieldamong three parallel lists —embedsandattn_maskare CONCAT. So it stops tracking the batch:Batch.concatreturns SHARED fields asvalues[0], andselect/slicepass them through whole.instantiate_vit_image_tokensthen walksembedsand indexesspatial_shapes[i], putting every entry past the retained copy out of range. That last step is read from the source, not observed.hi3_it2i.yamlalready recorded the fix as pending — "(the per-sample-tensors follow-up removes this)". This is it. The annotation change is a correction rather than a behaviour change: the only producer,vit_encode.py, already emitsList[torch.Tensor].The guard keys on
samples_per_prompt, but that is not the boundary. The shipped recipe's own arithmetic —num_devices: 8,batch_size: 8, its comment8x2=16 -> 2 images/GPU,forward_batch_size: 1— gives two parts to merge per rank, so the desync applies at G=2 as much as above it.2. The upstream batched chat template only tokenizes sample 0.
apply_chat_templatealways delegates with a hard-codedbatchify=True, and that branch callsbatch_gen_infer(prompt_list=[[]], infer_fn_kwargs_list=[... one per sample ...]).zippairs them, so the loop runs once regardless of B and samples 1..B-1 never reach the tokenizer — silently, and only above B=1. That the two lists should be parallel is the callee's own contract: its default is[{} for _ in prompt_list]. A context manager replicatesprompt_listto match, scoped to that one call and restored infinally. This is the single point where the repo enters the upstream tokenizer, so one shim covers all five modes.3.
tokenizer_outputis no longer transported when nothing can read it. Its only reader is_update_state, which runs only whendiffuse_kv_cacheis on, so the three diffusion-stage sites now attach it only in that case. The AR sites are out of scope: that path genuinely readsreal_posfrom the object.Related Issue
N/A.
Test Plan
pre-commit run --files <the six changed files>— all hooks pass, no hook-applied edits.Part.concatonmainleavesembedsat length 2 against aspatial_shapesof length 1; on this branch all three lists stay at 2. This measures the desync, not the raise.spatial_shapeschange with the guard already deleted: one run at FBS=1 reached rollout 63/500, one at FBS=2 reached 50/500, neither logging aTraceback, an OOM or aCannot concat. That tree is based one1a814f, so it is not this diff.tests/tree since test: remove tests directory #267. Say the word if you would like a transport-level unit test.Compatibility / Risk
No config, recipe, checkpoint or public API change.
spatial_shapesis produced only by HI3'svit_encode.py, one entry per sample;wan21andhunyuan_video15leave the fieldNone, which behaves identically under either kind.attn_maskis mis-annotated the same way but is left alone — it has non-HI3 producers that really do pass a tensor. Atforward_batch_size: 1the §2 shim's rewrite condition never fires, so that path is byte-identical.One contract does tighten: a
spatial_shapeswhose length does not match the batch used to be kept silently as a shared value and now fails the merge. Nothing in-tree produces one.Reviewer Notes
Cannot concat valuesinside the dp>1 merge, which cannot have come fromspatial_shapes:Batch.concatreturns SHARED fields asvalues[0], with no comparison and no error path. It may have been keyed to something we have not identified, so the case for removing it is the G=6 runs above rather than an analysis.diffuse_kv_cache: truewith B>1. With the cache on,tokenizer_outputstill rides the transport with no per-sample meaning. Left alone here — the two candidates are a wrapper class absent frommain, or aB > 1guard that would also reject single-rank cache-on batching.hi3_it2i.yaml, the only shipped recipe that reaches these files, sets itfalse.Checklist