Skip to content

Allow custom root directory for config/cache#5828

Closed
adrn wants to merge 15 commits intoastropy:masterfrom
adrn:config-custom-dir
Closed

Allow custom root directory for config/cache#5828
adrn wants to merge 15 commits intoastropy:masterfrom
adrn:config-custom-dir

Conversation

@adrn
Copy link
Member

@adrn adrn commented Feb 20, 2017

The idea here is to provide the option to put / look for config and cache files in a customizable directory (i.e. not default to ~/.astropy/pkgname). This would mainly be for packages that use Astropy but aren't planned to be affiliated packages. My own need for this is that I like the ConfigObj validation and the extra class-based ConfigItem functionality built on top, but I don't want my package to have config settings that live in ~/.astropy/XX.cfg. In looking through the config code, however, I'm wondering why we don't try contributing it back to the Python 3 port of ConfigObj itself? Most of the code is very general and not astronomy-specific...

I think the changes made here won't affect affiliated packages because the root path will default to .astropy.

This is probably still a work in progress because I think it still needs some more robust testing of the new functionality. But I need to figure out the best way to do that...

Fixes #5779.

cc @eteq

@adrn adrn changed the title [WIP] Allow custom root directory for config/cache Allow custom root directory for config/cache Feb 20, 2017
@pllim
Copy link
Member

pllim commented Feb 20, 2017

Also have to consider those packages who previously have to endure ~/.astropy/package.cfg until now. If they want to switch over to ~/.package/package.cfg but already using old config, what would be the least painful way?

Is it possible to do something like ~/.stsci/package.cfg? (Note the different parent directory name.)

@adrn
Copy link
Member Author

adrn commented Feb 20, 2017

@pllim To answer your 2nd question first, with this you would be able to use ~/.stsci/package.cfg by doing: get_config('package', rootname='stsci').

@adrn
Copy link
Member Author

adrn commented Mar 4, 2017

@eteq any suggestions for more tests to add in?

@bsipocz
Copy link
Member

bsipocz commented May 9, 2017

@adrn - how much is missing from here? Can we aim to get this in this week?

@adrn
Copy link
Member Author

adrn commented May 10, 2017

@bsipocz From my perspective, this is done! Just waiting for @eteq

image

@bsipocz
Copy link
Member

bsipocz commented May 10, 2017

@adrn - This needs a changelog entry.

@bsipocz bsipocz requested a review from eteq May 10, 2017 08:47
@adrn adrn force-pushed the config-custom-dir branch from b84d7c2 to edb47a8 Compare May 10, 2017 14:23
@adrn
Copy link
Member Author

adrn commented May 10, 2017

@bsipocz @eteq added a changelog entry and rebased.

@pllim
Copy link
Member

pllim commented May 10, 2017

Is there an associated PR over at package-template to take advantage of this?

Also, maybe mention this in http://docs.astropy.org/en/latest/development/affiliated-packages.html ?

@adrn
Copy link
Member Author

adrn commented May 11, 2017

@pllim no current PR but I will talk to @Cadair today about this.

BTW: I think the build failure is unrelated

@bsipocz
Copy link
Member

bsipocz commented May 11, 2017

@adrn - If you could push a minor comment into http://docs.astropy.org/en/latest/development/affiliated-packages.html, then we could merge this before coffee?

@adrn
Copy link
Member Author

adrn commented May 11, 2017

In reviewing this on my own, I realized that I think this requires some changes in https://github.com/astropy/astropy/blob/master/astropy/config/configuration.py as well. The package template seems to only interact with the config file directly through:

config.update_default_config(
                    __package__, config_dir, version=__version__)

Also: https://github.com/astropy/astropy/blob/master/astropy/config/configuration.py#L345

@bsipocz bsipocz added this to the v2.0.0 milestone May 16, 2017
@bsipocz
Copy link
Member

bsipocz commented Jun 16, 2017

@adrn - Could you clarify whether the changes are needed to be done here or in the package template. If the latter, is this ready to final review and merge?

@adrn
Copy link
Member Author

adrn commented Jun 16, 2017

I think this requires more changes or an assessment of whether this is still relevant - in either case, I don't think it will be ready in time for v2.0, so maybe we should re-milestone to v3.0? :/

