Skip to content

APE3: New configuration system#3

Merged
astrofrog merged 5 commits into
astropy:masterfrom
mdboom:config
Apr 30, 2014
Merged

APE3: New configuration system#3
astrofrog merged 5 commits into
astropy:masterfrom
mdboom:config

Conversation

@mdboom

@mdboom mdboom commented Dec 10, 2013

Copy link
Copy Markdown
Contributor

No description provided.

Comment thread APE4.rst Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

commented out to me means they are commented - should we clarify by saying uncommented?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doh. Good point.

@astrofrog

Copy link
Copy Markdown
Member

If the 'default' configuration from 0.3.0 or earlier is found with no changes, would it make sense to throw it out and replace it with the new one, automatically? Or at least raise a warning that we recommend removing this file?

@mdboom

mdboom commented Dec 10, 2013

Copy link
Copy Markdown
Contributor Author

We should definitely raise a warning recommending removing the file. However, the situation I was hoping to avoid was one where a user moves back and forth between different versions of astropy, but on further reflection, this doesn't really address that either. We may need to include the version in the config file name, so that the two could exist side-by-side. Then, when you don't find one for the current version, you find the next oldest, upgrade that, and write it out as the current version. Then, of course, you may have to update config values in multiple files if you really need to be using different versions of astropy all the time... Anyway, I guess that means the versioning part of this APE requires further thought.

@eteq

eteq commented Dec 10, 2013

Copy link
Copy Markdown
Member

I'll read over this in more detail later... but one quick thing: why APE "4"? Currently there's only 1 and 2, so the next should be "3"...?

@mdboom

mdboom commented Dec 10, 2013

Copy link
Copy Markdown
Contributor Author

I thought the wiki had "allocated" the numbers, but maybe not...

https://github.com/astropy/astropy-APEs/wiki

@astrofrog

Copy link
Copy Markdown
Member

@mdboom - I didn't mean the order to be important for the APE? entries. The numbers should be allocated as pull requests are open, so +1 to changing this to APE3 if it's easy to do.

@eteq

eteq commented Dec 10, 2013

Copy link
Copy Markdown
Member

Yeah, I think its better if we assign numbers just at the time the PR is issued. So I'd agree that changing this to 3 makes more sense. (I think it's just renaming the file and changing the title of the PR, right?

@mdboom

mdboom commented Dec 10, 2013

Copy link
Copy Markdown
Contributor Author

Renamed to APE3.

Comment thread APE3.rst Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"overtime" ?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

haha. Will fix.

Comment thread APE3.rst Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think maybe "hinders reproducibility" might be better wording.

@astrofrog

Copy link
Copy Markdown
Member

Regarding the generation of the config file - there is one solution that hasn't really been mentioned which is simply to require users to create and add information to the config file. That is, we never copy any default file to the home directory, but the user can create it there. It means that we can then not worry about having to deal with all the magic to figure out if files have changed, etc.

@mdboom

mdboom commented Dec 11, 2013

Copy link
Copy Markdown
Contributor Author

@astrofrog: I'm not sure never copying the file over actually would resolve much. You still have the problem of understanding/upgrading a user's file from a previous version of astropy if they've created it. As it stands, I'm still proposing that we never replace the user's config file without her explicitly requesting astropy to do so.

@embray

embray commented Dec 13, 2013

Copy link
Copy Markdown
Member

Providing a sequence of migration scripts is no biggie, though they do have to be somewhat flexible and fail gracefully on goofy input. But the vast majority of users aren't even likely to know their config file is there. I think an astropy.cfg.sample is a good middle ground because for most upgrades that file can just be overwritten with no loss, and for the few who do have a real config file they have an easy way to update it and clean up any artifacts in it that are no longer relevant. I consider that a nice-to-have more than a must-have though.

@mdboom

mdboom commented Dec 17, 2013

Copy link
Copy Markdown
Contributor Author

