Fix three messages that pass extra positional arguments - #14329
Conversation
Two ValueError raises and one logger.info call pass a second positional
argument where a single formatted string was intended.
VaeImageProcessor and the Wan image processor raise the RGB/grayscale
conflict with the last clause as a separate argument, so Python puts each
into args and str(e) renders the tuple repr:
('`do_convert_rgb` and `do_convert_grayscale` can not both be set to
`True`, if you intended ... `do_convert_grayscale = False`.', ' if you
intended to convert the image into grayscale format, please set
`do_convert_rgb = False`')
Half the guidance arrives wrapped in quotes and parentheses, and the
message is the only thing telling the caller which of the two flags to
turn off.
pipeline_spectrogram_diffusion passes the segment index positionally to a
message with no placeholder:
logger.info("Generated segment", i)
logging treats i as a %-format argument, formatting raises inside the
handler, and logging swallows it and prints "--- Logging error ---" with a
traceback instead. The progress line never reaches a handler.
Concatenate the two message halves and add %d to the log format.
Solaris-star
left a comment
There was a problem hiding this comment.
Reviewed — correct fix, well-scoped. Three distinct issues, all handled properly:
-
ValueError with multiple positional args (both
image_processor.pyandwan/image_processor.py):ValueError("a", "b", "c")creates a tuple of args, sostr(e)renders('a', 'b', 'c')instead of a clean concatenated message. Removing the commas between adjacent string literals lets Python implicitly concatenate them into one string. The test assertinglen(ctx.exception.args) == 1andnot message.startswith("(")pins this nicely. -
logger.info("Generated segment", i)in the spectrogram pipeline: the format string had no%dplaceholder, soiwas either silently dropped or would trigger a formatting error depending on the logging backend. Adding%dmakes it proper lazy formatting.
One minor, non-blocking note: the spectrogram pipeline lives under deprecated/, so the logger fix is valid but low-impact. Not a blocker — fixing it everywhere is the right call for consistency.
The test coverage is solid for the ValueError case. Nice catch on the tuple-args rendering issue.
|
Hi @ErenAta16, thanks for the PR! It does not appear to link an issue it fixes. If this PR addresses an existing issue, please add a closing keyword (e.g. |
What does this PR do?
Three messages pass an extra positional argument where a single formatted string was intended.
1 & 2 —
VaeImageProcessor.__init__andWanImageProcessor.__init__The last clause is a second argument, so Python puts each into
argsandstr(e)renders the tuple repr:Half the guidance arrives wrapped in quotes and parentheses. That message is the only thing telling the caller which of the two flags to turn off, so both halves matter.
3 —
pipeline_spectrogram_diffusionloggingtreatsias a%-format argument for a message with no placeholder, formatting raises inside the handler, andloggingcatches it and writes--- Logging error ---plus a traceback to stderr. The per-segment progress line never reaches a handler — and this one is inside the denoising loop, so it fires once per segment.Concatenated the two message halves; added
%dto the log format.An AST sweep over
src/diffusers/for multi-argument stringraises and for logging calls with a placeholder-free first argument plus extra positional args reports these three. The fourth hit,examples/community/llm_grounded_diffusion.py:636, is inexamples/— happy to include it if you want examples in scope, left out to keep this tosrc/.Before submitting
test_rgb_and_grayscale_conflict_message_is_one_stringintests/others/test_image_processor.pyassertslen(e.args) == 1, that both halves of the guidance are present instr(e), and that the message doesn't start with(.Stashing only
image_processor.pygivesAssertionError: assert 2 == 1; with the change,tests/others/test_image_processor.pyis 11 passed.ruff checkandruff format --checkclean on all four files.Who can review?
@yiyixuxu @sayakpaul @DN6