Skip to content

Commit 79b820b

Browse files
fix: address all Copilot review comments in test_dream_params.py
- Add MIN_STEPS, MAX_STEPS, MIN_GUIDANCE_SCALE, MAX_GUIDANCE_SCALE constants to bot.py as single source of truth - Import validation constants from bot.py instead of redefining - Fix unused variables by adding verification assertions - Fix priority logic: change 'or' to 'is not None' checks - Fix unreachable statements in test_override_priority_model_override_over_config - Add test cases for Qwen model_overrides behavior (true_cfg_scale priority) - Fix user_guidance variable usage in Z-Image test All 18 tests pass, lint checks pass.
1 parent d2d2d5f commit 79b820b

File tree

2 files changed

+78
-9
lines changed

2 files changed

+78
-9
lines changed

src/oneiro/bot.py

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,14 @@
3434
MIN_LORA_WEIGHT = -2.0
3535
MAX_LORA_WEIGHT = 2.0
3636

37+
# Steps validation limits (for /dream and /model commands)
38+
MIN_STEPS = 1
39+
MAX_STEPS = 100
40+
41+
# Guidance scale validation limits (for /dream and /model commands)
42+
MIN_GUIDANCE_SCALE = 0.0
43+
MAX_GUIDANCE_SCALE = 15.0
44+
3745

3846
def validate_lora_weight(weight: float, lora_name: str) -> None:
3947
"""Validate that a LoRA weight is within acceptable bounds.

tests/test_dream_params.py

Lines changed: 70 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -4,11 +4,12 @@
44
and setting model defaults via /model command.
55
"""
66

7-
# Constants for validation limits (should match bot.py)
8-
MIN_STEPS = 1
9-
MAX_STEPS = 100
10-
MIN_GUIDANCE_SCALE = 0.0
11-
MAX_GUIDANCE_SCALE = 15.0
7+
from oneiro.bot import (
8+
MAX_GUIDANCE_SCALE,
9+
MAX_STEPS,
10+
MIN_GUIDANCE_SCALE,
11+
MIN_STEPS,
12+
)
1213

1314

1415
class TestDreamParamValidation:
@@ -50,6 +51,8 @@ def test_steps_user_override(self):
5051

5152
actual_steps = user_steps if user_steps is not None else model_config_steps
5253
assert actual_steps == 50
54+
# Verify user value takes precedence over model default
55+
assert actual_steps != model_config_steps
5356

5457
def test_guidance_scale_none_uses_model_default(self):
5558
"""When guidance_scale is None, model config default should be used."""
@@ -66,6 +69,8 @@ def test_guidance_scale_user_override(self):
6669

6770
actual_guidance = user_guidance if user_guidance is not None else model_config_guidance
6871
assert actual_guidance == 4.5
72+
# Verify user value takes precedence over model default
73+
assert actual_guidance != model_config_guidance
6974

7075
def test_guidance_scale_zero_is_valid_override(self):
7176
"""Guidance scale of 0.0 should be treated as a valid override, not None."""
@@ -75,6 +80,8 @@ def test_guidance_scale_zero_is_valid_override(self):
7580
# Use 'is not None' check, not truthiness
7681
actual_guidance = user_guidance if user_guidance is not None else model_config_guidance
7782
assert actual_guidance == 0.0
83+
# Verify that 0.0 overrides the model default, not falls back to it
84+
assert actual_guidance != model_config_guidance
7885

7986
def test_steps_zero_is_invalid(self):
8087
"""Steps of 0 should be outside valid range."""
@@ -119,9 +126,18 @@ def test_override_priority_user_over_model_override(self):
119126
model_override_steps = 30
120127
model_config_steps = 20
121128