I've updated a new draft here. It addresses all of the minor points in this PR, and then:

  • In light of the comments here about upgrading the config files, I'm now proposing something much simpler based on what Debian package upgrades do. This is explained in the APE, if you're not familiar, and described pretty well in this blog post http://raphaelhertzog.com/2010/09/21/debian-conffile-configuration-file-managed-by-dpkg/, though I'm not advocating having the interactive prompts that Debian has.)
  • Simplifying the creation of the config file template down to the simplest possible thing -- a manually maintained file in the source code repository. This eliminates all problems with keeping code importable at build time. I think it will also make @embray happier that setup is doing fewer magical things. While that will have issues keeping it in sync with the source code, we also plan to have many fewer config file options at the end of this APE, and they shouldn't change very often. I think the benefits outweigh the downsides here.

@astrofrog

Copy link
Copy Markdown
Member

@mdboom - I agree with all the changes. 👍 from me for this APE.

@eteq

eteq commented Dec 17, 2013

Copy link
Copy Markdown
Member

Note that, if we want to go with #4, you'll want to slightly update this to have dating fields consistent with that, @mdboom.

@eteq

eteq commented Dec 17, 2013

Copy link
Copy Markdown
Member

I ended up merging #4, so either @mdboom can update this to match that convention, or whoever merges this can (after all, the revision date changes then anyway).

Comment thread APE3.rst

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to be sure I understand: this version isactually the Astropy release version, right? Not some special version that advances separately from Astropy?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes -- my thinking is that it's the astropy version (excluding any "dev" part, because that will just kill it for us developers).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, that sounds good.

@eteq

eteq commented Dec 18, 2013

Copy link
Copy Markdown
Member

Thanks for putting this all together, @mdboom!

  • I completely agree with everything you have to say about distinguishing "science state" from "platform configuration."
  • This may have been what you were suggesting, but I like the idea of a template context manager that sets a specific value, which can be given a reasonable default, and automatically shows up with a reasonable docstring in automodapi. E.g. default_cosmology = ScienceState(WMAP9, 'The default cosmology ...'), which gets used in the module as default_comsology() (just like the current configuration does), but that users must set as with default_cosmology.set(WMAP5): or just default_cosmology.set(WMAP5) and that maybe check the type but otherwise that's the whole API. If that sounds good to you, maybe add that to the implementation explicitly?

The rest only applies to "platform configuration"

  • Am I understanding correctly that you want to keep the current syntax for configuration items, but move them to subpackage.config instead of the actual subpackage itself? Or that that's a temporary measure for the next release, but after that, just the name and value will do the job? Or is that the TBD part?
  • Along similar lines, is the plan to keep the ConfigObj backend? That ended up rather hacky and harder to write than I expected... But then again it is currently working. But something else might work better with the new organization proposed here where the config is all in subpackages.
  • You might guess this, but I have strong misgivings about the duplication of information called for here: the template has to be kept in source, as does the actual object, and the docstring has to be duplicated in the documentation. I just don't see it all getting kept in sync. I think we might manage it (most of the time) for the core package, but I'm sure affiliated packages will get out of sync (or they just won't use the system because it requires added cognitive load of keeping track of the same thing twice). That was one of the main things that drove the original API design - a need to keep all "declaration" in one place, and I think that's still very important. So let my try to offer a few alternatives for consideration (these are suggestions to change only the relevant parts of this APE, while leaving everything else alone):
  1. Keep the template configuration file in-source, but have it be the only definition for the configuration. Then keep the API similar to how it is now for using the objects, but have them go through a "gateway" in astropy.config, which magically creates all the items . E.g., the package code would do something like astropy.config.subpkg.CONFIG_ITEM(), and users can set values with astropy.config.subpkg.CONFIG_ITEM.set(val) and save with astropy.config.subpkg.CONFIG_ITEM.save() or astropy.config.subpkg.save(). This removes all the trouble with importing the whole module tree and building the configuration file, but keeps the definition all in one place. It also would be pretty easy to implement without too much change from the current system.
  2. Move all declarations to subpkg.config modules, but still generate the default configuration file at build. This could be sped up a lot by importing all of the subpkg.config package on their own, rather than as part of importing all of astropy. That solves the problem of the default build taking a long time and requiring a full import of everything. But it also places some limitations on what can be in the subpkg.configs (basically only standard python or numpy types), and it would still require some of the weirdness in the build scripts that generate the config defaults... So I think I like option 1 better over this one.

Also, I do agree with @mdboom's point that the configuration should be in the documentation. But I think we should avoid writing directly in the docs for the reasons I gave above - it should be possible to fiddle with automodapi to make this happen automatically in either of these two plans, though. (Maybe even in the scheme the current APE suggests, although we'd have to decide which of the two are the more "authoritative" - as-declared in code or as the comment in the template.)

