Make the configuration file optional by saimn · Pull Request #10877 · astropy/astropy

saimn · 2020-10-16T02:28:07Z

Ref discussion in #10738 and #10090 : stop writing/updating the configuration file by default when importing the package, and do nothing when reading if the file is absent.
I also replaced update_default_config with a new function create_config_file which can be used to create or update the default file. And removed some old stuff for versions < 0.4. And removed the default config file which is present in the package since it can be generated, this will avoid having to update it in the release procedure.

cc @astrofrog @pllim @embray

EDIT:
Fix #5899
Close #7290
Close #6511
Close #10090

astrofrog

Just a quick comment below and also wanted to say that we should make sure we aren't breaking anything for downstream packages (the change in behaviour is fine, I mean no removal of functions etc that might break downstream)

astrofrog · 2020-10-16T10:01:54Z

astropy/config/configuration.py

-    ``pkg.version.cfg`` as a "template" for the user.
+    Create the default configuration file for the specified package.
+    If the file already exists, it is updated only if it has not been
+    modified.  Otherwise the ``force`` flag is needed to overwrite it.


We could make it even simpler and just error if the file already exists unless force is given, without checking content? Would simplify the code further...

In my experience though, if user really comes across this exception, we will have a feature request to ask for option to overwrite... 🤷

Yes, I'm happy to simplify more.

Thinking more about this, the problem is that the direct string comparison will fail if a configuration item has been added. So is_unedited_config_file is a nice a simple way to check if the config file has not been customized, whatever the version was used to generate it.

I restored update_default_config since other packages use it, and it relies on is_unedited_config_file.

pllim · 2020-10-16T15:05:03Z

@saimn , since title says WIP, I converted this PR to draft. FYI.

pllim

I do like the amount of simplification this introduces, but given the behavior change, this should be in 4.3 or later, at least. I have set the milestone to 5.0 for now, but it is open for discussion.

User facing doc also needs to be written to fully explain what to expect with this change. Perhaps even a section for downstream package maintainers.

This patch should be tested with coordinated packages and a few affiliated ones (I would want to test mine at least) before being merged. The consequences of setting config alias, adding new config, removing old config, or changing existing config data type should also be considered.

pllim · 2020-10-16T15:11:30Z

astropy/config/configuration.py

-    ``pkg.version.cfg`` as a "template" for the user.
+    Create the default configuration file for the specified package.
+    If the file already exists, it is updated only if it has not been
+    modified.  Otherwise the ``force`` flag is needed to overwrite it.


In my experience though, if user really comes across this exception, we will have a feature request to ask for option to overwrite... 🤷

astropy/config/configuration.py

saimn · 2020-10-16T15:19:07Z

Thanks @pllim, I could not find how to make it a draft PR...

saimn · 2020-10-16T15:23:03Z

@astrofrog - Just a quick comment below and also wanted to say that we should make sure we aren't breaking anything for downstream packages (the change in behaviour is fine, I mean no removal of functions etc that might break downstream)

