Input Validation, Tensorboard Tweaks + Drag-n-drop support - #908
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
for more information, see https://pre-commit.ci
This comment was marked as outdated.
This comment was marked as outdated.
Every field has basic validation on its data type, reverting if an invalid input is provided (with a debounced delay). The only field with anything really fancy is
I strongly believe we still need, it will be toggeable though!. If we had more gatekeepers to getting started with OT or it had remained power user only software then yes I would agree, however that is not our reality |
This reverts commit 71cde89.
|
I have now disabled DnD on linux after extensive conversations with the maintainer of TKInterdnd2 and another dev, who noted DnD does not even work with Qt on all linux distros (the gold standard). Didnt want to do this but, no choice :(, tested again on Linux and Windows. |
…ith invalid values. Logic tweaks.
|
After discussing further with Dxq, value reversion has been removed. In its place I have added
I think this is ready for further reviews again. |
There was a problem hiding this comment.
-
it still reverts, for example when I enter a string into Learning Rate. Is that intended? [see Discord - as long as it's a strict improvement it isn't a stopper, even though I prefer just the red marking with no reverting] - minor: "prevent overwrite" is enabled. I select a preset, "Model Output Destination" goes to default "model/model.safetensors". That file exists, so "Prevent Overwrite" is adding "_1" to the file. But the orange marking stays, even though there isn't a problem anymore
- it complains that "File does not exist" when Base Model is a huggingface path like
lodestones/Chroma1-HD. Not initially when selected a preset, but after I manually entered it - the "File does not exist" tooltip when it appears and then I switch to another tab like
training, it blinks out of nowhere into that tab even though the error is on themodeltab - I'm a slightly confused by all the functionality and switches affecting the overwrite-files behaviour (Auto-correct input, Prevent Overwrite, Use Friendly Names). It basically only affects the
Model Output Destination, right? The only other output path_entrys I could find are directories (workspace, debug, cache), where this functionality mostly doesn't apply. - The "run name" is used in backups, for example
Creating Backup workspace/run/backup/2026-02-06_13-17-53-**model_1**-backup-20-0-20. I think using any type of prefix/suffix in backups breaks the backup discovery when you enable "Continue from last backup". At least, I remember this to be the reason why we haven't used the "Save Prefix" for backups previously. - the "run name" is consistently used for saves, tensorboard and samples. However, the "Save Filename Prefix" is ignored in most cases. The "run name" is the output file name. I think this breaks how the prefix is used currently by many users, including myself: You do 3 differents runs with different parameters, and use the save prefix so you can later identify the runs in tensorboard, samples and saves. For example, you might use "LR1e-3" as a prefix. Now you only get "model_1", "model_2", and "model_3" instead, unless you anticipate that and modify the output filename accordingly.
- review: test functionality that might affect it with cloud training
| is_full_width=False, | ||
| show_toggle_button=True | ||
| show_toggle_button=True, | ||
| placeholder_text="You have no concepts defined. Either click the 'Add Concept' button or drag and drop a directory here to add a new concept." |
There was a problem hiding this comment.
please add a check for DnD being available, so we don't tell the user to do something that doesn't work
| data.append(("validation_auto_correct", True, bool, False)) | ||
| data.append(("validation_show_tooltips", True, bool, False)) | ||
| data.append(("use_friendly_names", False, bool, False)) | ||
| data.append(("prevent_overwrite", True, bool, False)) |
There was a problem hiding this comment.
should this be the default?
I don't like it personally, because it'll accumulate output files even though I never use them. I only use saves.
|
|
||
| # validation settings | ||
| data.append(("validation_auto_correct", True, bool, False)) | ||
| data.append(("validation_show_tooltips", True, bool, False)) |
There was a problem hiding this comment.
why is this a setting? Isn't this always good? Why would I want to disable this?
| self.bucket_ax.cla() | ||
| aspects = [str(x) for x in list(aspect_buckets.keys())] | ||
| aspect_ratios = [self.decimal_to_aspect_ratio(x) for x in list(aspect_buckets.keys())] | ||
| aspect_ratios = [self.decimal_to_aspect_ratio(x) for x in aspect_buckets] |
There was a problem hiding this comment.
what are these code changes to aspect statistics?
Can't find anything about this in the PR description
There was a problem hiding this comment.
Just for self note: It was because of testing around the matplotlib TkAgg issue with DnD and I noted we were doing it inefficently
| name = path.stem | ||
|
|
||
| # fall back to prefix if no name could be extracted | ||
| if not name: |
There was a problem hiding this comment.
see comment in main. save prefix is ignored in most cases.
| @@ -1,56 +1,170 @@ | |||
| from __future__ import annotations | |||
There was a problem hiding this comment.
not reviewing this file - please double check before merge
| from typing import Any | ||
|
|
||
| from customtkinter import CTk, CTkToplevel | ||
| from tkinterdnd2 import DND_FILES |
There was a problem hiding this comment.
always import, even if Linux? See the other file for conditional import
| @@ -0,0 +1,482 @@ | |||
| """Input validation utilities for UI components.""" | |||
There was a problem hiding this comment.
please reconsider if we need all this code for one entry field (see main comment)
|
|
||
| if sys.version_info < deprecated_ver: | ||
| sys.stderr.write("Warning: Deprecated Python version found. Update to %s or newer\n" % (tuple_to_str(deprecated_ver))) | ||
| sys.stderr.write("Warning: Deprecated Python version found. Its strongly recommended to update to %s or newer\n" % (tuple_to_str(deprecated_ver))) |
There was a problem hiding this comment.
just for my information: why?
There was a problem hiding this comment.
Because we have had 8+ users coming in going "OMGWTF how do I update to python 3.11?!" so instead of saying you must update, instead strongly recomend. Python 3.10 support will likely be dropped by the end of this year for PT (speculation)
| safetensors==0.5.3 | ||
| tensorboard==2.19.0 | ||
| tensorboard==2.20.0 | ||
| StrEnum==0.4.15; python_version < "3.11" # For tensoboardMode on 3.10 |
|
Closing as spliting up. |
High level summary of what this PR does:
path_entrywidgetsfile_entryanddir_entryinto a single functionInitial checklist
Always-onis True, shouldnt be terminated in training nor afterAlways-Ontensorboard doesnt break cloud trainingOffandTrain Onlymode don't break cloud training nor start Tensorboard locally during trainingValidation specific checklist
Tensorboard has now changed from enable + toggle to just a dropdown
Drag and drop demo
drag-n-drop-pr.mp4