@astrofrog

Copy link
Copy Markdown
Member

Regarding the point of duplicating information, I would vote that the template is the only definition (if possible). Just to check, this means the template gets installed in the package, since we can't rely on the user-modified configuration file, but we have to use the original template to define the possible configuration items, right? Another question is, how do we set the defaults since the template would have all values commented out?

@mdboom

mdboom commented Dec 18, 2013

Copy link
Copy Markdown
Contributor Author

@eteq: With regard to context managers: Yes, I envision a base class for all of these context managers. Note that set will need to be overridable -- not all of them will simply be changing a value. Some (as units.set_enabled_units already does) may need to do a whole bunch of data structure wrangling under the hood. I mention creating a base class for the context managers in the last paragraph of implementation.

@eteq wrote:

Am I understanding correctly that you want to keep the current syntax for configuration items, but move them to subpackage.config instead of the actual subpackage itself? Or that that's a temporary measure for the next release, but after that, just the name and value will do the job? Or is that the TBD part?

At present, the config items are in seemingly random locations -- generally in the source file where the config option is used. That is not only hard for the user to guess, but ties its location to implementation and the organization of the source files, which may change. I'm proposing that the standard location for all config options is subpackage.config. It could just as well be subpackage, but I think it's clearer what these things are if they are in subpackage.config.

Along similar lines, is the plan to keep the ConfigObj backend? That ended up rather hacky and harder to write than I expected... But then again it is currently working. But something else might work better with the new organization proposed here where the config is all in subpackages.

Yes, the plan is to keep it, though obviously the "load everything and write a config file part" (IMHO the hackiest part of it) would go away.

You might guess this, but I have strong misgivings about the duplication of information called for here: the template has to be kept in source, as does the actual object, and the docstring has to be duplicated in the documentation.

I agree about the first two, but can't the docstring be automatically pulled into the documentation? That may mean subpackage.config has to be a class rather than a module, but that's not a big deal.

Keep the template configuration file in-source, but have it be the only definition for the configuration. Then keep the API similar to how it is now for using the objects, but have them go through a "gateway" in astropy.config, which magically creates all the items .

I assume, specifically, you mean that a configspec file exists in-source, from which is generated a template config file. The template config file will not have enough information (e.g. the type of the values) to properly generate a ConfigurationItem.

E.g., the package code would do something like astropy.config.subpkg.CONFIG_ITEM(), and users can set values with astropy.config.subpkg.CONFIG_ITEM.set(val) and save with astropy.config.subpkg.CONFIG_ITEM.save() or astropy.config.subpkg.save(). This removes all the trouble with importing the whole module tree and building the configuration file, but keeps the definition all in one place. It also would be pretty easy to implement without too much change from the current system.
Move all declarations to subpkg.config modules, but still generate the default configuration file at build. This could be sped up a lot by importing all of the subpkg.config package on their own, rather than as part of importing all of astropy. That solves the problem of the default build taking a long time and requiring a full import of everything. But it also places some limitations on what can be in the subpkg.configs (basically only standard python or numpy types),

Don't we have that restriction now, given the use of ConfigObj?

and it would still require some of the weirdness in the build scripts that generate the config defaults... So I think I like option 1 better over this one.

