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

2164 update config from list of variables to class #2168

Merged
merged 88 commits into from
Oct 14, 2022

Conversation

lucas-wilkins
Copy link
Contributor

@lucas-wilkins lucas-wilkins commented Aug 20, 2022

This PR is for a big housekeeping effort surrounding the configuration system.

It does the following things:

  1. A new folder called system that replaces the custom_configs/local_config system which contains:
    a) config directory, which contains a class called config which controls configuration variables, config_meta describes the behaviour of this class, including loading and saving. More fully documented in the config class pramble.
    b) web a class containing all the urls and email addresses
    c) legal a class containing any legal stuff, currently just the copyright notice
    d) env a utility class for dealing with environment variables
    e) log a relocation of logger_config.py
    Config settings are now all defined though config.py and runtime changes to these variables are written to a local config.json file on closing (which is loaded on startup)
    There is now no writing python files, and variable usage is much more easily followed
  2. Removal of config items that are no longer used, there were a lot of these.
  3. Simplification of the way configuration data is utilised.

Things that probably need the most attention when reviewing:

  1. Do the GPU settings still integrate properly with your hardware and with sasmodels? (@pkienzle ?)
  2. Places issues might occur causing silent errors e.g. in fitting methods where config is used?
  3. Have I changed the meaning of anything, or logic of anything that was handled by the local/custom config system?
  4. Have I broken anything on other OSs than windows?

Note: this PR addresses multiple issues (linked) which will need to be manually closed.

@lucas-wilkins lucas-wilkins linked an issue Aug 20, 2022 that may be closed by this pull request
@wpotrzebowski wpotrzebowski added the Discuss At The Call Issues to be discussed at the fortnightly call label Aug 29, 2022
@wpotrzebowski wpotrzebowski removed the Discuss At The Call Issues to be discussed at the fortnightly call label Aug 30, 2022
Copy link
Contributor

@krzywon krzywon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking through this, I have a few comments, but the direction you are taking looks good. My related PR (#2167) currently uses get_custom_config which you are deprecating, but that looks like the only clash we should have once you're finished.

src/sas/sasview/custom_config.py Show resolved Hide resolved
installers/installer_generator.py Outdated Show resolved Hide resolved
installers/installer_generator.py Outdated Show resolved Hide resolved
installers/installer_generator64.py Outdated Show resolved Hide resolved
src/sas/__init__.py Outdated Show resolved Hide resolved
src/sas/__init__.py Outdated Show resolved Hide resolved
@lucas-wilkins
Copy link
Contributor Author

Status update:

I've finished making the config class, the next task is look the variables within it, throw out unused ones, replace bad uses of a config with more proper usages etc. Basically I've done steps 1, 2, 5 and 7 from my initial list.

Appropriate loading and saving on startup is yet to be implemented, basically because I'm not sure about where the user config file (which is now the only config file that should exist) should live.

There is pretty decent test coverage for these changes.

@krzywon

Copy link
Contributor

@pkienzle pkienzle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly suggestions that you can ignore.

You should sort out the circular reference between setup logger and config.load, preferably by moving config.load after setup logger and out of the root sas namespace.

There are still some imports of sas.sasview that can be removed.

build_tools/release_automation.py Show resolved Hide resolved
build_tools/release_automation.py Show resolved Hide resolved
src/sas/qtgui/MainWindow/AboutBox.py Outdated Show resolved Hide resolved
src/sas/qtgui/MainWindow/AboutBox.py Show resolved Hide resolved
src/sas/qtgui/MainWindow/MainWindow.py Outdated Show resolved Hide resolved
src/sas/system/log.py Outdated Show resolved Hide resolved
src/sas/system/log.py Show resolved Hide resolved
src/sas/system/config/config_meta.py Outdated Show resolved Hide resolved
src/sas/system/config/config_meta.py Outdated Show resolved Hide resolved
src/sas/qtgui/Utilities/FileConverter.py Outdated Show resolved Hide resolved
@lucas-wilkins
Copy link
Contributor Author

@pkienzle @krzywon Will merge this tomorrow if there are no further changes.

@lucas-wilkins
Copy link
Contributor Author

lucas-wilkins commented Sep 28, 2022 via email

@pkienzle
Copy link
Contributor

I moved the discussion on SAS_OPENCL to #2226

@lucas-wilkins lucas-wilkins mentioned this pull request Sep 30, 2022
@wpotrzebowski wpotrzebowski removed the Discuss At The Call Issues to be discussed at the fortnightly call label Oct 4, 2022
@lucas-wilkins
Copy link
Contributor Author

@krzywon @krzywon Right, I think its ready!

@wpotrzebowski wpotrzebowski added the Discuss At The Call Issues to be discussed at the fortnightly call label Oct 10, 2022
@butlerpd
Copy link
Member

@krzywon will take a final look.

@butlerpd butlerpd removed the Discuss At The Call Issues to be discussed at the fortnightly call label Oct 11, 2022
src/sas/qtgui/MainWindow/MainWindow.py Outdated Show resolved Hide resolved
src/sas/system/config/config_meta.py Show resolved Hide resolved
src/sas/system/config/config_meta.py Show resolved Hide resolved
@lucas-wilkins lucas-wilkins merged commit 28a9005 into main Oct 14, 2022
@krzywon krzywon mentioned this pull request Oct 21, 2022
@krzywon krzywon deleted the 2164-update-config-from-list-of-variables-to-class branch October 26, 2022 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Housekeeping Tidying, renaming, formatting, minor refactoring, boring stuff in general Major Big change in the code or important change in behaviour
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update config from list of variables to class
5 participants