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

Cadair · 2017-12-14T18:03:50Z

This is an adoption of #5828 because I think I have more reason to want it than @adrn.

I have rebased this for 3.0+ (so it's Python 3 only now) and I have also opened astropy/package-template#290 which demonstrates the changes needed to use this in the template. Also I have used this successfully in DKISTDC/dkist#3 to prove it works.

@eteq opinions?

EDIT: Fix #5779 , close #5828

astropy-bot · 2017-12-14T18:03:52Z

Hi there @Cadair 👋 - 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 noticed the following issue with this pull request:

Changelog entry section (v3.0) inconsistent with milestone (v3.1)

Would it be possible to fix this? Thanks!

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

pllim · 2017-12-14T19:54:01Z

CHANGES.rst

+    customizable. This allows affiliated packages to put their configuration
+    files in locations other than ``CONFIG_DIR/.astropy/``. [#5828]
+
+  - The config and cache directories and the name of the config file are now


Duplicate change log entry. Also needs to be updated to include this PR number.

pllim · 2017-12-14T19:54:25Z

CHANGES.rst

 ^^^^^^^^^^^

 - Removed deprecated ``wcs.rotateCD``. [#6170]
+- The bundled version of pytest has now been removed, but the


This entry seems unrelated to the PR.

pllim · 2017-12-14T20:00:35Z

astropy/config/configuration.py

        conf = Conf()
    """
+
+    rootname = 'astropy'


This is defined again in ConfigItem. Is this necessary here? Seems weird to define this twice separately.

Different bits of the machinery use ConfigItem and ConfigNamespace I could not see a simple way of sharing the value.

although maybe this isn't actually used anywhere... 😕

pllim · 2017-12-14T20:02:46Z

astropy/utils/data.py

        The timeout, in seconds.  Otherwise, use
        `astropy.utils.data.Conf.remote_timeout`.

+    pkgname : TODO


This docstring is incomplete.

pllim · 2017-12-14T20:04:47Z

astropy/utils/data.py



-def download_files_in_parallel(urls, cache=True, show_progress=True,
+def download_files_in_parallel(urls, cache=False, show_progress=True,


This change in behavior seems unrelated to this PR.

pllim · 2017-12-14T20:04:59Z

astropy/utils/data.py


 def _do_download_files_in_parallel(args):
-    return download_file(*args)
+    return download_file(*args, show_progress=False)


This change in behavior seems unrelated to this PR.

pllim · 2017-12-14T20:05:51Z

astropy/utils/data.py

    else:
        progress = io.BytesIO()

+    if timeout is None:


This change in behavior seems unrelated to this PR.

Cadair · 2017-12-14T21:30:48Z

looks like @pllim just found a whole bunch of my rebase fails 😆

:

Cadair · 2017-12-21T12:35:33Z

I think all the major wrinkles in this are iorned out. Review please!

Cadair · 2017-12-21T12:49:41Z

That's curious, I can't reproduce the Circle CI fail locally. That is a post-lunch problem.

adrn · 2017-12-22T22:21:15Z

Given that I wrote some fraction of this code, it would be good for @eteq to review this 😄

eteq

I guess I'm willing to accept this in concept since it's clear SunPy has a need for it.

The one high-level concern I have on this is that setting the cache consistently is quite awkward. But I guess I'm ok with that because it will still keep people from having more than one location? I think for the cache it's harder to understand why I would even want this, since the point of a cache is to save things for re-use...

But aside from the inline comments I made about an API-breaking change that shouldn't be there, one other thing is missing: docs explaining how to use this! I would put most of the discussion in http://docs.astropy.org/en/stable/config/index.html and then some mention in http://docs.astropy.org/en/stable/utils/index.html#module-astropy.utils.data about the cache directory

eteq · 2017-12-23T23:39:12Z

astropy/config/paths.py



-def get_config_dir(create=True):
+def get_config_dir(rootname, create=True):


This is a public API function so this is a breaking API change. I think rootname needs to default to astropy, and be the second argument. I realize this isn't used that much, but I think it is used by some affiliated packages that use tricks to do something else.

eteq · 2017-12-23T23:40:04Z

astropy/config/tests/test_configs.py

-    assert 'astropy' in paths.get_config_dir()
-    assert 'astropy' in paths.get_cache_dir()
+    assert 'astropy' in paths.get_config_dir('astropy')
+    assert 'astropy' in paths.get_cache_dir('astropy')


Related to the above: this test has to be left unchanged to not be a public API-breaking change. Same with all the stuff below in this file.

eteq · 2017-12-23T23:48:27Z

astropy/config/paths.py



-def get_cache_dir():
+def get_cache_dir(rootname):


Same deal here with it being an API-breaking change. Needs to default to 'astropy'

astrofrog · 2018-07-18T21:10:25Z

There was a bug in the bot, it should not have been closed without the warning so re-opening.

Cadair · 2018-07-19T10:27:53Z

I will get back to this one day...

astropy-bot · 2018-07-19T15:32:55Z

Hi humans 👋 - this pull request hasn't had any new commits for approximately 6 months. I plan to close this in a month if the pull request doesn't have any new commits by then.

In lieu of a stalled pull request, please consider closing this and open an issue instead if a reminder is needed to revisit in the future. Maintainers may also choose to add keep-open label to keep this PR open but it is discouraged unless absolutely necessary.

If this PR still needs to be reviewed, as an author, you can rebase it to reset the clock. You may also consider sending a reminder e-mail about it to the astropy-dev mailing list.

If you believe I commented on this pull request incorrectly, please report this here.

astropy-bot · 2018-08-23T12:59:15Z

⏰ Time's up! ⏰

I'm going to close this pull request as per my previous message. If you think what is being added/fixed here is still important, please remember to open an issue to keep track of it. Thanks!

If this is the first time I am commenting on this issue, or if you believe I closed this issue incorrectly, please report this here.

This was referenced Dec 14, 2017

[WIP] Add basic machinery for custom config dir astropy/package-template#290

Closed

Allow custom root directory for config/cache #5828

Closed

pllim requested review from adrn and eteq December 14, 2017 19:50

pllim added config Enhancement labels Dec 14, 2017

pllim added this to the v3.0.0 milestone Dec 14, 2017

pllim added the whatsnew-needed label Dec 14, 2017

pllim reviewed Dec 14, 2017

View reviewed changes

adrn and others added 13 commits December 21, 2017 12:02

initial attempt

e408718

typo

0c3a107

another typo in error msg

6820b52

add option to specify root dir

00ac5cd

update function needs to default to 'astropy'

a889b43

add some very simple unit tests of new functionality

9f88575

fix var names and docstrings

0d97899

change logic for setting rootname

37191d2

add comment

e288021

add some tests of paths

c8a61ec

added changelog and rebased

7dc72bb

:

rootname as class variables

5c99744

Fix rootname kwarg passing

2e4a82e

Cadair force-pushed the config-custom-dir branch 2 times, most recently from 1a394ec to 8b94857 Compare December 21, 2017 12:23

Cadair added 3 commits December 21, 2017 12:24

some small lint improvements

9cf679e

fix rebase issues

1f0836b

add docstring

6fb60a3

Cadair force-pushed the config-custom-dir branch from 8b94857 to 6fb60a3 Compare December 21, 2017 12:24

only need rootname on config item

5eb8d1f

Cadair added a commit to Cadair/package-template that referenced this pull request Dec 21, 2017

updates for latest changes to astropy/astropy#6985

f4539e2

bsipocz added the Ready-for-final-review label Dec 21, 2017

Cadair added 2 commits December 21, 2017 15:21

fix test_configs

0728501

attempt 2 at fixing the inconsistent test patterns

2ad7fc9

eteq requested changes Dec 23, 2017

View reviewed changes

bsipocz modified the milestones: v3.0.0, v3.1 Jan 18, 2018

eteq removed the Ready-for-final-review label Feb 3, 2018

astropy-bot bot closed this Jul 18, 2018

astrofrog reopened this Jul 18, 2018

astrofrog added the keep-open label Jul 18, 2018

astropy deleted a comment from astropy-bot bot Jul 19, 2018

astrofrog removed the keep-open label Jul 19, 2018

astropy-bot bot closed this Aug 23, 2018

bsipocz added the closed-by-bot Closed by stale bot label Aug 23, 2018

Cadair mentioned this pull request Dec 6, 2018

Allow custom root directory for config #8237

Merged

2 tasks

pllim mentioned this pull request Apr 30, 2020

Use user-specified custom cache directory #10244

Open



		def download_files_in_parallel(urls, cache=True, show_progress=True,
		def download_files_in_parallel(urls, cache=False, show_progress=True,



		def get_config_dir(create=True):
		def get_config_dir(rootname, create=True):

Uh oh!

Conversation

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

Uh oh!

Uh oh!

astropy-bot bot commented Dec 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Cadair commented Dec 14, 2017

Uh oh!

Cadair commented Dec 21, 2017

Uh oh!

Cadair commented Dec 21, 2017

Uh oh!

adrn commented Dec 22, 2017

Uh oh!

eteq left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

astrofrog commented Jul 18, 2018

Uh oh!

Cadair commented Jul 19, 2018

Uh oh!

astropy-bot bot commented Jul 19, 2018

Uh oh!

astropy-bot bot commented Aug 23, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Cadair commented Dec 14, 2017 •

edited

Loading

astropy-bot bot commented Dec 14, 2017 •

edited

Loading

eteq left a comment •

edited

Loading