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

Refactor config loading #185

Merged
merged 6 commits into from
Sep 5, 2024

Conversation

SecretiveShell
Copy link
Contributor

Why should this feature be added?
This PR refactors the way config files are loaded into tabbyAPI

It refactors the current functions to return a dictionary, reducing the use of the global variables.
A new load function is added to aggregate all the possible load config functions into a list of dictionaries
The global config is constructed by performing a deep merge on all the resulting configurations in the order they are listed
The individual functions to get the configuration are refactored to use a single factory, with nested functions using nested calls

Examples
This makes adding new sources of configuration much easier
The order of which configuration takes precedence when multiple competing settings are present is now much more obvious

- improve DRY
- alter logging
- allow extensibility
- add foundation for environment variables as config
@bdashore3
Copy link
Member

Good concept. However, I'd like a merge function in common/utils.py rather than needing to install a new package for a dict merge. I don't believe tabby will get complicated enough to warrant a full kit of merging dictionaries together.

Also, please run ruff format which should pass the PR checks. If ruff isn't installed, run pip install .[dev] in your venv and try again.

- remove deepmerge dependency
- fix ruff formatting
@SecretiveShell
Copy link
Contributor Author

I have added the internal implementation as requested

The config categories can have defined separation, but preserve
the dynamic nature of adding new config options by making all the
internal class vars as dictionaries.

This was necessary since storing global callbacks stored a state
of the previous global_config var that wasn't populated.

Signed-off-by: kingbri <[email protected]>
@bdashore3
Copy link
Member

bdashore3 commented Sep 5, 2024

This PR wasn't working when testing, but the concept was good. So, I decided to migrate the global config module into a shared class instance instead. This should preserve the flexibility of adding more config options while using class based references in other modules.

@SecretiveShell please take a look and I'll merge this in.

SecretiveShell and others added 2 commits September 5, 2024 15:33
- move to a complet class singleton to avoid propagation errors
- remove legacy load confing precedure
Rename some methods and change comments.

Signed-off-by: kingbri <[email protected]>
@bdashore3 bdashore3 merged commit ec7f64d into theroyallab:main Sep 5, 2024
1 check passed
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.

2 participants