From 485597e651d38398b9b24fa08c48263509d22300 Mon Sep 17 00:00:00 2001 From: Mick Date: Mon, 16 Mar 2026 16:55:30 +0800 Subject: [PATCH] [diffusion] fix: fix some sampling args passed via cli are omitted (#20630) --- .../configs/sample/sampling_params.py | 28 +++++-- .../test/unit/test_sampling_params.py | 78 +++++++++++++++++++ 2 files changed, 100 insertions(+), 6 deletions(-) diff --git a/python/sglang/multimodal_gen/configs/sample/sampling_params.py b/python/sglang/multimodal_gen/configs/sample/sampling_params.py index bd8256de5..499340237 100644 --- a/python/sglang/multimodal_gen/configs/sample/sampling_params.py +++ b/python/sglang/multimodal_gen/configs/sample/sampling_params.py @@ -575,7 +575,9 @@ class SamplingParams: user_kwargs.pop("diffusers_kwargs", None) user_sampling_params = SamplingParams(*args, **user_kwargs) # TODO: refactor - sampling_params._merge_with_user_params(user_sampling_params) + sampling_params._merge_with_user_params( + user_sampling_params, explicit_fields=set(user_kwargs.keys()) + ) sampling_params._adjust(server_args) sampling_params._validate_with_pipeline_config(server_args.pipeline_config) @@ -641,7 +643,7 @@ class SamplingParams: parser.add_argument( "--negative-prompt", type=str, - default=SamplingParams.negative_prompt, + default=None, help="Negative text prompt for generation", ) parser.add_argument( @@ -928,16 +930,29 @@ class SamplingParams: sampling_params_fields = {attr.name for attr in dataclasses.fields(cls)} args_attrs = set(vars(args).keys()) attrs = sampling_params_fields & args_attrs - return {attr: getattr(args, attr) for attr in attrs if hasattr(args, attr)} + return { + attr: getattr(args, attr) + for attr in attrs + if hasattr(args, attr) and getattr(args, attr) is not None + } def output_file_path(self): if self.output_path is None: return None return os.path.join(self.output_path, self.output_file_name) - def _merge_with_user_params(self, user_params: "SamplingParams"): + def _merge_with_user_params( + self, + user_params: "SamplingParams", + explicit_fields: set[str] | None = None, + ): """ Merges parameters from a user-provided SamplingParams object. + + Args: + explicit_fields: field names explicitly set by the user (e.g. from + CLI kwargs). These are always treated as user-modified even when + their value matches the base-class default. """ if user_params is None: return @@ -951,8 +966,9 @@ class SamplingParams: user_value = getattr(user_params, field_name) default_class_value = getattr(SamplingParams, field_name) - # A field is considered user-modified if its value is different from the default - is_user_modified = user_value != default_class_value + is_user_modified = user_value != default_class_value or ( + explicit_fields is not None and field_name in explicit_fields + ) is_protected_field = field_name in predefined_fields if is_user_modified and ( allow_override_protected or not is_protected_field diff --git a/python/sglang/multimodal_gen/test/unit/test_sampling_params.py b/python/sglang/multimodal_gen/test/unit/test_sampling_params.py index 4003fef5c..90a805696 100644 --- a/python/sglang/multimodal_gen/test/unit/test_sampling_params.py +++ b/python/sglang/multimodal_gen/test/unit/test_sampling_params.py @@ -1,3 +1,4 @@ +import argparse import math import unittest @@ -67,5 +68,82 @@ class TestSamplingParamsSubclass(unittest.TestCase): DiffusersGenericSamplingParams(num_frames=0) +class TestNegativePromptMerge(unittest.TestCase): + """Regression tests for negative_prompt not being passed through CLI""" + + def test_get_cli_args_filters_none(self): + ns = argparse.Namespace(negative_prompt=None, prompt="hello") + result = SamplingParams.get_cli_args(ns) + self.assertNotIn("negative_prompt", result) + self.assertEqual(result["prompt"], "hello") + + def test_get_cli_args_keeps_explicit_value(self): + ns = argparse.Namespace(negative_prompt="ugly, blurry") + result = SamplingParams.get_cli_args(ns) + self.assertEqual(result["negative_prompt"], "ugly, blurry") + + def test_merge_preserves_subclass_default_when_not_explicit(self): + """Without explicit_fields, value matching base default is not merged, + so the subclass default (empty string) is preserved.""" + target = DiffusersGenericSamplingParams() + self.assertEqual(target.negative_prompt, "") + + user = SamplingParams() + target._merge_with_user_params(user) + self.assertEqual(target.negative_prompt, "") + + def test_merge_applies_different_negative_prompt(self): + target = DiffusersGenericSamplingParams() + user = SamplingParams(negative_prompt="ugly, blurry") + target._merge_with_user_params(user) + self.assertEqual(target.negative_prompt, "ugly, blurry") + + def test_merge_explicit_field_matching_base_default(self): + """Even when the user value matches the base-class default, it should + still be applied if listed in explicit_fields.""" + base_default = SamplingParams.negative_prompt + target = DiffusersGenericSamplingParams() + self.assertEqual(target.negative_prompt, "") + + user = SamplingParams(negative_prompt=base_default) + target._merge_with_user_params(user, explicit_fields={"negative_prompt"}) + self.assertEqual(target.negative_prompt, base_default) + + def test_cli_roundtrip_no_negative_prompt(self): + """Simulate CLI without --negative-prompt: subclass default is kept.""" + ns = argparse.Namespace(negative_prompt=None, width=512, height=512) + kwargs = SamplingParams.get_cli_args(ns) + self.assertNotIn("negative_prompt", kwargs) + + user = SamplingParams(**kwargs) + target = DiffusersGenericSamplingParams() + target._merge_with_user_params(user, explicit_fields=set(kwargs.keys())) + self.assertEqual(target.negative_prompt, "") + + def test_cli_roundtrip_with_negative_prompt(self): + """Simulate CLI with --negative-prompt: user value is applied.""" + user_neg = "bad quality, watermark" + ns = argparse.Namespace(negative_prompt=user_neg, width=512, height=512) + kwargs = SamplingParams.get_cli_args(ns) + user = SamplingParams(**kwargs) + + target = DiffusersGenericSamplingParams() + target._merge_with_user_params(user, explicit_fields=set(kwargs.keys())) + self.assertEqual(target.negative_prompt, user_neg) + + def test_cli_roundtrip_with_base_default_negative_prompt(self): + """Simulate CLI where --negative-prompt value matches the base default: + user value should still be applied (not dropped).""" + base_default = SamplingParams.negative_prompt + ns = argparse.Namespace(negative_prompt=base_default, width=512, height=512) + kwargs = SamplingParams.get_cli_args(ns) + self.assertIn("negative_prompt", kwargs) + + user = SamplingParams(**kwargs) + target = DiffusersGenericSamplingParams() + target._merge_with_user_params(user, explicit_fields=set(kwargs.keys())) + self.assertEqual(target.negative_prompt, base_default) + + if __name__ == "__main__": unittest.main()