Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

You are going to have an overwrite issue. #9

Closed
Gerschel opened this issue Feb 13, 2023 · 16 comments
Closed

You are going to have an overwrite issue. #9

Gerschel opened this issue Feb 13, 2023 · 16 comments
Labels
bug Something isn't working

Comments

@Gerschel
Copy link

Gerschel commented Feb 13, 2023

When a person updates the extension, since the git has the files aspect_ratio.txt and resolutions.txt, this means, any changes they have made would be overwritten.

This is the cause because the webui decided to use a hard reset when making a pull.
They made this decision, because some did not know how to handle a merge conflict when their local changes did not match the remote files. So now it just replaces them.

Git was not designed to be an update mechanism.

The webui has a way of using an install.py file for updates/installs.

This file does not work as others might think.
This file is executed on a full shutdown and restart of the webui.
So if a user hits the apply and restart button for an update, it does not execute this file.

The better way to handle this is to create your own updater.

How I managed this in the past, I had python move the configs to a different file name when the python script (in your case, sd-webui-ar.py) executes. But it also does a check to see if the offest file exists, if it exists it does not rename the file (avoid overwrite), if the offset was never created, then perform the rename.

I'm not going to write your updater, but here's a copy of what I did. As a note, I included a blank file in the git commit, since I had written this, in other projects I use a different file name instead of 'preset_manager_update...', you can target your configs, rename if needed, and if not needed, delete them. Treat them like a flag, they exist after redownload, on first start they get deleted, if they don't exist, then it must be up to date, if they exist, they just did a redownload or downloaded for the first time.

import os
import shutil


update_flag = "preset_manager_update_check"

additional_config_source = "additional_components.json"
additional_config_target = "additional_configs.json"
presets_config_source = "preset_configuration.json"
presets_config_target = "presets.json"

file_path = scripts.basedir() # file_path is basedir
scripts_path = os.path.join(file_path, "scripts")
path_to_update_flag = os.path.join(scripts_path, update_flag)
is_update_available = False
if os.path.exists(path_to_update_flag):
    is_update_available = True

    print("Preset Manager: Checking for pre-existing configuration files.")


    source_path = os.path.join(file_path, additional_config_source)
    target_path = os.path.join(file_path, additional_config_target)
    if not os.path.exists(target_path):
        shutil.move(source_path, target_path)
        print(f"Created: {additional_config_target}")
    else:
        print(f"Not writing {additional_config_target}: config exists already")
                    
    source_path = os.path.join(file_path, presets_config_source)
    target_path = os.path.join(file_path, presets_config_target)
    if not os.path.exists(target_path):
        shutil.move(source_path, target_path)
        print(f"Created: {presets_config_target}")
    else:
        print(f"Not writing {presets_config_target}: config exists already")
                    
                    
    os.remove(path_to_update_flag)
@alemelis
Copy link
Owner

alemelis commented Feb 14, 2023

Thanks for flagging this!

I'll implement something similar for this repository...but I'm not sure I understand the logic though.
How are the user configs handled?

Say the user installs the extension:

  • aspect_ratios.txt is cloned with the repo, it contains the default values
  • when the web-ui starts, the aspect_ratio_offset.txt doesn't exist
  • aspect_ratio_offset.txt is created and it is a copy of aspect_ratios.txt
  • the extension loads aspect_ratio.txt
  • user is happy

Now, the user wants a new setting:

  • open aspect_ratios.txt
  • re-launch web-ui
  • aspect_ratio_offset.txt is already there

at this point the extension loads what? aspect_ratios.txt or aspect_ratios_offset.txt?

  • if it loads aspect_ratios.txt the new configs are used, but at the next pull, these are reverted to default. Nothing changes from the current implementation
  • if instead aspect_ratios_offset.txt is loaded, the new changes are not taken into account
    In both case the user is going to miss the functionality.

It's not enough to have an additional file, we should also check for its content. Namely, if aspect_ratios.txt is the same of the main branch, aspect_ratios_offset.txt should be loaded (as it may contain the latest user changes). If aspect_ratios.txt is not the default one, we assume that the user made changes and it doesn't care about the previous state in aspect_ratios_offset.txt

Hope it makes sense, please feel free to correct me if I'm wrong

