[diffusion] fix: fix some sampling args passed via cli are omitted (#20630)
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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()
|
||||
|
||||
Reference in New Issue
Block a user