Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Create a separate astropy-helpers package to contain common infrastructure code for the core and affiliated packages #1563

Merged
merged 10 commits into from
May 19, 2014

Conversation

embray
Copy link
Member

@embray embray commented Dec 16, 2013

(This conversation started in astropy/package-template#32, but I will attempt to summarize here)

At the moment, affiliated packages need to import Astropy in order to have access to infrastructure code such as code to help populate the setup metadata. This is a bad idea, because packages shouldn't have to import a large package such as Astropy to be able to run the setup.py script.

@embray summarized it better than I could:

The astropy package contains a number of useful utilities and hacks on distutils for building and installing, and distributing a package that depends on Numpy and Cython and that developed on git (as it includes utils for including the git changeset in the version info). The problem is that, as all those utilities reside in the astropy package, it has to be installed before they can be used in another package's setup.py. The setup_requires feature allows a package to be made available when running a project's setup.py script. Unfortunately, the astropy package itself being fairly sizeable and slow to build makes running any setup.py requiring astropy very slow. Furthermore, the way the feature is implemented requires running the setup() function, which supply's all the project's metadata, even though some of that metadata might require some of astropy's setup utilities to be generated (such as the full version string).``

The solution we have converged to in astropy/package-template#32 is to create a separate repository containing the common infrastructure code. Again, to quote @embray:

The first, and my personal preference especially from the view of affiliated packages (though also potentially beneficial for the main Astropy package) is to create a separate subproject, say, "astropy-distutils" that contains all the utilities used in setting up Astropy or affiliated packages using our framework. This could potentially include all the test stuff too. This would be a much smaller and simpler package than Astropy itself, and so can easily be used with the setup_requires feature of setuptools (where a package listed in setup_requires is downloaded and added to sys.path if it's not already installed).

I have some experience with this: STScI has a package I wrote called "stsci-distutils" that serves the same purpose: a common collection of build and installation tools shared by all our packages. This works rather well. A particular advantage of this is that in many cases, if a tweak is needed to the build infrastructure (say, for example, Apple changes their default C compiler again) support for that can be added simply by releasing a new "astropy-distutils", rather than having to release a whole new astropy to fix builds on the platform in question. Of course, this won't always be possible, but it would work in many cases.

There are some minor questions, such as:

  • whether this package should be a namespace package for astropy (i.e. it would be separate, but share the namespace with astropy), or whether it should be its own independent package/namespace.
  • whether it could be included via sub-modules (which doesn't seem necessary)

Also, we need to think which of the following it makes sense to move to this separate package - i.e. where do we stop? Do we include only what is strictly necessary for setup.py? For example:

  • setup_helpers.py
  • version_helpers.py
  • utils/release.py

What about the testing framework? Or the configuration framework? (once it's refactored)

Of course, we don't want to do something so drastic before 0.3, but we should do it as soon as possible after 0.3.0 is out so that we have many months to iron out any bugs.

@astrofrog
Copy link
Member Author

Also, on a more minor level, there is the choice of name for this package - astropy_distutils, astropy_setuputils, etc.

@eteq
Copy link
Member

eteq commented Oct 12, 2013

I few items to add from the previous issue:

  • @wkerzendorf wanted to avoid a name that explicitly says "astropy" on the grounds that it might be useful in non-astropy contexts. That may make sense, but (as @embray also mentioned), it will take some work to make in non-astropy-specific, and I don't think we want to do that later (if there's ever call for it). So I like the idea of trying to use a name that can be generified (sciencepkg_utils or similar), but not actually putting any further effort into generifying it right now.

  • I had earlier expressed concern about keeping the astropy and this other package's release cycle together. @embray has convinced me that this isn't that big of a deal because it will be easy to release the new package because it will be simple. I do still think there's a concern about how we manage the github repositories, though. Is it a separate repository, or does it live inside the astropy repository? The former seems better for a separate package (and absolutely necessary if we go the submodule route), but then how do we manage the issues? Does it have a separate issue list, or is it shared with astropy? I can pretty much guarantee you in the separate issues case that a lot of users will report the issues in the wrong place.

  • @embray mentioned the following:

    I'm a little concerned about the submodule idea, but my concerns may be due to lack of experience with submodules. Mainly, can we set up submodules so that when one does a git clone path/to/astropy.git it also automatically includes the submodule in the tree? Otherwise things like pip install git+http://path/to/astropy.git will fail spectacularly since the tools needed to run the setup.py will be absent from the default repository clone.

    There's two answers to this. You can do git clone --recursive path/to/astropy.git and it will automatically synchronize the submodules. But we can't count on users to know this, so the solution IPython has is to have their setup.py check for synchronizing. That's a bit annoying for us because that's exactly the sort of tool we would want in the setup package that we're synchronizing. But I don't think there's any solution to that, so we'd just have to include it as part of the template setup.py. This does make things a bit more complicated for the initial setup of an affiliated package, though, because they also have to set the appropriate submodules.
    TL;DR: we can make this work without much (if any) extra effort for users, but affiliated package developers will probably have more work.

@eteq
Copy link
Member

eteq commented Oct 12, 2013

Oh, and to address @astrofrog's comment:

Also, we need to think which of the following it makes sense to move to this separate package - i.e. where do we stop? Do we include only what is strictly necessary for setup.py?
What about the testing framework? Or the configuration framework? (once it's refactored)

I think probably the safest route forward is to do the minimal required initially (basically the three files @astrofrog mentioned). If we go any further than that we should go all the way, though. That is, move the testing, configuration, documentation tools, and astropy.utils sorts of items to a separate pacakge. Then only the actual astronomy stuff is in astropy. The downside there is that then we really do have to maintain it separately, with separate docs and such. If we don't want to do that (or until we're willing to do that), we should keep everything not actually needed for setup in astropy.

@astrofrog
Copy link
Member Author

So I like the idea of trying to use a name that can be generified (sciencepkg_utils or similar), but not actually putting any further effort into generifying it right now.

Why not start with the name astropy_setuputils then simply rename it if we decide to expand the scope?

Is it a separate repository, or does it live inside the astropy repository? The former seems better for a separate package (and absolutely necessary if we go the submodule route), but then how do we manage the issues? Does it have a separate issue list, or is it shared with astropy? I can pretty much guarantee you in the separate issues case that a lot of users will report the issues in the wrong place.

Good point about the issues. We definitely want a separate repository IMHO (since it will have its own releases, tags, etc.) but we could consider disabling issues (but not pull requests) on that repository if we don't want things to be spread out too much.

TL;DR: we can make this work without much (if any) extra effort for users, but affiliated package developers will probably have more work.

Just to be clear, this comment relates just to the submodule idea, right? It seems to me it might be simpler for everyone if we can make it work as a normal package instead. Note that this also enables affiliated package maintainers to easily use this package without having to use the full template too.

I think probably the safest route forward is to do the minimal required initially (basically the three files @astrofrog mentioned)

Ok, let's go with the minimum required for now, and we can see how that works before moving ahead.

Shall I give it a go and try and create a simple repo?

@astrofrog
Copy link
Member Author

Also, it might be worth waiting until #1493 and #1555 are resolved (one way or another) since that will simplify some of the setup code.

@astrofrog
Copy link
Member Author

Another option for a name - astropy_helpers or astropy_setuphelpers

@embray
Copy link
Member

embray commented Oct 14, 2013

Still catching up on this, but I would like to make one comment that I also left in the package-templates issue:

I also wouldn't be against, if not slightly in favor of a hybrid approach, whereby we have a separate package for install utils, but we can also include the the master branch of that as a subpackage in Astropy, since I would always want to test against the latest dev version anyways.

@embray
Copy link
Member

embray commented Oct 14, 2013

Another bit on submodules:

There's two answers to this. You can do git clone --recursive path/to/astropy.git and it will automatically synchronize the submodules. But we can't count on users to know this, so the solution IPython has is to have their setup.py check for synchronizing. That's a bit annoying for us because that's exactly the sort of tool we would want in the setup package that we're synchronizing. But I don't think there's any solution to that, so we'd just have to include it as part of the template setup.py. This does make things a bit more complicated for the initial setup of an affiliated package, though, because they also have to set the appropriate submodules.

I wouldn't mind writing a little "bootstrap" script for the "astropy_science_package_setuptools_utils" package or whatever we're calling it. This can be very simple actually: First it would attempt the git submodule synchronization you described, a la IPython. This would enable the "hybrid" approach I mentioned, where we still include a copy of the setup utils in the main Astropy repo as a submodule. I think this is preferable for anyone doing Astropy development.

If it's not a git clone (or the repo in question simply doesn't use submodules), however, it would just use the setuptools setup_requires approach to grab a copy of the setup package--this is the approach that would be used in most installation contexts. Just include the boostrap script alongside setup.py, and all the setup.py has to do is import it. I could probably roll it up with the stuff I already included in setuptools_bootstrap.py.

@eteq
Copy link
Member

eteq commented Oct 14, 2013

Ahh, now I think I get @embray's hybrid approach idea. I like it, as it seems to combine the ease of a separate package (as suggested by @astrofrog) with a straightforward way to include it in the astropy repo. Then affiliated package devs have it easier because they don't have to synchronize, but they can if they want. So that addresses most of my concerns (aside from the issue-tracker question, and I agree with @astrofrog that it might make sense to just turn off issues there as long as it has astropy in the name).

Another reason why having submodule stuff might be nice is that then we might be able to start using those tools in extern where relevant - that is, we wouldn't have to keep updating the extern packages within our repo, we could instead use them as submodules. This was discussed in some other issue that I can't find, and this would give us a good reason to implement the machinery for it (a la IPython).

And on the name: @astrofrog says

Why not start with the name astropy_setuputils then simply rename it if we decide to expand the scope?

because if we re-name it, then we have to have two separate projects (with different names) on pypi that are continuations of each other. In my experience, that seems to be really difficult to do without user confusion. But maybe I'm worrying too much.

@embray
Copy link
Member

embray commented Oct 15, 2013

I think in this case they're still different enough. Right now we're talking only something specifically for Astropy and affiliated packages.

I've had though the thought for a while now that it would be nice to make some of the stuff we've hacked together easier to use for other projects too, but if someone actually has the time and inclination to do that I think it would evolve into something different enough to warrant a different name.

I don't think "we" (as in the Astropy project) need to directly support such a project. I'd be interested in helping out with something like that sometime, but I see it as a tangential endeavor.

@eteq
Copy link
Member

eteq commented Oct 16, 2013

Good point, @embray - so in that case I'm fine with @astrofrog's suggestion of astropy_helpers - that doesn't narrow the scope specifically to setup, in case we want to expand it to include testing and such later.

@iguananaut
Copy link

I'm pretty happy with astropy_helpers, I think. Perhaps this should be a candidate for an APE? I'd be happy to draft it, but we don't even have APE-0 to describe the process....

@mdboom
Copy link
Contributor

mdboom commented Oct 16, 2013

We don't? https://github.com/astropy/astropy/wiki/APE1

On Wed, Oct 16, 2013 at 12:59 PM, Erik notifications@github.com wrote:

I'm pretty happy with astropy_helpers, I think. Perhaps this should be a
candidate for an APE? I'd be happy to draft it, but we don't even have
APE-0 to describe the process....


Reply to this email directly or view it on GitHubhttps://github.com//issues/1563#issuecomment-26437148
.

|/|o | . _ | | ._ ||| _ _ ._ _
| ||(| |(|(/| |/|()(/|_ ||)()()| | |

http://www.droettboom.com/

@astrofrog
Copy link
Member Author

@embray - feel free to start drafting APE2! :)

@iguananaut
Copy link

Well for one it's incomplete and hasn't even been discussed or remotely finalized as far as I'm aware. But at least it's a start.

@astrofrog
Copy link
Member Author

APE1 seems reasonably complete, but I agree it hasn't been approved explicitly yet.

@embray
Copy link
Member

embray commented Nov 14, 2013

By the way, another thought regarding this that #1785 brings to mind, is that if we have a separate package for this stuff we can also have a separate test suite for it. Now, technically we could also include tests for it in the normal Astropy test suite, but build/installer tests can be annoying to run--time consuming, and error-prone.

That said, it can be done. I have test suites for d2to1 and stsci.distutils that get pretty good coverage, and I would add the same to astropy-distutils or whatever we wind up calling it.

@astrofrog astrofrog mentioned this pull request Nov 16, 2013
24 tasks
@astrofrog
Copy link
Member Author

@embray - APE1 is accepted (https://github.com/astropy/astropy-APEs), so we can start working on the APE for the astropy_helpers package. Note that the repo now exists (https://github.com/astropy/astropy_helpers).

@astrofrog
Copy link
Member Author

@embray +1 on having a separate test suite for it

@astrofrog
Copy link
Member Author

@embray - do you want to have a go at drafting APE2 or APE3 (now that APE1 is accepted) or shall I?

@embray
Copy link
Member

embray commented Dec 2, 2013

I will try to take this on. This is the sort of thing that practically demands a sample implementation to go along with the APE, since otherwise it would be difficult ahead of time to figure out exactly all the issues involved. Though I will start an outline of the APE first to give a starting point.

@embray
Copy link
Member

embray commented Dec 16, 2013

Coverted this into a WIP PR to support using astropy_helpers with Astropy. Right now this pulls in my fork of astropy_helpers as a submodule (see astropy/astropy-helpers#1). Before merging into master, this will be switched over to the main fork of astropy_helpers once it is ready.

This PR will serve as part of the initial sample implementation to go along with the first draft of the APE.

@astrofrog
Copy link
Member Author

@embray - just a quick question (though of course I should really wait for the APE). Will packages that want to use astropy_helpers need to include ez_setup.py and setuptools_bootstrap.py in addition to ah_bootstrap.py? If a package includes a sub-module version of astropy_helpers, is there a reason those files can't exist inside the astropy_helpers sub-module to keep things tidy?

@embray
Copy link
Member

embray commented Dec 17, 2013

@astrofrog As a matter of fact, I was just thinking about that.

I could put in some try/excepts that would allow packages to exclude setuptools_bootstrap.py and ez_setup.py, but I wouldn't recommend it. I might consider moving the functionality of setuptools_bootstrap.py into ah_bootstrap.py. But ez_setup.py must remain separate for many reasons. I would also leave it at the root of the repository. It needs to be very easy to import.

I do intend to address this matter in the APE.

@embray
Copy link
Member

embray commented Dec 17, 2013

Also, in the interest of somewhat reducing the number of files at the root of the repo, we could move the .dmg_background.png and .wininst_background.png files elsewhere, perhaps.

@astrofrog
Copy link
Member Author

@embray - ok, great that you were thinking about it too :) At the end of the day I was basically wondering what a set of instructions for affiliated packages would look like, e.g. add this sub-module and these two files, and you're all set.

@embray
Copy link
Member

embray commented Apr 11, 2014

Note before merging this PR should be squashed.

try:
module = self.fspath.pyimport()
# Just ignore searching modules that can't be imported when
# collecting doctests
Copy link
Contributor

Choose a reason for hiding this comment

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

I think #2310 is probably fixed by this.

@astrofrog
Copy link
Member Author

@embray - you mentioned that the PR should be squashed before merging. As indicated in my APE4 comments, I think we should go ahead and merge this over the next day or so if there are no last-minute objections, so maybe now would be a good time to rebase and let travis have one last go at this.

@astrofrog
Copy link
Member Author

@embray - do you think it will be possible to finalize this early this coming week? Let me know if there is anything I can do to help re: affiliated packages.

@astrofrog
Copy link
Member Author

@embray - now we've merged in astropy-helpers, I think the URL can be updated here too? Did you want to squash before merging?

@embray
Copy link
Member

embray commented May 19, 2014

Yeah, I'm going to update that URL and do some rebasing.

embray added 10 commits May 19, 2014 10:43
…I'm adding my fork of astropy_helpers but will update the submodule to point to https://github.com/astropy/astropy_helpers.git once this is ready to merge into astropy's master.

Updating submodule ref
…. Had to change all the various 'setup_package' modules to also astropy_helpers. Seems to work in basic cases.

Updating pointer to astropy_helpers
… script in setup.py

Update ah_bootstrap copy

Update astropy_helpers submodule version

Updating the version of ah_bootstrap.py

Update the version of astropy_helpers to the latest, including a number of bug fixes
…y_helpers.sphinx for the doc builds. The astropy.sphinx module will still hang around for the time being for backward-compat, but can later be removed.
…' capability

Update the call to get_package_info since its interface changed
…portError when trying to import it to look for tests. In particular this is needed to skip modules that require astropy_helpers, like some of the setup_package.py modules. This seems like reasonable-enough behavior anyway--if some actual problem is causing ImportErrors in some modules that will hopefully be picked up by the normal unit tests as well.
@embray
Copy link
Member

embray commented May 19, 2014

Sorry, made a bit of a mess of things in rebasing this. But I think it's good now.

@astrofrog
Copy link
Member Author

Looks good to me! @embray - ready to merge?

@embray
Copy link
Member

embray commented May 19, 2014

Excellent--merging then.

embray added a commit that referenced this pull request May 19, 2014
Create a separate ``astropy-distutils`` package to contain common infrastructure code for the core and affiliated packages
@embray embray merged commit f832a98 into astropy:master May 19, 2014
@embray embray deleted the astropy_helpers branch May 19, 2014 19:23
@astrofrog
Copy link
Member Author

Great! 🎆

Thanks @embray!

@embray embray changed the title Create a separate astropy-distutils package to contain common infrastructure code for the core and affiliated packages Create a separate astropy-helpers package to contain common infrastructure code for the core and affiliated packages Jun 26, 2014
@embray embray changed the title Create a separate astropy-helpers package to contain common infrastructure code for the core and affiliated packages Create a separate astropy-helpers package to contain common infrastructure code for the core and affiliated packages Jun 26, 2014
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.

5 participants