Add function to generate the configuration file by saimn · Pull Request #10148 · astropy/astropy

saimn · 2020-04-15T03:47:22Z

Ref #10146. This was fun and quick enough to do, but I had to hard-code the list of modules to import, not sure if there is a more clever (and simple) solution ?

Result: https://gist.github.com/saimn/b01966dbc6cef2c8c849ac4b933e84d3
This is almost the same as astropy.cfg, the order of subpackages is slightly different (alphabetical with the function here), and the only thing missing is the manual subsections like ### CORE DATA STRUCTURES AND TRANSFORMATIONS.

Next steps:

Find a way to include the output in the docs
Should this be used to update astropy.cfg occasionally, or replace it ?
Replace astropy.cfg is not so easy since it is used as a template for the default config file, which is checking while importing astropy. But the solution here takes a bit of time since it needs to import all modules with config items...

EDIT: Fix #10146

pllim · 2020-04-15T12:52:52Z

A quick grep shows you where they are defined:

astropy/config/configuration.py:92:        conf = Conf()
astropy/io/fits/__init__.py:62:conf = Conf()
astropy/io/votable/__init__.py:36:conf = Conf()
astropy/logger.py:87:conf = Conf()
astropy/nddata/__init__.py:51:conf = Conf()
astropy/samp/__init__.py:39:conf = Conf()
astropy/table/jsviewer.py:30:conf = Conf()
astropy/table/__init__.py:45:conf = Conf()  # noqa
astropy/units/quantity.py:59:conf = Conf()
astropy/utils/data.py:95:conf = Conf()
astropy/utils/iers/iers.py:145:conf = Conf()
astropy/visualization/wcsaxes/__init__.py:41:conf = Conf()
astropy/__init__.py:134:conf = Conf()

Re: Replacing astropy.cfg -- Let's not get too ambitious right now, especially if we decide on not using it by default anymore as mentioned in #10090 (comment) . Also even if we want to replace it, how is it going to be generated in a way that is compatible with the various way that people might be installing it? Easiest option is maybe add a step for the release maintainer to do, as with updating the leap seconds table (ref).

mhvk · 2020-04-15T14:35:21Z

This looks very good! My own sense would be to try to aim to have it auto-included in the documentation. If that's done at the end, we don't even have to worry about importing astropy modules...

bsipocz · 2020-04-16T03:59:52Z

I have this stupid little script here: https://github.com/astropy/ci-helpers/blob/master/utils/import_submodules.py that maybe of use here, too.

