Migrate config to toml #2981

cphyc · 2020-11-25T12:56:10Z

PR Summary

PR Checklist

pass black --check yt/
pass isort . --check --diff
pass flake8 yt/
pass flynt yt/ --fail-on-change --dry-run -e yt/extern
New features are documented, with docstrings and narrative docs
Adds a test for any bugs fixed. Adds tests for new features.

neutrinoceros · 2020-11-25T13:01:26Z

looks like a lot of the changes are just flynt running lose on documentation (which was not done already, to my surprise ?). It's nice but I maybe it's out of scope here, or am I missing something ?

matthewturk

I like this. One question I had, since we're in a mood to add the occasional dependency, is if there is a config library out there (that perhaps we even have a transitive dependence on) that would be a good idea to migrate to.

matthewturk · 2020-11-25T15:19:04Z

yt/config.py

+    def get(self, section, *path, strict=True, **kwargs):
+        config = self.values[section]
+
+        # This works as follow: if we try to access


Does this leave open the opportunity to do frontend-specific config?

It does! You can actually do something like

matthewturk · 2020-11-25T15:19:20Z

yt/config.py

+        if len(path) == 0:
+            return config
+
+        ok = False


wow this statement is a real mood, I have to say

Changed it, sorry!

matthewturk · 2020-11-25T15:23:19Z

yt/config.py

+    ytcfg.read(CURRENT_CONFIG_FILE)
+cwd = Path(".").absolute()
+while cwd.parent != cwd:
+    cfg_file = cwd / "yt.toml"


love pathlib

matthewturk · 2020-11-25T15:24:08Z

yt/config.py

+    if cfg_file.exists():
+        ytcfg.read(cfg_file)
+        break
+    cwd = cwd.parent


I'm looking at this and trying to figure out if it's possible to get to the root, or to get into an infinite loop, and I can't quite tell. Is it ever possible we'll see an exception for trying to get to the parent of a non-parented path?

I think the code is correct, but I'll let others weigh in.

I updated this bit as it was causing me headache to make sure it will work in any case.

yt/config.py

cphyc · 2020-11-25T16:40:29Z

looks like a lot of the changes are just flynt running lose on documentation (which was not done already, to my surprise ?). It's nice but I maybe it's out of scope here, or am I missing something ?

I've moved them to #2983

cphyc · 2020-11-25T17:17:14Z

I like this. One question I had, since we're in a mood to add the occasional dependency, is if there is a config library out there (that perhaps we even have a transitive dependence on) that would be a good idea to migrate to.

I could not find any library that was relying on toml, but given the size of our configuration, this may be a bit overkill. Currently, the config manager repropduces part of the ConfigParser API.

neutrinoceros · 2020-11-25T18:13:16Z

I have a general question here: is it still relevant to store the global config file in $HOME/.config/yt/yt.toml rather than $HOME/.config/yt.toml ? what else is expected to be there ?

neutrinoceros

Still 2 files to go but here are my first comments. I'll finish this tomorrow !

doc/source/reference/command-line.rst

doc/source/reference/configuration.rst

yt/config.py

yt/mods.py

yt/utilities/parallel_tools/parallel_analysis_interface.py

yt/utilities/command_line.py

neutrinoceros

some more minor comments. I'll approve when we converge on the global VS local config file discussion :-)

yt/utilities/configuration_tree.py

Xarthisius · 2020-12-01T14:55:09Z

@yt-fido test this please

yt/config.py

yt/utilities/command_line.py

doc/source/reference/changelog.rst

doc/source/reference/command-line.rst

doc/source/reference/configuration.rst

yt/__init__.py

Co-authored-by: Clément Robert <cr52@protonmail.com>

cphyc · 2021-01-11T13:41:19Z

@munkm any chance you review this? Otherwise, I'm fine with it being merged in.

pep8speaks · 2021-01-21T09:10:40Z

Hello @cphyc! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2021-01-21 13:11:09 UTC

for more information, see https://pre-commit.ci

cphyc · 2021-01-21T13:21:54Z

The Jenkins failure are due to the fact Jenkins does not install the requirements listed in setup.py, which now include toml as a dependency. This should probably be fixed on Jenkins' side rather than in this PR (by installing those requirements).

matthewturk · 2021-01-21T13:23:39Z

@Xarthisius Can that be fixed without your intervention?

I think otherwise this may be ready to go, once the tests pass.

neutrinoceros · 2021-01-21T18:01:37Z

Following the merging of #2981 , I need to patch this with the new deprecation utilities. Then I believe it will finally be ready for going in :)

brittonsmith · 2021-01-21T19:18:01Z

I'll merge this when the tests finish.

matthewturk approved these changes Nov 25, 2020

View reviewed changes

neutrinoceros reviewed Nov 25, 2020

View reviewed changes

yt/config.py Outdated Show resolved Hide resolved

neutrinoceros reviewed Nov 25, 2020

View reviewed changes

yt/config.py Outdated Show resolved Hide resolved

cphyc force-pushed the migrate-config-to-toml branch from c37e1e5 to 4b708ef Compare November 25, 2020 16:39

cphyc force-pushed the migrate-config-to-toml branch from 0623390 to e980756 Compare November 25, 2020 16:55

cphyc added backwards incompatible This change will change behavior enhancement Making something better infrastructure Related to CI, versioning, websites, organizational issues, etc yt-4.0 feature targeted for the yt-4.0 release labels Nov 25, 2020

cphyc marked this pull request as ready for review November 25, 2020 17:18

cphyc mentioned this pull request Nov 26, 2020

Per field config #1931

Merged

6 tasks

neutrinoceros reviewed Nov 26, 2020

View reviewed changes

neutrinoceros reviewed Nov 27, 2020

View reviewed changes

cphyc commented Nov 27, 2020

View reviewed changes

yt/utilities/configuration_tree.py Show resolved Hide resolved

cphyc force-pushed the migrate-config-to-toml branch from 8670ee1 to fc943de Compare December 16, 2020 16:35