ConfigManager class with update_config functionality

[btobers]

Closes #72.

ConfigManager class added to config.py, which now includes an update_config() function. This will allow for the advanced_test_tw notebook to be run as another test, as the include_frontalablation setting in ~/PyGEM/config.yaml can now be updated programmatically. Need to update all PyGEM-Community/PyGEM-notebooks to match. the new structure.

[btobers]

@ddundo Let me know what you think of this. I suspect that the current tests would fail right now, since the way the config.yaml file is read in in the test notebooks now needs updating. If I update the notebooks in PyGEM-notebooks and submit a related PR to the dev branch, would we be able to run the tests with those updated notebooks?

[btobers]

Closes #77

[btobers]

PyGEM-notebooks PR #12 updates all notebooks to work with the new ConfigManager class reading.

[ddundo]

Thanks for this @btobers! I made some initial comments but I think more changes will be necessary. I will also think about how to nicely test notebooks on non-main branches. I'll try to do that tomorrow.

Also, we should add tests whenever we add a new functionality. So for example, you could add a new file in pygem/tests called test_config.py which would have tests like this one:

def test_update_config():
    updates = {
        "sim.nsims": "5",
        "user.email": "updated@example.com",
        "constants.density_ice": "850"
    }
    
    config_manager = ConfigManager()  # or whatever needs to go here
    config_manager.update_config(updates)
    config = config_manager.read_config()
    
    assert config["sim"]["nsims"] == 5
    assert config["user"]["email"] == "updated@example.com"
    assert config["constants"]["density_ice"] == 850

so this tests that the update_config method does what it's supposed to do. And similarly for other methods. Then these will automatically be tested in the CI.

Let me know if I can help with this :)

[btobers]

Cleaned up config.py to remove some redundancy and add some rasie errors if certain conditions are met (e.g. don't allow user to update a dictionary key in the config file directly - subkeys must be specified). This should be all set. I like the idea of making a test for this. Perhaps it's also be good to add a method in the ConfigManager class that checks to make sure that the necessary keys exist. But this could get quite messy since there's so much in there.

[btobers]

@ddundo if you want to take another look at this, I cleaned up the ConfigManager class a little bit, adding error raising in the update_config function, and also created a simple config test function as you had mentioned. test_config. test_update_config () will test all the methods in the ConfigManager class (.__init__(), .ensure_config(), .crate_config(), .update_config(), .read_config()).

[ddundo]

Nice! There are still some kinks to work out, but already looks better :) And yeah I'd encourage you to write tests first since that will help you identify bugs too.

[btobers]

The expanded tests look nice - although I'm not sure the TypeErrors will be flexible enough given the current format of the configuration file. This is why I only had a few type checks in the update_config().

[ddundo]

If I update the notebooks in PyGEM-notebooks and submit a related PR to the dev branch, would we be able to run the tests with those updated notebooks?

#63 is now ready for review. When that gets merged, you could temporarily change test_suite.yml in this branch to do git clone -b 11_config_update --depth 1 https://github.com/pygem-community/PyGEM-notebooks.git. That will checkout the 11_config_update branch of PyGEM-notebooks repo and test the updates in the CI. Then before merging into dev just revert that back to how it was.

It's not the best solution, but I think it's fine for now. In the future we can discuss how to best handle this. For example, maybe we could add PyGEM-notebooks as a git submodule (see https://git-scm.com/book/en/v2/Git-Tools-Submodules).

[btobers]

If I update the notebooks in PyGEM-notebooks and submit a related PR to the dev branch, would we be able to run the tests with those updated notebooks?

#63 is now ready for review. When that gets merged, you could temporarily change test_suite.yml in this branch to do git clone -b 11_config_update --depth 1 https://github.com/pygem-community/PyGEM-notebooks.git. That will checkout the 11_config_update branch of PyGEM-notebooks repo and test the updates in the CI. Then before merging into dev just revert that back to how it was.

It's not the best solution, but I think it's fine for now. In the future we can discuss how to best handle this. For example, maybe we could add PyGEM-notebooks as a git submodule (see https://git-scm.com/book/en/v2/Git-Tools-Submodules).

Great, I'll do this after we Merge branch 72_config_tests with this branch.

[ddundo]

I left comments, after which I think it will be good to go :) There are a few comments about making some methods private. Private methods are essentially methods that we don't want users to use themselves.

Before addressing the comments, I'd suggest you merge dev into this branch to pick up the latest changes and to resolve the conflict it reports.

[btobers]

@ddundo all issues resolved. All tests passed, but then once I switched the git clone of PyGEM-notebooks in test_suite.yml back to the main branch

git clone -b 11_config_update --depth 1 https://github.com/pygem-community/PyGEM-notebooks.git

the notebook tests all fail. Any thoughts on how we should handle this? As of now, until we merge both repo's dev branches into main I think we'll fail these tests. Perhaps there's a way for the dev branch of PyGEM to clone the notebooks fom PyGEM-notebook's dev branch, and set PyGEM main to clone from the respective main branch of PyGEM-notebooks?

So the following for PyGEM main:
git clone --depth 1 https://github.com/pygem-community/PyGEM-notebooks.git

And then for PyGEM dev:
git clone -b dev --depth 1 https://github.com/pygem-community/PyGEM-notebooks.git

I don't know if there's a way to logic the test_suite.yml in a way such that the appropriate PyGEM-notebook repo is cloned.

[ddundo]

Looks good :)