Generate file prefix by looking into dream_log.txt instead of last image in the directory

[SMUsamaShah]

This also fixes a bug where it keeps overwriting last file when run as dream.py --from_file path/input.txt -o output/path/

[SMUsamaShah]

If you run again after removing the last generated image because you didn't like it, it will save with the name of file you just deleted. Now you have two entries in log file with same name. You can't reliably lookup in log with this.

That's why using log file for next name is better in my opinion.

[lstein]

Ouch. This unwanted behavior is an indirect consequence of the decision to make pngwriter stateless in PR #231. Originally, I had pngwriter keep track of the last file name written during the session rather than going back to the filesystem. Of course, the bug you've identified would still have bit us if the user deleted the last file between sessions.

Before I review your solution, we need to discuss what to do in the event that the user deletes or moves the log file. Here are some options:

1. Give a big fat warning if the directory contains png files and no log file.
2. Always check whether the generated filename exists and never clobber an existing file (I might be missing it, but I don't think this check is implemented yet)
3. Instead of using the log file, read the last written filename from a hidden file in the directory. On Linux and Mac, this would be a file starting with a ".". I think on Winodws use set the "h" attribute?

I'm leaning a bit towards (3), but there's also the risk that the user finds the hidden file and deletes it, not knowing that it will bring calamity upon them. In this event, maybe we fall back on tallying the files in the directory (or the log) and recreate the hidden file.

Bringing in @bakkot on this conversation, cause he's worked on the filenaming issue in the past.

[bakkot]

The refactor in #231 didn't change this behavior. The old pngwriter would also look for a new "prefix" from the filesystem every time the class was instantiated, which happened every time you entered a new prompt at dream.py.

Anyway, we should figure out what the actual behavior we want is. Personally I would be inclined to:

• keep track of the last prefix used during this session
• during initialization (i.e. when the PngWriter class is instantiated), start that counter by reading the last entry in the log file
• whenever we go to generate a new prefix, read the output directory and ensure the candidate prefix is not already in use

Not a fan of introducing a new hidden file - as a user hidden files tend to provide a pretty annoying experience, because it means I can't just reset everything by deleting all the files I can see.

[morganavr]

I have a file "counter.txt". Inside it there is a number. This number will be used for generation of next image. Then number inside "counter.txt" will be increased by one. I have not experienced any problems with this approach.

[SMUsamaShah]

keep track of the last prefix used during this session

I think this is a much better approach. We shouldn't be needing to go back to log file or whole directory to get next prefix in the same session.

[SMUsamaShah]

Since we are reading opt.outdir in loop for some reason, i didn't find a simple way to put last prefix in session.
Updated to get prefix from png if log was not found for some reason.

[lstein]

keep track of the last prefix used during this session

I think this is a much better approach. We shouldn't be needing to go back to log file or whole directory to get next prefix in the same session.

I like the "counter.txt" approach. It means we only have to read one file and it helps the user understand exactly what is happening. There still needs to be a check that we don't overwrite existing files.

[morganavr]

I like the "counter.txt" approach. It means we only have to read one file and it helps the user understand exactly what is happening. There still needs to be a check that we don't overwrite existing files.

Another advantage of "counter.txt" is that user can open this file to input any number they want. I do it myself occasionally when software starts generating 5 digits numbers that are too difficult to remember, so I reset it to "12" :)

[SMUsamaShah]

Adding one more file for count means one more moving part. I believe, from your comment somewhere else @lstein that you see log.txt as a way to quickly lookup command used to create an image. It's nice and simple approach and we should retain that.

If log has reached the 50.png and counter.txt says 10, then 11.png and subsequent ones will be entered twice in log.txt and you will have to read the command from png itself.

[lstein]

How about we do this?

• at pngwriter initialization time, it looks for the log file (once) and gets the latest prefix
• if log file is missing, then read the directory (once) and get the latest prefix
• if no files in directory, then reinitialize to prefix 000001
• prefix stored in self and incremented for each generation step so we don't have additional filesystem overhead
• there is always a check before clobbering a file, regardless of what the prefix says

