fix: NextStep pipeline class in config fallback path#1960
fix: NextStep pipeline class in config fallback path#1960Joshna-Medisetty wants to merge 2 commits into
Conversation
…json is missing Signed-off-by: Joshna Medisetty <joshna.medisetty@intel.com>
|
cc @xuechendi |
|
any ci test for this bug? |
|
@Joshna-Medisetty , may you help to add a new test file in https://github.com/vllm-project/vllm-omni/tree/main/tests/e2e/offline_inference ? |
Signed-off-by: Joshna Medisetty <joshna.medisetty@intel.com>
|
@hsliuustc0106 |
|
Duplicate #1947 and already (indirectly) fixed by #1908 For the test, all tests of the example code snippets in the documentation will be introduced in #1910 , which include the NextStep use example in Offline Text to Image page. Because according to my own test, seems this is related to that specific text-to-image.py script. If we create So overall this PR is duplicate. Thanks for the contribution though. If you are interested in, we are also calling for model tests (similar to the scripts your have written here, but your version does not follow the official guideline) you can drop your name at #1832 and offer relevant test to NextStep in another PR |
Fix: NextStep pipeline class in fallback path (omni_diffusion.py)
Summary
Fix NextStep-1.1 failing with
ValueError: Unknown model type: nextstep, architectures: ['LlamaForCausalLM']when the fallback path is used (nomodel_index.json). The fix is to always setpipeline_class = "NextStep11Pipeline"whenmodel_type == "nextstep", and remove the incorrectif od_config.model_class_name is None:guard around it.Why the original line was added
In PR #612 ([Model] Add new nextstep_1 (Diffusion) model (only T2I)), the NextStep branch was added with:
The intent was to respect caller override: the CLI (
text_to_image.py) explicitly setsomni_kwargs["model_class_name"] = "NextStep11Pipeline"for NextStep. The author wanted “only set the pipeline class when the caller hasn’t already set it,” so they guarded the assignment withif od_config.model_class_name is None.Why that line is wrong in the current code
The fallback path was later refactored to a single pattern:
pipeline_class.if od_config.model_class_name is None: od_config.model_class_name = pipeline_class.So “don’t overwrite when the caller already set it” is already enforced in that one place. In the refactored flow, the nextstep branch must always set
pipeline_class = "NextStep11Pipeline"so that (a) we don’t raise “Unknown model type” whenpipeline_class is None, and (b) the end block can assign it tood_config.model_class_namewhen the caller did not set it.That is wrong here because:
model_class_name(e.g. from the CLI), we do not setpipeline_class, sopipeline_classstaysNone, and we hitif pipeline_class is None: raise ValueError(...).pipeline_classto whether the caller setmodel_class_name.Why we don’t need the guard anymore
if od_config.model_class_name is None: od_config.model_class_name = "NextStep11Pipeline".pipeline_class = "NextStep11Pipeline". The shared block at the end (if od_config.model_class_name is None: od_config.model_class_name = pipeline_class) already implements “only set when the caller didn’t set it” for all model types, including NextStep. So we do not need theif od_config.model_class_name is None:guard in the nextstep branch; we only need to setpipeline_classunconditionally.Change
if od_config.model_class_name is None: pipeline_class = "NextStep11Pipeline"pipeline_class = "NextStep11Pipeline"Behavior after the fix:
model_class_name→ we don’t overwrite (end block skips assignment).pipeline_class(end block assigns).