Yep, I realized after that astroquery may use update_default_config (and it does: https://github.com/astropy/astroquery/blob/master/astroquery/_astropy_init.py#L43). astroquery is probably the only package using the configuration system ?
So we could keep update_default_config longer, or make it do nothing (keeping it as an empty function would stop updating the config file as for astropy core) ?

pllim · 2020-10-16T15:26:15Z

astroquery is probably the only package using the configuration system

Actually, a few of my own packages relies on it. For example, synphot and stsynphot.

draft PR

Click the small arrow on the giant green button that says "create PR" and there will be a second option to create draft PR. 😉

saimn · 2020-10-17T23:22:54Z

I do like the amount of simplification this introduces, but given the behavior change, this should be in 4.3 or later, at least. I have set the milestone to 5.0 for now, but it is open for discussion.

5.0 seems a bit far way, and I don't think this change would be a big disruption ;)

User facing doc also needs to be written to fully explain what to expect with this change. Perhaps even a section for downstream package maintainers.

Sure!

This patch should be tested with coordinated packages and a few affiliated ones (I would want to test mine at least) before being merged. The consequences of setting config alias, adding new config, removing old config, or changing existing config data type should also be considered.

Not sure what you mean with config alias and data type ?
I think the main thing to test is that the generate_config/create_config_file functions work well for other packages. I'm trying with astroquery, and there are some issues ;) (one of the subpackages is listed 3 times in the generated config file), so I will try to fix that.

astrofrog · 2020-10-18T06:15:30Z

What about milestoning for 4.3 and then merging as soon as possible after the 4.2 branching which will give ample time for testing with downstream packages?

saimn · 2020-10-18T18:00:37Z

Ref #10893 for a few fixes to get generate_config works correctly with astroquery.

embray

👍 aside from this one question.

embray · 2020-10-19T17:16:18Z

astropy/config/configuration.py

+        with open(cfgfn, 'rt', encoding='latin-1') as fd:
+            content = fd.read()
+
+        identical = (content == template_content)


Instead of comparing the config files byte-for-byte, why not just parse the existing config file and compare it to the default configuration?

This might require some refactoring of generate_config to have it generate the actual config data structure first, then a separate method for writing it to a file; probably how it should have been done in the first place.

Unless we really care about ensuring that it is byte-for-byte identical.

Yes, that's the way it was done. But just after is_unedited_config_file loads the config file and checks if any keys are defined. So, since now this will not be run at import, maybe we can just keep the second check?

Original review comments addressed

saimn · 2021-04-07T20:51:57Z

😲

saimn · 2021-04-07T21:02:34Z

Oh, that's the master→default rename ...

pllim

Let's do it! ✔️

saimn added the config label Oct 16, 2020

astrofrog reviewed Oct 16, 2020

View reviewed changes

pllim added needs-discussion Enhancement labels Oct 16, 2020

pllim added this to the v5.0 milestone Oct 16, 2020

pllim marked this pull request as draft October 16, 2020 15:04

pllim requested review from embray and pllim October 16, 2020 15:05

pllim added the whatsnew-needed label Oct 16, 2020

pllim reviewed Oct 16, 2020

View reviewed changes

saimn force-pushed the config-optional branch from df0bdc6 to 886c4ea Compare October 17, 2020 22:51

saimn mentioned this pull request Oct 18, 2020

Comment api_key in the default config file astropy/astroquery#1865

Merged

embray reviewed Oct 19, 2020

View reviewed changes

This was referenced Oct 22, 2020

Fix a few issues with generate_config #10893

Merged

Suppress config loading with ASTROPY_SUPPRESS_CONFIG #10090

Closed

saimn force-pushed the config-optional branch from 886c4ea to ecacdf7 Compare January 30, 2021 22:47

saimn changed the title ~~WIP: Make the configuration file optional~~ Make the configuration file optional Feb 6, 2021

saimn marked this pull request as ready for review February 6, 2021 19:37

saimn modified the milestones: v5.0, v4.3 Feb 6, 2021

saimn requested review from astrofrog, embray and pllim February 6, 2021 19:38

pllim mentioned this pull request Apr 7, 2021

Add QDP data file handling #11256

Merged

saimn added 17 commits April 7, 2021 23:02

Do not write / update the config when importing astropy

be23146

Remove unedited detection for old versions

c0c0998

Remove ConfigurationMissingWarning

629200a

Fix tests

145d00a

Fix exception case in get_config

8a55931

Replace update_default_config with a new function create_config_file

762dadb

Use get_config_filename and add comments

027c24c

Remove the default config file

7b3828e

Skip this test for now

5d39d31

Rename force to overwrite

39255e1

Restore update_default_config for packages that use it

a68ac93

Simplify check for unedited file

cf9803e

Add test

1373fad

Update documentation [ci skip]

7352f32

Add changelog entry [ci skip]

8f61d17

Add review comments and add back ConfigurationMissingWarning

165b245

Convert changelog to towncrier

c8db8fd

saimn force-pushed the config-optional branch from 35864fa to c8db8fd Compare April 7, 2021 21:03

pllim mentioned this pull request Apr 7, 2021

Refactor optional dependency checking #11490

Merged

pllim approved these changes Apr 7, 2021

View reviewed changes

pllim removed the needs-discussion label Apr 7, 2021

pllim merged commit fa5ca6d into astropy:main Apr 7, 2021

saimn deleted the config-optional branch April 7, 2021 21:45

This was referenced Apr 7, 2021

MNT: Deprecate update_default_config and ConfigurationMissingWarning #11497

Closed

MNT: Stop using update_default_config and ConfigurationMissingWarning astropy/package-template#508

Closed

pllim mentioned this pull request Aug 10, 2021

BUG: Fix cfgtype list serialization, fix table.jsviewer.conf.css_urls cfgtype #12037

Merged

9 tasks

Uh oh!

Conversation

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

Uh oh!

Uh oh!

astrofrog 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!

pllim commented Oct 16, 2020

Uh oh!

pllim 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!

Uh oh!

saimn commented Oct 16, 2020

Uh oh!

saimn commented Oct 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pllim commented Oct 16, 2020

Uh oh!

saimn commented Oct 17, 2020

Uh oh!

astrofrog commented Oct 18, 2020

Uh oh!

saimn commented Oct 18, 2020

Uh oh!

embray 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!

saimn commented Apr 7, 2021

Uh oh!

saimn commented Apr 7, 2021

Uh oh!

pllim left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

saimn commented Oct 16, 2020 •

edited by pllim

Loading

pllim left a comment •

edited

Loading

saimn commented Oct 16, 2020 •

edited

Loading