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

Clean up settings properly for removed models and also when user manu… #2098

Merged
merged 2 commits into from
Mar 11, 2024

Conversation

manyoso
Copy link
Collaborator

@manyoso manyoso commented Mar 9, 2024

…ally deletes.

@manyoso manyoso requested a review from cebtenzzre March 9, 2024 15:04
@cebtenzzre
Copy link
Member

So now if a user temporarily moves/renames a .gguf file or changes their model directory, GPT4All will permanently forget the settings the user has configured for whichever models it doesn't see the next time it launches? That doesn't seem like a good thing.

@manyoso
Copy link
Collaborator Author

manyoso commented Mar 10, 2024

So now if a user temporarily moves/renames a .gguf file or changes their model directory, GPT4All will permanently forget the settings the user has configured for whichever models it doesn't see the next time it launches? That doesn't seem like a good thing.

I disagree. We cannot control what a user does in all circumstances outside of the GUI. If the user moves files around manually we have no way of associating the old settings with that new location. The settings are useless in that case. We should we keep them? What proposed behavior would you have instead?

The only other thing I can think to do would be to just ignore those settings but not erase them. In other words, just skip over them without the erase... then if a user somehow restores the file to the proper place they become relevant again? Seems we're coding for a very very rare eventuality in that case and the settings file can get larded up with old settings. I guess I could be convinced to do it this way but the OCD in me really hates that we have no way to clean up the settings file in that case.

@ThiloteE
Copy link
Collaborator

In the settings of the GUI, there is the "restore defaults" button, which I would expect something to do like this, but the restore defaults button only works per model, not global.

There is also the "remove" button, but that one also only removes a single model settings.

  • One possibility could be to add another button that cleans up the model settings file --> 👎 Too many buttons
  • Another possibility could be to make the "remove" button a multiple choice button that offers to not only delete the current model settings, but allows the user to choose "also delete settings from removed models" or something like that --> 👍
  • Another possibility could be to cleanup the model settings file only during uninstallation or during an upgrade of GPT4All and only with permission by the user.

@manyoso
Copy link
Collaborator Author

manyoso commented Mar 10, 2024

There is also the "remove" button, but that one also only removes a single model settings.

This is only for clones...

  • One possibility could be to add another button that cleans up the model settings file --> 👎 Too many buttons

Yeah, no good.

  • Another possibility could be to make the "remove" button a multiple choice button that offers to not only delete the current model settings, but allows the user to choose "also delete settings from removed models" or something like that --> 👍

Doesn't help because the remove button is only for clones.

  • Another possibility could be to cleanup the model settings file only during uninstallation or during an upgrade of GPT4All and only with permission by the user.

Yeah, this still doesn't help with the stale settings from models that have been removed/renamed. The only option I can think of is again:

  • What this patch does and cleans up the stale settings when it can't find the file
  • Ignore those settings but don't actually remove them

I'll wait for @cebtenzzre to comment if he has another idea...

@cebtenzzre
Copy link
Member

cebtenzzre commented Mar 11, 2024

Simply not listing models that don't exist--or at least, listing them with a non-blank filename and an indication that they are not available--would be consistent with previous versions of GPT4All.

IMO, deleting settings is a destructive option that should only be done if the user explicitly requests it--whether they click a "remove" button, or acknowledge a warning dialog ("three model files were not found, would you like to remove them from your configuration?"), or click a "cleanup model settings" button, or anything else like that.

I will always be frustrated by software that tries to be smart and ends up creating more work for me. Perhaps I start GPT4All and forgot to plug in the external hard drive I keep my model files on. Then upon restarting it with the drive plugged in, all of my settings are gone. That would be a bug to me. It is wrong to assume that just because a file is not currently present at a particular path that it is unlikely to appear again.

@manyoso
Copy link
Collaborator Author

manyoso commented Mar 11, 2024

I will always be frustrated by software that tries to be smart and ends up creating more work for me. Perhaps I start GPT4All and forgot to plug in the external hard drive I keep my model files on. Then upon restarting it with the drive plugged in, all of my settings are gone. That would be a bug to me.

Ok, then I will make it so that the setting is ignored and not listed if it doesn't have a corresponding valid file, but I won't erase it.

…ally deletes.

Signed-off-by: Adam Treat <treat.adam@gmail.com>
Signed-off-by: Adam Treat <treat.adam@gmail.com>
@manyoso manyoso merged commit a6a3e00 into main Mar 11, 2024
6 of 10 checks passed
@3Simplex
Copy link
Contributor

I tested this update. I found after the update this remnant exists. It has no filename to associate with a model, so it would never be used right? If it can't be associated, is it safe to be removed? Or would this be a situation that you want to ask the user to link it to a model or remove it.
Screenshot 2024-03-11 150716

@cebtenzzre
Copy link
Member

I found after the update this remnant exists.

The title of this PR wasn't updated before it was merged, but in its current form the stale entries will simply be ignored instead of deleted, for the reasons discussed above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants