-
-
Notifications
You must be signed in to change notification settings - Fork 2k
Package layout #5
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since I see that you have the version stuff in a separate module, I wonder if it would make sense to have:
from astropy.version import version as __version__ here
since the unofficial seems to have packagename.__version__ available?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest not putting anything in astropy/init.py just yet., as I kind of always figured astropy would be a "namespace package". But that's not something that's really been discussed formally yet.
|
This pull request seems fine to me. I like the layout. I definitely agree with adding |
|
I agree that adding the |
|
I may want to have a couple in our branch look at this. Is it typical to have the C extensions that separated from the Python code? Usually they are tightly coupled with a particular module or package and I wonder if it makes sense to put things in two different places. What is the advantage of doing that? |
|
The advantage of a separate /extern directory for c code is that you can put "un-altered" C libraries in there so that they don't need to be downloaded separately, but can be easily updated in their own directories when the upstream library is updated. The /astropy/wrappers directory is for the wrappers for the /extern c libraries only - it's in /astropy because it's clearer to have the Cython/C code be where it actually ends up in terms of the python package layout, but remain separated out so that it's clear that that code is directly interfacing with the /extern C libraries. The intent is that any Cython or C code that is not directly related to an external library gets placed in the particular module it is used in, exactly as you say. These two separated directories are only meant for pre-existing C-libraries that need to be wrapped. |
|
This looks good to me as a starting template, other than my above comment about namespace packages. That's not really an actionable comment at this point though, as there has been no discussion about it. My point about namespace packages, however, is that if astropy is going to be a large platform, it may be desireable to break out at least parts of it into separate optional installables, in which case namespace packages are the way to go. I don't know if anyone else had this in mind though. [And as an aside on namespace packages in general, I'm aware that there's been discussion in scikits about not using them anymore, but I think that's misguided, especially in anticipation of PEP 382/402, either of which will make them easier to deal with once accepted.] |
|
Actually, the intent as specified in the vision and coding guidelines is specifically not to have astropy be a namespace package. The idea is that the astropy package contains all the core functionality, and everything else uses the "awastropy" or no top-level namespace (just the package name directly), until it's merged into the astropy core. So this pull request is about the layout of the core package, and the astropy/project-template package is the one that is the template for affiliated packages. (it will reflect this design fairly closely so as to make merging easier, but will have slightly different names and include examples and such) Once this layout is finalized, we will implement the affiliated package index, and it is those affiliated packages that will allow for the separate optional installable you have in mind. While I see the advantages of the namespace package idea, they're a pain to use given the problems that PEP 382/402 were designed to fix. 382/402 are great, but we can't use them while still maintaing 2.x compatibility (they only apply to 3.2 and 3.3). So at least for the near term, this scheme is more straightforward. |
|
I guess my problem is that it hasn't really been defined what make up the "core functionality" required for other Astronomy packages. By some definitions that could be a lot of stuff. But I'm fine with saying "no namespace packages" for now, and it can be worried about if "being too big" actually becomes a real problem. |
|
You have a good point to keep in mind - we should definitely have this option on the radar for the future as we merge other affiliated packages in and be sure to at least allow for a change to a namespace package if "too big" becomes a problem. If nothing else, hopefully by then everyone will have moved to 3.x and we can safely use the 382/402 enhancements. |
… suggestions by Mark Sienkiewicz
astropy/extern/__init__.py
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think /extern should be /cextern now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, good catch! Corrected.
|
This looks good to me. Obviously the layout might still change slightly in future, but I think we should merge this for now. |
force freezing version to use log and skip odict when builtin in use
force freezing version to use log and skip odict when builtin in use
…ng manual conflict resolutions
Use MSVC to compile instead of mingw
made fits-tables example
Co-authored-by: Adrian Price-Whelan <[email protected]>
Add PyVO as astropy affiliated package to registry.json
This pile of commits define the setup.py and initial layout for the package.
One item I wasn't sure about: versioning. As it stands right now, version numbers are set by the "version" module, which creates version numbers of the form "x.x.x" if the
releasevariable is True, and otherwise produces "x.x.xdev-r##" where ## is the total number of commits in the github repository. This requires a little trick to get the version numbers to properly freeze when installing a development version (see the astropy_build_ext in setup.py), but it works fine as-is.I think it's better to at least have "dev" at the end of the version name - pypi, setupytools, and distutils all behave properly when you do this - a version that ends in the string "dev" is always an "earlier" version than one with "dev", and the "-r##" means you always know if you have forgotten to re-run python setup.py install or the like. But I could change it to just "x.x.xdev" or "x.x.xalpha" or something like that without the version number, if that's deemed a bad idea.