Also, I do agree with @mdboom's point that the configuration should be in the documentation. But I think we should avoid writing directly in the docs for the reasons I gave above - it should be possible to fiddle with automodapi to make this happen automatically in either of these two plans, though. (Maybe even in the scheme the current APE suggests, although we'd have to decide which of the two are the more "authoritative" - as-declared in code or as the comment in the template.)

I should have been clearer on that -- I never meant that the documentation would be written directly in the .rst files, and had assumed automodapi would do the work here. My point there was that the config options should always be found (from the point of view of the built docs) in a standard place, just as we have our standard headings for other things already.

@mdboom

mdboom commented Dec 18, 2013

Copy link
Copy Markdown
Contributor Author

@astrofrog: I think the way to do what Erik's suggesting (where the config file is the canonical and only definition of the config options) is to include configspec file in the source tree, and generate a template config file from that on-the-fly. configspec files handle data types and defaults for each of the options, but a config file would not.

@eteq

eteq commented Jan 16, 2014

Copy link
Copy Markdown
Member

True enough - so yeah, at least some implementation might make some of the above discussion easier to foolow.

Comment thread APE3.rst Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"each the" -> "each of the"

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mdboom - it looks like this typo still needs fixing?

@astrofrog

Copy link
Copy Markdown
Member

Since there have been no objections to APE3 and no reports of issues with affiliated packages, I suggest that we go ahead and merge this and the associated pull request. Over the next month, before the 0.4.0 release, we might run into bugs, etc. but the sooner we merge this the sooner we will have this tested more widely.

@eteq @mdboom @embray - any objections to this?

@mdboom

mdboom commented Apr 22, 2014

Copy link
Copy Markdown
Contributor Author

No objections. I don't have perms to merge this. I'll merge the implementation once Travis passes.

Comment thread APE3.rst Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"there are number" -> "there are a number"

@embray

embray commented Apr 22, 2014

Copy link
Copy Markdown
Member

I gave it one more once-over and added a few more comments. Otherwise I'm 👍

@astrofrog

Copy link
Copy Markdown
Member

Ok - in that case once @eteq agrees, we can merge this!

@eteq

eteq commented Apr 24, 2014

Copy link
Copy Markdown
Member

The APE looks fine to me now! Some detailed items on astropy/astropy#2094 , but I suppose this can be accepted even if that's not finalized yet.

@astrofrog

Copy link
Copy Markdown
Member

@eteq - just to check, do we merge this then mark it as accepted in a separate commit?

@eteq

eteq commented Apr 28, 2014

Copy link
Copy Markdown
Member

@astrofrog - what I've been doing is locally adding an accept commit and then manually merging into astropy-APEs/master, as that way I know for sure I haven't forgotten to commit the accept information. (And that auto-closes the PR, of course.) But what you're suggesting works fine too, as long as the accept commit actually appears, so whatever you find easier is fine with me.

@astrofrog

Copy link
Copy Markdown
Member

Shall we merge this now or wait until astropy/astropy#2094 is also ready?

@eteq

eteq commented Apr 28, 2014

Copy link
Copy Markdown
Member

I think it's fine to merge this now as long as @mdboom doesn't foresee changes to this based on feedback in astropy/astropy#2094

@astrofrog

Copy link
Copy Markdown
Member

Ok - @mdboom - can you confirm that this APE will not change based on the feedback in astropy/astropy#2094?

@mdboom

mdboom commented Apr 30, 2014

Copy link
Copy Markdown
Contributor Author

No, it won't change.

@astrofrog

Copy link
Copy Markdown
Member

Ok, in that case, let's do this! Thanks @mdboom!

astrofrog added a commit that referenced this pull request Apr 30, 2014
APE3: New configuration system
@astrofrog astrofrog merged commit 0bb124c into astropy:master Apr 30, 2014
@eteq

eteq commented Apr 30, 2014

Copy link
Copy Markdown
Member

👏

mwcraig pushed a commit to mwcraig/astropy-APEs that referenced this pull request Nov 24, 2014
@jehturner jehturner mentioned this pull request Dec 11, 2014
kbwestfall pushed a commit that referenced this pull request Jan 27, 2026
docs: add verbiage about Coordinate
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants