Allow custom root directory for config by Cadair · Pull Request #8237 · astropy/astropy

Cadair · 2018-12-06T19:39:32Z

This PR makes three main changes to the way configuration files are handled, one is intended for affiliated package authors and two affect end users.

Allow the customisation of the directory name for configuration files, via the rootname argument. This allows affiliated package authors to use a directory like ~/.dkist/config/dkist.cfg rather than ~/.astropy/config/dkist.cfg
Provide a user facing helper (the previous API wasn't user facing but hasn't been changed) to be called if the user wants to write out the default config for subsequent manual editing.
Remove the code that automatically writes the template config to disk on import of astropy (fixes Consider providing template config file in docs instead of writing to disk #8552)

It also makes one minor change, get_config_dir now defaults to create=False so as to not create the config dir when a config file is not to be written to it.

This is a replacement for #6985

TODO:

Add documentation on writing and editing default config files.
Rebase this mess.

Cadair · 2018-12-06T19:58:24Z

So this seems to be working in DKISTDC/dkist#3

Cadair · 2018-12-06T21:15:02Z

Edit: This is now irrelevant.

~~@eteq I think this is almost ready to roll. The one bit I don't understand is how it interacts with this:~~

https://github.com/astropy/package-template/blob/cookiecutter/%7B%7B%20cookiecutter.package_name%20%7D%7D/%7B%7B%20cookiecutter.module_name%20%7D%7D/_%7B%7B%20cookiecutter._parent_project%20%7D%7D_init.py#L43-L60

bit of the package template. That code seems obnoxiously complex and if we have to ask affiliated package authors to modify it like I have here: https://github.com/DKISTDC/dkist/pull/3/files#diff-03100dba6855f116c1325d9e7258e54e then it might need a re-think.

astrofrog · 2018-12-07T14:59:38Z

Can a template version of that code not exist in astropy.config instead of in affiliated packages?

codecov · 2019-04-16T09:29:17Z

Codecov Report

Merging #8237 into master will decrease coverage by 0.16%.
The diff coverage is 88.05%.

@@            Coverage Diff             @@
##           master    #8237      +/-   ##
==========================================
- Coverage   87.13%   86.96%   -0.17%     
==========================================
  Files         408      400       -8     
  Lines       62061    59330    -2731     
  Branches     1096     1100       +4     
==========================================
- Hits        54077    51597    -2480     
+ Misses       7362     7092     -270     
- Partials      622      641      +19

Impacted Files	Coverage Δ
astropy/config/configuration.py	`84.07% <100%> (-0.04%)`	⬇️
astropy/utils/data.py	`80.93% <100%> (-1.27%)`	⬇️
astropy/config/paths.py	`70.68% <69.23%> (+1.33%)`	⬆️
astropy/visualization/wcsaxes/coordinates_map.py	`86.88% <0%> (-11.65%)`	⬇️
astropy/modeling/mappings.py	`84.31% <0%> (-8.79%)`	⬇️
astropy/convolution/src/convolve.c	`80.68% <0%> (-8.34%)`	⬇️
astropy/stats/jackknife.py	`89.28% <0%> (-7.39%)`	⬇️
astropy/wcs/src/str_list_proxy.c	`60.43% <0%> (-7.38%)`	⬇️
astropy/wcs/src/unit_list_proxy.c	`54.92% <0%> (-5.94%)`	⬇️
astropy/stats/histogram.py	`92% <0%> (-5.85%)`	⬇️
... and 201 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 297589c...a6c4169. Read the comment docs.

Cadair · 2019-04-16T09:50:13Z

Oh good, the tests seem to write the config to a different path on Python 3.5 🤦‍♀️

mhvk

Only had a glance, really, but like where this is going. Comments incomplete.

astropy/__init__.py

astropy/config/paths.py

astropy/config/tests/test_configs.py

astrofrog

Looks good to me! Other than the minor comments/questions below, could you add a small section to the narrative docs mentioning this capability? (http://docs.astropy.org/en/stable/config/index.html)

astropy/config/paths.py

astrofrog · 2019-04-17T08:59:12Z

astropy/utils/data.py

+    pkgname : `str`, optional
+        The package name to use to locate the download cache. i.e. for
+        ``pkgname='astropy'`` the default cache location is
+        ``~/.astropy/cache``.


Related to my previous comment, here you have ~ as the prefix for the config dir.

I am not entirely sure what the behaviour is here, so I think this is probably the best way to doc it.

Maybe instead of assuming it is ~/.astropy, crosslink to the config doc, since over there, it explains that user can further customize with XDG stuff?

To be honest I am going to open another PR after this which will change all the directories stuff anyway, so I don't really think it's worth going down the pit with.

Cadair · 2019-04-17T15:26:00Z

Ok I have now added docs to this, (I also removed the old 0.4 migration docs) and addressed all the comments. I think this is ready for review again.

pllim · 2019-04-18T17:56:57Z

For item 3 you mentioned above, are there unwanted side effects? Like, we added a new item or changed default of existing item but template no longer gets copied over, but user has an older version from last install, so user now accidentally using an outdated config?

pllim

Usage of both "config" and "configuration" interchangeably is confusing to read. Either do something like "configuration (hereafter, config)" and then only use "config" after, or stick to "configuration" unless you are referring to config as in a variable name.

For affiliated package maintainers like myself, I would like a section to help those who want to transition from ~/.astropy (as they had no choice before) to using their own root name now; i.e., what function to call differently in their own config, how they should tell their users about the switch (what old files to get rid of), and so on. Helper functions would be even better but probably out of scope here.

astropy/__init__.py

astropy/config/configuration.py

pllim · 2019-04-18T18:50:08Z

astropy/config/configuration.py

    return digest in known_configs


+def write_default_config(pkg, rootname=None):


I find it very confusing that this function has the same name as the one in __init__.py.

The purpose of this is to be overrideable for affiliated packages and the one in __init__.py just has the defaults set for the specific package.

astropy/config/tests/test_configs.py

astropy/utils/data.py

docs/config.rst

pllim · 2019-04-18T19:18:23Z

astropy/utils/data.py

+    pkgname : `str`, optional
+        The package name to use to locate the download cache. i.e. for
+        ``pkgname='astropy'`` the default cache location is
+        ``~/.astropy/cache``.


Maybe instead of assuming it is ~/.astropy, crosslink to the config doc, since over there, it explains that user can further customize with XDG stuff?

docs/config.rst

CHANGES.rst

Cadair · 2019-04-24T12:16:18Z

For item 3 you mentioned above, are there unwanted side effects? Like, we added a new item or changed default of existing item but template no longer gets copied over, but user has an older version from last install, so user now accidentally using an outdated config?

This behaviour is the same as before, currently if the user has changed their config a new template is written with the version number appended. This is the same behaviour as used here, just it happens explicitly when called rather than on import.

Cadair · 2019-04-24T13:38:47Z

@pllim I think I have addressed all your review comments now.

Cadair · 2019-04-24T15:09:56Z

The doc fail seems unrelated.

[ci skip]

Co-Authored-By: Brigitta Sipocz <b.sipocz@gmail.com>

Cadair · 2019-10-28T13:59:29Z

Ok, I finally fixed the stupid changelog.

astrofrog

Just confirming that this looks good to me

pllim · 2019-10-28T15:44:28Z

Shall we merge then?

bsipocz · 2019-10-28T16:11:29Z

Let's get on with this then.

Thank you @Cadair!

pllim requested a review from eteq December 6, 2018 20:13

pllim added the config label Dec 6, 2018

pllim added this to the v3.2 milestone Dec 6, 2018

pllim added utils whatsnew-needed labels Dec 6, 2018

pllim mentioned this pull request Apr 4, 2019

Consider providing template config file in docs instead of writing to disk #8552

Closed

Cadair force-pushed the config-custom-dir branch 2 times, most recently from 7ff652c to 5676dc6 Compare April 15, 2019 14:46

mhvk reviewed Apr 16, 2019

View reviewed changes

astropy/__init__.py Outdated Show resolved Hide resolved

astropy/config/paths.py Outdated Show resolved Hide resolved

astropy/config/paths.py Show resolved Hide resolved

astropy/config/tests/test_configs.py Show resolved Hide resolved

astrofrog requested changes Apr 17, 2019

View reviewed changes

bsipocz requested a review from astrofrog April 17, 2019 23:38

pllim self-requested a review April 18, 2019 17:00

pllim reviewed Apr 18, 2019

View reviewed changes

pllim modified the milestones: v3.2, v4.0 Apr 19, 2019

Cadair force-pushed the config-custom-dir branch from b7bf526 to b49a18c Compare April 24, 2019 10:14

astrofrog reviewed Apr 24, 2019

View reviewed changes

CHANGES.rst Outdated Show resolved Hide resolved

Cadair force-pushed the config-custom-dir branch from 08e0697 to 6319363 Compare April 24, 2019 14:43

Cadair force-pushed the config-custom-dir branch 2 times, most recently from db42f39 to 5dd0285 Compare April 25, 2019 10:59

Cadair and others added 16 commits October 28, 2019 09:57

rootname as class variables

31e764b

Fix rootname kwarg passing

35d707f

some small lint improvements

f5dcd3b

fix rebase issues

f0b4504

add docstring

f819d7c

only need rootname on config item

6b5dd89

fix test_configs

0ab07ec

attempt 2 at fixing the inconsistent test patterns

ac9b535

update changelog [ci skip]

56edc70

Preserve the previous API

4ebcab2

[ci skip]

remove unused create argument

f04ada4

Add some documentation

6058585

[ci skip]

rebase fix

4023678

Flake

09361ea

Fix doc build

a105cc4

Update docs/config/index.rst

d98b06d

Co-Authored-By: Brigitta Sipocz <b.sipocz@gmail.com>

Cadair force-pushed the config-custom-dir branch from 3b1b136 to d98b06d Compare October 28, 2019 13:58

astrofrog changed the title ~~Allow custom root directory for config (Mk III)~~ Allow custom root directory for config Oct 28, 2019

astrofrog approved these changes Oct 28, 2019

View reviewed changes

bsipocz merged commit b8327d6 into astropy:master Oct 28, 2019

pllim mentioned this pull request Oct 28, 2019

Move the location of synphot.cfg ? spacetelescope/synphot_refactor#225

Closed

aarchiba added a commit to aarchiba/astropy that referenced this pull request Oct 30, 2019

Correct other PR: astropy#8237 [skip ci]

b866b0b

This was referenced Oct 30, 2019

Consider using appdirs for config #8245

Closed

MNT: Move change logs, fix test_earth.py AttributeError #9510

Merged

pllim mentioned this pull request Dec 3, 2019

Fix bug in temporary path setting (was: Clear up cache-related warnings on Windows) #9703

Merged

maxnoe pushed a commit to maxnoe/astropy that referenced this pull request Jan 6, 2020

Correct other PR: astropy#8237 [skip ci]

e97c0f7

rainwoodman mentioned this pull request Mar 16, 2020

Concurrent import of astropy occasionally fails with likely race condition on config files. #7290

Closed

		return digest in known_configs


		def write_default_config(pkg, rootname=None):

Uh oh!

Conversation

Cadair commented Dec 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Cadair commented Dec 6, 2018

Uh oh!

Cadair commented Dec 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

astrofrog commented Dec 7, 2018

Uh oh!

codecov bot commented Apr 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Cadair commented Apr 16, 2019

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

astrofrog left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

astrofrog Apr 17, 2019

Choose a reason for hiding this comment

Uh oh!

Cadair Apr 17, 2019

Choose a reason for hiding this comment

Uh oh!

pllim Apr 18, 2019

Choose a reason for hiding this comment

Uh oh!

Cadair Apr 24, 2019

Choose a reason for hiding this comment

Uh oh!

Cadair commented Apr 17, 2019

Uh oh!

pllim commented Apr 18, 2019

Uh oh!

pllim left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pllim Apr 18, 2019

Choose a reason for hiding this comment

Uh oh!

Cadair Apr 19, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pllim Apr 18, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Cadair commented Apr 24, 2019

Uh oh!

Cadair commented Apr 24, 2019

Uh oh!

Cadair commented Apr 24, 2019

Uh oh!

Cadair commented Oct 28, 2019

Uh oh!

astrofrog left a comment

Choose a reason for hiding this comment

Uh oh!

pllim commented Oct 28, 2019

Uh oh!

bsipocz commented Oct 28, 2019

Cadair commented Dec 6, 2018 •

edited

Loading

Cadair commented Dec 6, 2018 •

edited

Loading

codecov bot commented Apr 16, 2019 •

edited

Loading