Closed w-e-w closed 9 months ago
don't really understand what the lint error black
means
I see, the linter seems to me needlessly strict ...... should be "good" now https://github.com/w-e-w/sd-dynamic-prompts/actions/runs/7104213723
@w-e-w I rebased this now that #692 was merged β care to still take a look at the couple comments (in case I missed something about the quote()
use)?
you forgot to remove the old PngInfoSaver imports should be good now
@w-e-w Please see the comment though β why do we do json.dumps()
manually since isn't that what quote()
would do?
sorry I missed that message
I did try to explain it in my comment in that code https://github.com/adieyal/sd-dynamic-prompts/pull/691/files#diff-5a8a491817ca6caab16b88e82baff176d7ee992844184119b13d263cf5fae993R89-R90 but probably didn't explain it well
the issue is that json.dump()
is only apply by quote()
if ,
:
\n
are in the in the string
see code https://github.com/AUTOMATIC1111/stable-diffusion-webui/blob/f92d61497a426a19818625c3ccdaae9beeb82b31/modules/generation_parameters_copypaste.py#L40-L44
even though in the use case of prompts likely they will have a comma
or colon
or newline
in there somewhere so it will be quoted, but there is a chance of the quote actually not being applied
the problem is somewhere down the road the text would be strip()
which means some info will be lost
granted losing some leading or trailing probably don't matter, I was mostly just trying to be extra safe
I was trying to avoid quoting when it is not necessary that's why I only use the condition when it is affected by strip()
but rethinking this now I think it's safer to quote everything
as the original quote won't quote things like "
double quote in prompt, this is okay for whatever you are because those parameters are usually "sanitary", however this is not the case for user input prompts
with prompts we can potentially you can end up with some broken syntax
as far as I can tell invalid syntax doesn't seem to be causing an issue now, but down the line it is possible that in the future you might use it for something, so having valid syntax is maybe more important than readability this could also cause trouble when other extensions are involved I've push a new commit sp that quote will always be applied if it's not applied automatically
in order to prevent double quoting, I need to check if it would be automatically quoted by webui hence the logic required
when if the value of a generation_params
is None
then that key is ignored and not saved to image, the finction will return None
if string is blank
I believe this also takes into account that if in the future if webui change it's quoting logic
@w-e-w I'm not sure I'm following why we need to quote these by hand here at all? Isn't webui doing that in
for everything we put in extra_generation_params
?
@akx what part is hard to follow
the issue is that
json.dump()
is only apply byquote()
if,
:
\n
are in the in the string see code https://github.com/AUTOMATIC1111/stable-diffusion-webui/blob/f92d61497a426a19818625c3ccdaae9beeb82b31/modules/generation_parameters_copypaste.py#L40-L44
no manual quot
no manual quot | with manual quot |
---|---|
if quoting isn't applied manually then there is a chance that you can create an invalid syntax
and as I have said above
as far as I can tell invalid syntax doesn't seem to be causing an issue now, but down the line it is possible that in the future you might use it for something, so having valid syntax is maybe more important than readability this could also cause trouble when other extensions are involved
currently I'm still yet able to produce a sequence of prompt that would actually cause issues, but I cannot guarantee that such a sequence does not exist this is why I decided to play it safe and ensure that the prompts are quoted
it's possible it will be totally fine, if you really don't like it I can remove it
ok found one issue caused by non-quoting
if my prompt is
"example"
the geninfo will be Template: "example",
the information parsed out from this by webui is example
and not "example"
granted currently template is not parsed by this extension
but if it is parse in the future then they will output the wrong template
@w-e-w Yes, I did understand what you wrote and I do understand quote()
only quotes things when it needs to.
It feels like we shouldn't be quoting things by hand when we put them in p.extra_generation_params
, since extra_generation_params
isn't directly involved in creating the final textual infotext; rather, as linked, webui quotes (if it needs to) everything by default, and crucially it doesn't seem to unquote things first, so I'm afraid there'll be double quoting (encoding) if we quote things by hand.
To be clear, what I'm trying to say is that with just
if opts.dp_write_raw_template:
p.extra_generation_params["Template"] = original_prompt
p.extra_generation_params["Negative Template"] = original_negative_prompt
this is the result, and it seems fine to me:
(If there are no commas or colons in the template, it remains unquoted.)
quote() only quotes things when it needs to.
what I was trying to tell you is that this is not the case
I'm saying is webui can failed to quote()
when it needs to
because the way that the quote()
logic is written there are cases that the prompt can actually break out
the infotext was originally only designed to hold sanitized data, not arbitrary user input
example above is that if you have a prompt
"example bababab"
<- this will not be quoted by web UI
the text written to the infotxt would be Template : "example bababab"
, this is is incorrect
if in the future that this field is actually used, the text will be unquoted in the metadata that is actually read would be
example bababab
the double quotes are lost
see https://github.com/AUTOMATIC1111/stable-diffusion-webui/blob/f92d61497a426a19818625c3ccdaae9beeb82b31/modules/generation_parameters_copypaste.py#L257-L258 this is the part of code in webui that handles passing of infotax
yes this extension does not currently utilize the template to do anything but in the future if this is actually used if it were not quoted manually then certain information may be lost
yes this is an edge case if you wish to not handle this edge case I have no problems with that, as long as you understand that this edge case exist I'm just providing code that would handle this edge case
screenshot example of the issue the info text generated without ensure quoting
and this is what happes when you use read generation parameters from prompt
or send to txt2img
see how the "example bababb"
-> example bababb
, the double quotes are lost
would it be better they are double quotes to webui quoting logic?, yes and I might actually make a PR for on this
this is why I written ensure_quoted_parameter
in a way that if in the future we update the quoting logic at webui it should be compatible
but this would mean that if you would only be fixed for people for using newer versions of web UI, it's an edge case it's probably fine
the infotext was originally only designed to hold sanitized data, not arbitrary user input
I mean... it does also have the prompt though? But okay, I think I see where you're getting at here a little more now π
The only edge case I can think about (presently) is that the original text was actually just surrounded with quotes? In any other case, e.g. a sign saying "hello"
, quoting would kick in and we'd emit "a sign saying \"hello\""
, and then unquote it back properly. I don't think there's any actual use case for completely quoted prompts.
Of course it'd be better if infotext was actually a blob of JSON in the PNG iTXT chunk, but that ship may have sailed a while back (unless infotext v2...).
so do you want me to change it to not ensure quotes?
to be honest I don't really care either way (apart from my OCD), I don't even use this extension myself π
the fact is most prompts is highly likely to have a colon or comma in there somewhere so that it will be quoted anyway like I said this is an edge case
they could also be other edge cases I haven't discovered that have a bigger consequence, that's why I try to play it safe
well no
a sign saying "hello"
won't be automatically quoted, because it doesn't have comma colon or new line
but is also fine because it doesn't have double quotes at the beginning and end which means that it won't be unquoted
Of course it'd be better if infotext was actually a blob of JSON in the PNG iTXT chunk, but that ship may have sailed a while back (unless infotext v2...).
I would like that as well, but yeah I think that ship has sailed Into the storm
so do you want me to change it to not ensure quotes?
they could also be other edge cases I haven't discovered that have a bigger consequence, that's why I try to play it safe
I think it would be better if this extension didn't try to be clever (as you can see, we've tried being clever in the past, which led to this PR in the first place).
After all, webui itself is just doing
1112: self.extra_generation_params["Hires prompt"] = self.hr_prompt
1115: self.extra_generation_params["Hires negative prompt"] = self.hr_negative_prompt
so we should follow its lead here.
It would be nice not to add an entry for the negative prompt if it's empty, though?
@akx should be good now
I'll just go
over at webui we recive a bug reort [Bug]: Dynamic Prompt Metadata in blocks "Read Generation Parameters from Prompt" if extension is missing/disabled https://github.com/AUTOMATIC1111/stable-diffusion-webui/issues/14201#issuecomment-1839119678
as far as I can tell this is caused by sd-dynamic-prompts using an unorthodox method of writing the pnginfo when
opts.dp_write_raw_template
is enabledwrining pnginfo in newline causing the webui pnginfo parser webui break as I believe you are aware, base on do you have implemented a call back to remove the extra pnginfo added by this extension https://github.com/adieyal/sd-dynamic-prompts/blob/aedb3e4b91026351981f8412eb9e2e74167fdcff/sd_dynamic_prompts/pnginfo_saver.py#L45
in other words if one does not have dynamic-prompts, the pnginfo syntax is invalid
this PR changes the way how pnginfo in written I essentially commented out the code that you use to write pnginfo and use the "orthodox" method of writting pnginfo I chose to comment out and not remove the code because I don't know if there's other components that relies on those info writing classes the
strip_template_info
is still enabled for compatibility with existing imagesthere's one practical downside of my implementation the information displayed in the PNG info tab looks far less tidy as it is all cramped in one place but I believe compatibility should be more important than aesthetics