122-
# Priority: user > model_override > model_config
123-
actual_steps = user_steps or model_override_steps or model_config_steps
129+
# Priority: user > model_override > model_config (using 'is not None' checks)
130+
if user_steps is not None:
131+
actual_steps = user_steps
132+
elif model_override_steps is not None:
133+
actual_steps = model_override_steps
134+
else:
135+
actual_steps = model_config_steps
136+
124137
assert actual_steps == 40
138+
# Verify priority chain: user wins over both
139+
assert actual_steps != model_override_steps
140+
assert actual_steps != model_config_steps
125141

126142
def test_override_priority_model_override_over_config(self):
127143
"""Model override should take priority over base config default."""
@@ -138,6 +154,8 @@ def test_override_priority_model_override_over_config(self):
138154
actual_steps = model_config_steps
139155

140156
assert actual_steps == 30
157+
# Verify model_override wins over model_config
158+
assert actual_steps != model_config_steps
141159

142160

143161
class TestQwenGuidanceScaleMapping:
@@ -158,6 +176,46 @@ def test_qwen_true_cfg_scale_mapping(self):
158176

159177
assert guidance_scale == 1.3
160178

179+
def test_qwen_true_cfg_scale_with_model_override(self):
180+
"""Model override for guidance_scale should take priority over true_cfg_scale."""
181+
model_config = {
182+
"type": "qwen",
183+
"guidance_scale": 4.0,
184+
"true_cfg_scale": 1.3, # Qwen-specific default
185+
}
186+
model_overrides = {"guidance_scale": 2.5} # User set via /model command
187+
188+
# Simulate the logic from bot.py (lines 430-448):
189+
# 1. Start with model_config defaults
190+
model_guidance = model_config.get("guidance_scale", 0.0)
191+
# 2. Apply Qwen's true_cfg_scale if present
192+
if model_config.get("true_cfg_scale"):
193+
model_guidance = model_config.get("true_cfg_scale", 4.0)
194+
# 3. model_overrides takes priority if set
195+
if "guidance_scale" in model_overrides:
196+
model_guidance = model_overrides["guidance_scale"]
197+
198+
# Model override should win
199+
assert model_guidance == 2.5
200+
201+
def test_qwen_true_cfg_scale_no_override(self):
202+
"""Without model_override, Qwen should use true_cfg_scale."""
203+
model_config = {
204+
"type": "qwen",
205+
"guidance_scale": 4.0,
206+
"true_cfg_scale": 1.3,
207+
}
208+
model_overrides: dict = {} # No overrides set
209+
210+
model_guidance = model_config.get("guidance_scale", 0.0)
211+
if model_config.get("true_cfg_scale"):
212+
model_guidance = model_config.get("true_cfg_scale", 4.0)
213+
if "guidance_scale" in model_overrides:
214+
model_guidance = model_overrides["guidance_scale"]
215+
216+
# true_cfg_scale should be used
217+
assert model_guidance == 1.3
218+
161219

162220
class TestZImageGuidanceScaleWarning:
163221
"""Tests for Z-Image-Turbo guidance_scale handling."""
@@ -166,9 +224,12 @@ def test_zimage_ignores_guidance_scale(self):
166224
"""Z-Image-Turbo should always use 0.0 for guidance_scale internally."""
167225
# Z-Image-Turbo hardcodes guidance_scale=0.0
168226
# Even if user provides a value, pipeline ignores it
169-
user_guidance = 7.0 # noqa: F841 - documents user-provided value that gets ignored
227+
user_guidance = 7.0
170228
zimage_actual_guidance = 0.0 # Always 0.0 for Turbo
171229

172230
# Note: We might want to warn user if they provide non-zero guidance
173-
# This test documents the expected behavior
231+
# This test documents the expected behavior: user can set non-zero guidance,
232+
# but Z-Image-Turbo still uses 0.0 internally.
233+
assert (user_guidance, zimage_actual_guidance) == (7.0, 0.0)
234+
# Verify zimage ignores user guidance
174235
assert zimage_actual_guidance == 0.0

0 commit comments

Comments
 (0)