@adrn adrn removed this from the v2.0.0 milestone Jun 16, 2017
@Cadair
Copy link
Member

Cadair commented Jul 11, 2017

@adrn I just went diving into the astropy config system to see if this could be done. Turns out you already kinda did it.

What exactly is the issue with this as it stands? Where can I help?

@adrn
Copy link
Member Author

adrn commented Jul 11, 2017

@Cadair This just needs to be rebased, and needs some better tests. Also at some point I started having Jeff Goldblum feelings about this PR, but if you'd find it useful I can try to wrap it up!

@Cadair
Copy link
Member

Cadair commented Jul 11, 2017

It would allow me to use the astropy config system for both SunPy and DKIST packages and could be nicely exposed in the package template as a config option, so SunPy affiliated packages would use .sunpy and astropy ones .astropy etc.

I am going to go and have a poke around until my desire to eat lunch becomes overwhelming.

@Cadair
Copy link
Member

Cadair commented Jul 11, 2017

What seems to be missing in this system is anyway to customise this behaviour from the level of ConfigNameSpace and ConfigItem (a ConfigItem seems to have no way to know what ConfigNameSpace it is attached to).

What could work is if there was a way for packages to subclass those two classes into their own package.config subpackage and then change the default root path etc, and then import from their own namespace rather than astropy's?

see adrn#16 for a very fast attempt at this idea.

@adrn adrn force-pushed the config-custom-dir branch from edb47a8 to 16a6b5a Compare September 1, 2017 17:35
@astropy-bot
Copy link

astropy-bot bot commented Sep 1, 2017

Hi there @adrn 👋 - thanks for the pull request! I'm just a friendly 🤖 that checks for issues related to the changelog and making sure that this pull request is milestoned and labeled correctly. This is mainly intended for the maintainers, so if you are not a maintainer you can ignore this, and a maintainer will let you know if any action is required on your part 😃.

I see this is an experimental pull request. I'll report back on the checks once the PR discussion in settled.

If there are any issues with this message, please report them here.

@adrn adrn added this to the Future milestone Sep 1, 2017
@bsipocz bsipocz removed this from the Future milestone Sep 25, 2017
@Cadair
Copy link
Member

Cadair commented Sep 27, 2017

@adrn I would still like to see this moved forwards, do you have any opinions on what is in my PR to your branch (now in need of rebasing) or any other opinions on where we should go with this?

@astrofrog astrofrog self-requested a review December 8, 2017 15:13
@bsipocz bsipocz added this to the v3.0.0 milestone Dec 8, 2017
@eteq
Copy link
Member

eteq commented Dec 13, 2017

@adrn @Cadair - some general thoughts on this: when originally designing the config machinery, we thought of it as a feature that everything is in .astropy: that way you don't get a zillion .<something> directories that you don't know how to interpret - anything that uses the astropy machinery goes into .astropy. In principal the user should never be seeing the cache directory anyway, and config still had pkgname.cfg to distinguish it from astropy.cfg. So the Jeff Goldblum question viewpoint may be a valid one...

That said, I could see the argument for the sunpy distinction, as that's a separate "ecosystem" on some level. So I'm not against this, but am also not sure it's a high priority? (But I see how it would be for @Cadair !) As @adrn said, though, this seems to need to touch config stuff more?

A separate note is the idea of setting the cache directory for cases where there's large data sets that the user actually wants to manage. There I see the use case a lot more. But that's not so much a "package"-level deal as it is a feature needed for the caching machinery (i.e., should be able to use the "default" cache vs a custom location).

@Cadair
Copy link
Member

Cadair commented Dec 14, 2017

I have rebased this and made #6985 if someone want's to pull it back in here, that's fine.

@adrn
Copy link
Member Author

adrn commented Dec 18, 2017

All yours @Cadair!

@bsipocz
Copy link
Member

bsipocz commented Apr 28, 2020

I run into this again today, and it seems we can change the config dir, but not the cache dir? If I misunderstand the situation, then we probably need to add more examples to the config dir, one especially for the case of the custom cache dir.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make astropy.config directory configurable by packages that use astropy_helpers

6 participants