I think this will work. Did I miss something?

[SMUsamaShah]

there is always a check before clobbering a file, regardless of what the prefix says

and in case we do find an existing file with same name, we repeat the first three steps.

[bakkot]

there is always a check before clobbering a file, regardless of what the prefix says

Presumably with that check disabled for upscaling, which deliberately overwrites the original unless --save_original is passed?

Also, re "if log file is missing, then read the directory (once) and get the latest prefix" - I would do this unconditionally, and choose the prefix to be the maximum of the two strategies (directory enumeration and log file). A single directory enumeration shouldn't be costly, and it means that you still get incrementing prefixes as long as you keep either the files in the directory or the log, which seems nicer.

Other than those two points, that sounds good to me. It might be a little annoying if, for example, you were generating 5 images, and the first 2 didn't conflict but then the third one did - your prefix would change in the middle of the batch. But I expect this will basically never happen - given this design you have to come up with something pretty contrived for that to be possible - so I'm not worried about it.

[lstein]

This sounds on target. Who wants to take a crack at implementation?

(against the development branch, please)

[SMUsamaShah]

Again, couldn't just store prefix in session because we can read --outputdir on command any time. Made a few changes anyway.

1. Get next prefix from both log.txt and weblog.txt
2. if its 0, get by finding last png in the directory
3. when writing png, if file already exists, regenerate prefix by get prefix from last png in dir

[bakkot]

A few notes:

• this logic should live in pngwriter.py, not dream.py
• the pngwriter instance can keep a dictionary of outdirs that it knows the prefix for, to avoid re-initializing it every time even in the presence of --outdir (that is, you only need to re-initialize when you encounter a directory you haven't seen before)
• I haven't tested it, but from reading the code, it doesn't look like this correctly handles the upscaling case: in that case we actually do intend to overwrite the original image
• there are other consumers of pngwriter (in particular in server.py), and this logic should be applied to them as well

[lstein]

A few notes:

• this logic should live in pngwriter.py, not dream.py
• the pngwriter instance can keep a dictionary of outdirs that it knows the prefix for, to avoid re-initializing it every time even in the presence of --outdir (that is, you only need to re-initialize when you encounter a directory you haven't seen before)
• I haven't tested it, but from reading the code, it doesn't look like this correctly handles the upscaling case: in that case we actually do intend to overwrite the original image
• there are other consumers of pngwriter (in particular in server.py), and this logic should be applied to them as well

I agree with @bakkot 's comments. I'd add that we should have a unified logger to create and format the log entries. Since both logging and pngwriting need to generate a uniform prompt, some refactoring is required so that pngwriter isn't responsible for prompt generation.

This is all tied up with @bakkot 's proposal to create an object class that acts as a centralized location to store all the gathered parameter values. As I understand the vision, this thing will act like a dictionary for getting/setting values, and be able to produce serialized representations of the parameter state using JSON and command-line formats. See discussion at #351

[lstein]

Could you have a look at @bakkot 's comments and see if your implementation needs to change?

[SMUsamaShah]

Could you have a look at @bakkot 's comments and see if your implementation needs to change?

Updated again. Not sure how upscale case overwrites the images since it still asks for unique prefix first. I hope I got that right.

[lstein]

Thanks for the changes. I'll review this as soon as I complete the 1.14 release, either today or tomorrow.

[ioma8]

Hello, I have a question - you agreed to create a counter.txt file to keep track of last file prefix. I agree its good idea. But I suppose in near future we will want to save more user related setting / params. Eg the lastly used parameters settings from the webUI could be saved in-between sessions, etc. Wouldnt it be better to create a more generic settings file eg in json format, where among other things would be saved the last prefix? Just an idea.

[hipsterusername]

This PR seems stale, closing for now.