@alemelis alemelis added the bug Something isn't working label Feb 14, 2023
@Gerschel
Copy link
Author

The concern sounds more like a naming convention. But it also includes trying to preserve what the user has. If they've made changes to their configs, it's kind of too late. The best you can do is put a notice.

What you can try to do is use the git mv command to rename your files; git mv aspcet_ratios.txt initial_ratios.txt
I am not 100% certain how this will play out on their device when they do a pull. The real problem is that they decided that for the webui, they enforced a hard pull to overwrite files. It's not your fault. Once you fix the issue, it wont happen again, just pull the band-aid and get it over with.

I would choose a name that makes sense in both categories, the initial download and the file to change.

Something like default_ratios.txt and default_resolutions.txt, then it can rename to what you are reading for in your script.

For this project, fortunately, there should not be much needed for versioning.
But versioning is a thing to consider.

You can add a header that has some format you look for that states the version, and if the header does not exist, you can assume it's version 0.

Creating a version field in your file will give you the flexibility to change the structure of your data, but this also means writing different read and write rules for the data.

When you get that involved, it might be beneficial to write another class, where that class has an oppurtunity to read the data according to the version, update by read and write if possible, and pass the data back to your script.

So your script that gets run for the ui and handlers has the focus for the ui and logic, it gets the data from calling the updater/data_formatter class. The updater/formatter class might have several methods to restructure the data according to the version it sees.

Think of it where you might have made two updates, but a person might've been busy and not updated each update, so when they do an update, they are leaping by two, and if the data structure changed both times, then they would be too far behind for it to work, so you will need to keep the logic for at least two iterations, or have the logic where it wrangles the data from at least two versions ago. As a version becomes stale, those methods for that version can change to become just a data wrangler, which calls the next data wrangler, until it catches up to the newest version.

@alemelis
Copy link
Owner

I see, then it's possibly for the best to avoid shipping any .txt with the extension

at the first run the extension checks for a file, if it's not there it will create one with default settings. That file can be changed by the user at will and git will not overwrite it at the next pull

@alemelis
Copy link
Owner

done in 9c0827b

@Woisek
Copy link

Woisek commented Feb 20, 2023

It's not cool that the text files get always overwritten. Second time now. If you write that they get overwritten here on GitHub doesn't help the users, who update from WebUI directly.

The simplest solution would be to check if the text files are present and if so, back them up and then create new ones. Optionally you could check the size of the existing files to see if they are changed.

@alemelis
Copy link
Owner

The simplest solution would be to check if the text files are present

that is exactly the way it is already implemented...it works fine on my installation

def read_aspect_ratios(self):
ar_file = Path(aspect_ratios_dir, "aspect_ratios.txt")
if not ar_file.exists():
write_aspect_ratios_file(ar_file)

It's not cool that the text files get always overwritten. Second time now. If you write that they get overwritten here on GitHub doesn't help the users, who update from WebUI directly.

Just to understand better your issue. You git pull (or updated the extension from webui) and the files were removed (fine, this was expected). Then you started webui and the files were automatically populated with defaults (or you re-created them manually). What's the second time they were overwritten?

@alemelis alemelis reopened this Feb 20, 2023
@Woisek
Copy link

Woisek commented Feb 20, 2023

What's the second time they were overwritten?

Right now. I did an update in the last hour and after that, the both file where replaced by the defaults. Stupidly I didn't thought about of back them up prior to the update ...

@Gerschel
Copy link
Author

It's not Alemedis fault. Alemdis is trying to work around an issue the webui had created (intentionally, not a bug, so users don't get a rejected git pull if they've modified configs).

There are several ways in which to handle it, and there isn't a guide, which is what prompted me to give a heads up (maybe I should write a guide for developing for the webui, and some templates).

A lot of extensions end up going through this situation, and working around how git and the webui works has a little learning curve, it's not typically resolved in the first commit that tries to resolve it. It's not often that a developer has to code around a git pull hard, so they learn what sticks and does not stick on the next commits. Pretty much, they'd have to expierence it once to get a grasp, because the behavior can vary because of different ways users might do their update.

@Gerschel
Copy link
Author

