feature/add-model-to-log-file

[aalbacetef]

Description

When trying to reproduce an image, the information stored in "log.csv" is very useful. Unfortunately it's missing one key bit of information: what model you used.

This PR adds support for storing model information (name and hash) in the "log.csv" file.

It also takes care of the scenario where there's a preexisting "log.csv" file that needs to be migrated.

Here's an issue that brings this up: #5021

Screenshots/videos:

Here's the file read as a spreadsheet (I've omitted the prompt column).

Checklist:

• I have read contributing wiki page
• I have performed a self-review of my own code
• My code follows the style guidelines
• My code passes tests

[w-e-w]

no objection to what the PR is trying to achieve
so this is a warning that will be a merge conflict with #14645

[aalbacetef]

no objection to what the PR is trying to achieve so this is a warning that will be a merge conflict with #14645

I just went through the diff. You're right in that it's going to merge conflict, luckily it seems fairly straightforward to resolve.

[aalbacetef]

one potential issue this would cause a one-time issue to the current existing log.csv as this adds 2 exter keys

depending on what they're read the CSV

I don't know if this should be addressed adding extra logic seems overkill for a one-time minor issue

How so? I added code to ensure CSV files are always synced.

In fact, it should work whenever any number of fields are added. I tested this.

See:

stable-diffusion-webui/modules/ui_common.py

Lines 103 to 108 in 315e40a

	logfile_path = os.path.join(shared.opts.outdir_save, "log.csv")
	# NOTE: ensure csv integrity when fields are added by
	# updating headers and padding with delimeters where needed
	if os.path.exists(logfile_path):
	update_logfile(logfile_path, fields)

which calls

stable-diffusion-webui/modules/ui_common.py

Lines 39 to 59 in 315e40a

	def update_logfile(logfile_path, fields):
	import csv
	with open(logfile_path, "r", encoding="utf8", newline="") as file:
	reader = csv.reader(file)
	rows = list(reader)
	# blank file: leave it as is
	if not rows:
	return
	rows[0] = fields
	# append new fields to each row as empty values
	for row in rows[1:]:
	while len(row) < len(fields):
	row.append("")
	with open(logfile_path, "w", encoding="utf8", newline="") as file:
	writer = csv.writer(file)
	writer.writerows(rows)

[w-e-w]

How so? I added code to ensure CSV files are always synced.

ahh my bad, misread the code

[Sj-Si]

Just a couple nit picks on my part. By no means do you have to make these changes; these are mostly personal preference if anything.

One other thing I noticed is that we're importing import csv in multiple functions after this change. It may be better if we move that import to the top of the file instead of having it inside the functions.

[Sj-Si]

I'd recommend using typehints whenever adding new functions. The repo doesn't have them now but it would definitely be nice if new functions were implemented with them for functions/classes.

Also a simple docstring just to satisfy linters wouldn't hurt. This is just an example description of the function so if you can think of a better one then feel free to modify it.

-def update_logfile(logfile_path, fields):
+def update_logfile(logfile_path: str, fields: list[str]) -> None:
+    """Updates existing log file rows to match current log file column count."""

[aalbacetef]

agreed, I mostly wanted to retain the style of the surrounding, but if @AUTOMATIC1111 is in agreement, I'd be happy to add the typehints

[Sj-Si]

This is a bit nit picky on my part but this approach could be a bit more pythonic.

-    for row in rows[1:]:
-        while len(row) < len(fields):
-            row.append("")
+    rows = [row + ([""] * (len(fields) - len(row))) for row in rows[1:]]

[aalbacetef]

hmm I'll have to push back on this, while list comprehensions are usually my preference, this becomes nigh unreadable.

[Sj-Si]

hmm I'll have to push back on this, while list comprehensions are usually my preference, this becomes nigh unreadable.

Yeah fair enough. I got a bit carried away code golfing it seems. :)

[aalbacetef]

Haha it happens, I appreciate your input though 👍

[aalbacetef]

One other thing I noticed is that we're importing import csv in multiple functions after this change. It may be better if we move that import to the top of the file instead of having it inside the functions.

I debated this point a bit, I noticed csv was imported in the method where it was used so I tried to stay with the style, but I do agree a top-level import would be preferred. I defer to @AUTOMATIC1111's judgement.

[AUTOMATIC1111]

Top level import is fine.

I personally would rather not have update_logfile function at all for the sake of having less code - if a .csv file is missing a few headers, it should still be usable.

If you think it's needed, then at least it should not write to disk when there are no changes in header (which is almost always).

[aalbacetef]

Top level import is fine.

I personally would rather not have update_logfile function at all for the sake of having less code - if a .csv file is missing a few headers, it should still be usable.

If you think it's needed, then at least it should not write to disk when there are no changes in header (which is almost always).

Cool, I've done the following:

• resolved the merge conflict
• moved csv to a top level import
• skipped writing changes when there are no changes to the header
• tested out the code and can confirm it syncs old logfiles
• added docstring

[aalbacetef]

cc @AUTOMATIC1111 should be ready for review/merge now!

[Woisek]

Does this solve the issue of inserting the last used checkpoint instead of the original/first one? 🤔