(I use it in a WIP PR: https://github.com/astropy/astropy/pull/7952/files#diff-b91f3d5bd63fcd17221b267e851608e8R94)

saimn · 2020-04-16T04:04:55Z

@bsipocz - thanks, I just pushed something using pkgutil.walk_packages, and just skipping test packages.

saimn · 2020-04-21T02:44:34Z

So I added a very simple Sphinx extension to include directly the generated config in the docs:
https://64628-2081289-gh.circle-artifacts.com/0/docs/_build/html/config/astropy_config.html#astropy-config-file
with a link in https://64628-2081289-gh.circle-artifacts.com/0/docs/_build/html/config/index.html#getting-started

Should this extension go in sphinx-astropy ?
Should I update the astropy.cfg file for now ?

pllim · 2020-04-21T03:15:20Z

Should this extension go in sphinx-astropy ?

This extension only works for the core library, right? Should it just stay here like it is already in the PR?

saimn · 2020-04-21T03:52:49Z

Fine by me 👍

bsipocz · 2020-04-21T04:48:28Z

I see that we might want to use something like this for astroquery where we have configs for all subpackages, but keeping it in core for now sounds reasonable.

mhvk

This looks very nice! I'll leave the question about where this should live to @astrofrog, but think starting in astropy certainly makes sense. Only one suggestion for making the creation a bit more elegant.

pllim

So, this needs astropy/sphinx-astropy#30 for the HTML build to work properly, right?

Maybe add an entry to releasing procedures to update the astropy.cfg with this function before a release, near the instruction to update leap seconds table?

Overall, LGTM! I think with this in, the risk of the default astropy.cfg falling behind will decrease significantly. I also really appreciate the extra work that went into make the function useful for affiliated packages down the road. Thanks!

I'll let the sphinx-astropy dependency question get sorted out first before approving.

pllim · 2020-04-24T17:53:41Z

@@ -1,144 +1,169 @@
-# -*- coding: utf-8 -*-


coding is removed. Does it affect anything?

It was mostly an issue with Python 2, but with Python 3 it should be ok:

If no encoding declaration is found, the default encoding is UTF-8.
https://docs.python.org/3/reference/lexical_analysis.html#encoding-declarations

pllim · 2020-04-24T18:04:13Z

+        for mod in pkgutil.walk_packages(path=package.__path__,
+                                         prefix=package.__name__ + '.'):
+
+            if mod.module_finder.path.endswith('tests'):


Just in case?

Suggested change

if mod.module_finder.path.endswith('tests'):

if mod.module_finder.path.endswith(('tests', 'test')):

pllim · 2020-04-24T18:06:10Z

+                    # module name, but not if this is the root package...
+                    if item.module != pkgname:
+                        modname = item.module.replace(f'{pkgname}.', '')
+                        fp.write(f"[{modname}]\n\n")


Is it safer to use os.linesep instead of "\n"?

open takes care of this by default:

On input, if newline is None, universal newlines mode is
enabled. Lines in the input can end in '\n', '\r', or '\r\n', and
these are translated into '\n' before being returned to the
caller. [...]

On output, if newline is None, any '\n' characters written are
translated to the system default line separator, os.linesep.

Ah, that's so good to know!

pllim · 2020-04-24T18:07:19Z

+    with open(outfile) as fp:
+        conf2 = fp.read()
+
+    for c in (conf, conf2):


Maybe a comment to explain why these specific lines are selected for testing? Were they arbitrarily selected or were there reasons?

Just a random selection 😄

Added a comment.

pllim

LGTM! Since this depends on new feature in sphinx-astropy, I'll let @bsipocz and/or @astrofrog figure out when to merge.

Thanks!

bsipocz · 2020-04-28T03:43:01Z

Yes, merging this should wait until the sphinx-astropy release is out. Alternatively we can add the dev version as dependency in the meantime and wait with the release until the end of the RC testing period. Either would work equally well for me.

There is one more thing, we'll need to add a new minimum sphinx-astropy version requirement, otherwise the docs build will fail with the missing directive error.

astrofrog · 2020-04-28T17:23:05Z

I've just released sphinx-astropy 1.3! Can you try adding that as a minimum requirement if we are going to be relying on this new plugin? (and this will also trigger a CI build)

saimn · 2020-04-28T18:20:34Z

It works: https://66219-2081289-gh.circle-artifacts.com/0/docs/_build/html/config/astropy_config.html#astropy-config-file

bsipocz · 2020-04-28T19:47:21Z

Thank you @saimn, this looks awesome now!

saimn added config Work in progress labels Apr 15, 2020

bsipocz reviewed Apr 15, 2020

View reviewed changes

Comment thread astropy/config/configuration.py Outdated

pllim requested a review from eteq April 15, 2020 12:45

pllim reviewed Apr 15, 2020

View reviewed changes

Comment thread astropy/config/configuration.py Outdated

pllim reviewed Apr 15, 2020

View reviewed changes

Comment thread astropy/config/configuration.py Outdated

pllim requested a review from mhvk April 15, 2020 13:00

This comment has been minimized.

Sign in to view

pllim reviewed Apr 21, 2020

View reviewed changes

Comment thread astropy/config/configuration.py Outdated

mhvk reviewed Apr 21, 2020

View reviewed changes

Comment thread astropy/config/configuration.py Outdated

Comment thread docs/ext/astropy_config.py Outdated

saimn force-pushed the generate-config branch from 3ec32f1 to df1e889 Compare April 23, 2020 03:51

saimn removed the Work in progress label Apr 23, 2020

saimn added this to the v4.1 milestone Apr 23, 2020

This comment has been minimized.

Sign in to view

saimn force-pushed the generate-config branch 2 times, most recently from cad1fbd to a0c8eb9 Compare April 23, 2020 21:45

saimn mentioned this pull request Apr 24, 2020

Add extension to include generated config in the docs astropy/sphinx-astropy#30

Merged

pllim reviewed Apr 24, 2020

View reviewed changes

pllim approved these changes Apr 25, 2020

View reviewed changes

saimn and others added 17 commits April 28, 2020 13:58

Add function to generate the configuration file

88eaa48

Rename and allow file objects as input

2508498

Import all submodules

dbe000e

Ignore deprecation warnings

bb7f372

Add basic sphinx extension to include config in the docs [docs only]

f4b4062

Make the function generic, should work with other packages

185d2e2

Add test

654cca1

Use contextlib.ExitStack

72eb486

Update directive to take the package as argument

6a40dfc

Fix windows error

2059ddf

Skip private modules

6a6ed3e

Remove Sphinx extension, moved to sphinx-astropy

c591a49

Enhance docstring

2bdfe17

Update default config file

c6e545f

Add changelog

e18860c

Address review comments [ci skip]

ebe4862

Require sphinx-astropy>=1.3

37da645

saimn force-pushed the generate-config branch from 4574b71 to 37da645 Compare April 28, 2020 17:59

bsipocz approved these changes Apr 28, 2020

View reviewed changes

bsipocz merged commit 10c300b into astropy:master Apr 28, 2020

saimn deleted the generate-config branch April 28, 2020 20:08

bsipocz mentioned this pull request Apr 30, 2020

Suppress config loading with ASTROPY_SUPPRESS_CONFIG #10090

Closed

2 tasks done

saimn mentioned this pull request Oct 14, 2020

Config woes meta-ticket - add ASTROPY_CONFIG_DIR and ASTROPY_CACHE_DIR #10738

Closed

	if mod.module_finder.path.endswith('tests'):
	if mod.module_finder.path.endswith(('tests', 'test')):

Conversation

saimn commented Apr 15, 2020 • edited by pllim Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

pllim commented Apr 15, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mhvk commented Apr 15, 2020

Uh oh!

This comment has been minimized.

This comment has been minimized.

bsipocz commented Apr 16, 2020

Uh oh!

saimn commented Apr 16, 2020

Uh oh!

saimn commented Apr 21, 2020

Uh oh!

pllim commented Apr 21, 2020

Uh oh!

saimn commented Apr 21, 2020

Uh oh!

bsipocz commented Apr 21, 2020

Uh oh!

Uh oh!

mhvk left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

pllim left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pllim left a comment

Choose a reason for hiding this comment

Uh oh!

bsipocz commented Apr 28, 2020

Uh oh!

astrofrog commented Apr 28, 2020

Uh oh!

saimn commented Apr 28, 2020

Uh oh!

bsipocz commented Apr 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

saimn commented Apr 15, 2020 •

edited by pllim

Loading

pllim commented Apr 15, 2020 •

edited

Loading