What would be a nice feature for the webui, is when an extension has a message/notification system, where a developer can push a message. On a basic level it would say there's an update available, on a more advanced level, there can be warnings such as "update will overwrite these files, please make a backup of ... if you have changes you would like to keep".

But a push system might not work so well, as they might skip/miss several messages if there are several updates between them going online. So a pull might be needed. Then it can pull from something like an rss feed that contains the new messages since last pull, and this can be implemented in the extensions tab, and clicking pull messages is required before any updates are allowed.

@Woisek
Copy link

Woisek commented Feb 20, 2023

@Gerschel

It's not Alemedis fault.

I don't blame anyone of some fault. I'm just saying, that if some files get overwritten, especially user edited files, then at least they should be auto-backuped from the system. IMHO.

@Gerschel
Copy link
Author

Gerschel commented Feb 20, 2023

But that's impossible without a commit that would initially overwrite it. The overwrite is enforced through the webui, any update to try and back it up, that update would cause the overwrite and the backup would be the overwritten file. The only way to get around it, would be a script that the extension previously created where they can send commands for your machine to execute arbitrarily, and that's a safety issue, pretty much a botnet.

@Woisek
Copy link

Woisek commented Feb 20, 2023

OK, I'm currently know zero about git and how it works.
But in my simple mind I think, that when a file can get overwritten, it can also be checked. So I thought, before those files get overwritten, the system should check if they differ from the default/initial size. If no, it's safe to overwrite, if no, back them up and then overwrite.
Can't that be done?

@Gerschel
Copy link
Author

Gerschel commented Feb 20, 2023

Because of the way they call git, "git pull --hard" "git reset --hard", it means overwrite everything with no biases, then the code that could possibly check gets executed after the overwrite.

@Gerschel
Copy link
Author

Gerschel commented Feb 20, 2023

Found it. Line 72 in extensions.py

    def fetch_and_reset_hard(self):
        repo = git.Repo(self.path)
        # Fix: `error: Your local changes to the following files would be overwritten by merge`,
        # because WSL2 Docker set 755 file permissions instead of 644, this results to the error.
        repo.git.fetch('--all')
        repo.git.reset('--hard', 'origin')

Instead of users getting a warnging about overwrite which rejects a git pull until the user decides if they want to stash the files or overwrite, they opted to overwrite each time, because "docker" users couldn't handle doing a chmod on their own.

Git is not an update tool, nor is it a backup tool, using it in these ways causes these issues and is not efficient.

the system should check if they differ

It would by doing a git fetch or a regular git pull (which is a "git fetch" followed by a "git merge" command), but the flag "--hard" says to skip all that.

image

@Woisek
Copy link

Woisek commented Feb 21, 2023

Because of the way they call git, "git pull --hard" "git reset --hard", it means overwrite everything with no biases, then the code that could possibly check gets executed after the overwrite.

Ah, OK. So there isn't even a chance to check it beforehand. That's unfortunate ... :/

Edit: What suddenly come into my mind: Isn't it possible, to make the creation and check of the two files afterwards, when the script is running?
Meaning, at first install, those files will not be created, but only after the first run, when the script has control and checks are possible. Decouple those files at install time, so to say (I don't even know if this makes any sense what I'm writing ... :D ).

@alemelis
Copy link
Owner

Because of the way they call git, "git pull --hard" "git reset --hard", it means overwrite everything with no biases, then the code that could possibly check gets executed after the overwrite.

Ah, OK. So there isn't even a chance to check it beforehand. That's unfortunate ... :/

Edit: What suddenly come into my mind: Isn't it possible, to make the creation and check of the two files afterwards, when the script is running? Meaning, at first install, those files will not be created, but only after the first run, when the script has control and checks are possible. Decouple those files at install time, so to say (I don't even know if this makes any sense what I'm writing ... :D ).

Exactly, not much you can do after the extension has been updated.

The only thing that could have been done (were I to know about this beforehand) was to to ship the extension without the config files. Then at runtime, create the default ones programmatically in case the user didn't provide some.

Which is the state the extension is now after the latest commit. We're good for future updates but all the users will see their configs being overwritten once when upgrading to the current version.

A bit unfortunate...if I were to know I wouldn't have included those files in the first place 🤷

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants