-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
Create endpoint for resetting settings #6840
base: main
Are you sure you want to change the base?
Conversation
Thanks for this, I think a reset endpoint might be really useful.
Interesting! But idk, why would local clear it? (ref also: comment, sorry I didn't get to it before) if the user wants to reset the settings they set specially in the UI run, maybe it should fallback to what is loaded from the toml. 🤔 What is the use case for clearing, when the user has themselves written stuff in configuration files? This whole behavior has been strange for a long time, granted, but I don't know why it seems to become stranger, rather than go towards making it more user-friendly and understandable. |
llm_base_url='', | ||
remote_runtime_resource_factor=1, | ||
github_token=existing_settings.github_token, | ||
user_consents_to_analytics=existing_settings.user_consents_to_analytics, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just to note, hardcoding some defaults is a bit strange, as in unusual, arguably a duplicate and should be unnecessary... We had/have some of these in the /frontend
, but in the same time the backend had some schema for defaults for the API endpoint to use; then the backend had some methods to use to get the actual defaults from the configuration. (as defined in the dataclasses) . Anyway I think they were unused at some point, so we may have cleaned them both up from the codebase.
We actually do need the dataclass default values? Maybe! If we do need them, what if we were to get the real ones? WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah agreed we should have these defaults saved somewhere else. this line of code should probably just say reset_settings = Settings()
, which would pull in the defaults
Actually, scratch that... I think you're right these should be actual defaults. Just to pick your brain a bit here, I would propose that the understandable way this could work is:
Does that make sense? Does the saas use case need something different? |
Good points; if I am understanding correctly, you're suggesting to show the user-set settings (set either via the UI or something like I guess that would imply that the first-time defaults (if no settings exist) are from config toml. This would also mean that we are undoing the changes made in #6704 since the UI relies on a 404 to know whether to show the modal or not. For saas, the only requirement is that we can create a custom reset method. This is due to the fact that the default LLM API Key in saas is the one All Hands would provide to the user |
Yes, and we will need to save these in the json I think? Because if I reset, then reload the page, I guess it should continue to be defaults.
This sounds right. The behavior is nice though (opening Settings window), I know we wanted it for a long time to avoid some user confusion. Any chance we could hook it up to "some settings came from backend, but the api key is empty"? |
End-user friendly description of the problem this fixes or functionality that this introduces
Give a summary of what the PR does, explaining any non-trivial design decisions
This will make it easier to implement custom reset behaviour for different settings implementations (e.g., saas should reset to a base url / api key instead of clear it)
Link of any specific issues this addresses
To run this PR locally, use the following command: