Nerogar / OneTrainer

OneTrainer is a one-stop solution for all your stable diffusion training needs.
GNU Affero General Public License v3.0
1.77k stars 148 forks source link

[Bug]: Adding a blank/space at the end of a concept directory produces an error message when preparing the dataset for training #358

Closed gilga2024 closed 3 weeks ago

gilga2024 commented 4 months ago

What happened?

An error message that a concept directory could not be found was shown when preparing the dataset for training / caching.

Steps to reproduce (on a linux machine):

  1. create a directory that contains a blank/space at the end of the directory name -> "abc "
  2. create a concept and refer to the directory just created as the "path"
  3. start the training with the concept
  4. The error message is shown

Assumed problem/error in code: Somewhere in the pipeline that handles concept directory a "trim" method is called that removes the blanks/spaces at the end.

What did you expect would happen?

The training should start without an error message

Relevant log output

Traceback (most recent call last):
  File "/mypath/OneTrainer/modules/ui/TrainUI.py", line 528, in __training_thread_function
    trainer.train()
  File "/mypath/OneTrainer/modules/trainer/GenericTrainer.py", line 502, in train
    self.data_loader.get_data_set().start_next_epoch()
  File "/mypath/OneTrainer/venv/src/mgds/src/mgds/MGDS.py", line 49, in start_next_epoch
    self.loading_pipeline.start_next_epoch()
  File "/mypath/OneTrainer/venv/src/mgds/src/mgds/LoadingPipeline.py", line 75, in start_next_epoch
    module.start(self.__current_epoch)
  File "/mypath/OneTrainer/venv/src/mgds/src/mgds/pipelineModules/CollectPaths.py", line 62, in start
    file_names = sorted(self.__list_files(path, include_subdirectories))
                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/mypath/OneTrainer/venv/src/mgds/src/mgds/pipelineModules/CollectPaths.py", line 45, in __list_files
    dir_list = [os.path.join(path, filename) for filename in os.listdir(path)]
                                                             ^^^^^^^^^^^^^^^^
FileNotFoundError: [Errno 2] No such file or directory: '/mypath/trainingdata/abc'

Output of pip freeze

No response

velourlawsuits commented 4 months ago

Your file directory copied into OneTrainer doesn't seem to include a blank/space --> '/mypath/trainingdata/abc' not sure why you would want a blank space but that might be the issue?

gilga2024 commented 4 months ago

Yes, it does. At least in the GUI. I chose the directory using the file picker. The error message shows, that the blank space at the end is lost on the way from gui/settings to the data import/caching process. Hence, the error message shows "abc" instead of "abc " at the end. Just follow the steps to repeat to check yourself.

I usually would not want a whitespace at the end of directory names. This was just a typo. But it drove me nuts that this error occurred (several other concepts just loaded fine), without an apparent reason. Once I removed the whitespace from the directory name, the error was gone. Although it might be uncommon, does not make it no bug.

PS: Not gonna answer to @velourlawsuits anymore.

velourlawsuits commented 4 months ago

It is not considered best practices to include spaces in your filenames for a variety of reasons. I wouldn't consider this a bug if it wasn't intentional.

gilga2024 commented 4 months ago

It is not considered best practices to include spaces in your filenames

Sorry, just laughed very hard at this.

Spaces in filenames as well as directory names are the most common thing. It might not be "normal" at the end of a directory name, but anyways no implementation that handles filenames/directory names/paths should call a "trim"-operation or something similar somewhere in the code. That's probably what happens. People might make a similar typo like me and maybe there is other more "bad" cases associated to the code that produces this bug. That's why I raised this issue (and why bugs tickets are created in general). It's not severe, far from it. But still relevant.

The dev team may do with it whatever they like. I know my way around it.

velourlawsuits commented 4 months ago

Odd sense of humor. Also you're wrong :) good luck.

mx commented 4 months ago

The cure here could be worse than the disease. A lot of people paste pathnames into UI fields, and paste output often includes extraneous whitespace on the ends. I think we need to balance whether fixing this just opens up more problems for people.

gilga2024 commented 4 months ago

Ok, this sounds reasonable and is a hard one to solve... one could trim in case copy & paste was used / a path was manually entered and not trim if the file chooser was used. But this sounds inconsistent.

Maybe showing a warning message in case something that seems dangerous/wrong was entered into this field would make sense!? For example a small red explanation mark beside the input field and a warning message in the mouse hover text.

The UI concept itself could also be used for other fields in the future (since people often seem to run into these kinds of problems judging from issues in the tracker)...