Modify the directory structure

[montefra]

I propose to modify the directory structure of the project to put it more in line with packaging standard. The current structure is (comments are mine):

├── changelog
├── conf.py
├── COPYING
├── ds9.py            # obsolete: there is I warning since more than one year, I would just remove
├── index.rst
├── Makefile         # sphinx makefile
├── MANIFEST      # I don't think that we need this: it's not used anyway
├── MANIFEST.in  # IIRC this is the correct file name
├── pyds9.py
├── README
├── setup.py
├── test.fits
├── xpa
│    └── [...]
├── xpa.py

I propose the following structure

├── changelog
├── COPYING
├── docs
│     ├── source
│     │     ├── conf.py
│     │     ├── index.rst
│     │     └── [...]
│     └── Makefile
├── MANIFEST.in  # Define the non python files that need to be installed (like example files
├── pyds9
│     ├── __init__.py  # from pyds9 import * or only what we want a public interface. This way we maintain backward compatibility 
│     ├── data
│     │     ├── test.fits
│     ├── pyds9.py
│     ├── xpa
│     │    └── [...]
│     └── xpa.py
├── README
├── setup.py
└── tests
      └──  [...]  # test modules and data

Advantages:

• It's a standard directory structure
• It's tidier
• when installing, it will create in site-packages only one pyds9 directory with all the relevant files in it, instead of having multiple files in site-packages (libxpa.so, xpa.py, pyds9.py, ds9.py)

Disadvantages:

• need some work to reorganize and make setup.py work properly.

[cdeil]

:+1: , although I'd suggest two changes to what's outlined above, from what I've seen / learned from Astropy and other packages:

• Put the tests in the package, i.e. in pyds9/tests. This makes it easier to install them and run the tests the save from the git folder or when the package is installed in site-packages or distributed via tarball.
• What you now call pyds9/pyds9.py, call it pyds9/core.py. This is to avoid users getting confused if they usually do import pyds9 as they should, but then someone does from pyds9 import pyds9 and gets a different namespace content.

IMO it's important that a Python package only installs stuff in one namespace, so :+1: to remove the ds9.py that's still there for backwards-compatibility. Although probably @ericmandel should decide if this can be done now or at which point it's OK to remove that.

[montefra]

Put the tests in the package, i.e. in pyds9/tests. This makes it easier to install them and run the tests the save from the git folder or when the package is installed in site-packages or distributed via tarball.

I don't have a strong opinion. Anyway what is the advantage of installing them? The standard way to run tests is from the source directory with

python setup.py test

or

py.test

The only advantage that I see would be that we can also run tests from within a python script/session:

python -c 'import pyds9; pyds9.test()'

Git tarbal includes the whole repository. I have no experience with tarbal for PyPI, but if it's not already the default I guess that you can decide which file include.

What you now call pyds9/pyds9.py, call it pyds9/core.py. This is to avoid users getting confused if they usually do import pyds9 as they should, but then someone does from pyds9 import pyds9 and gets a different namespace content.

fine with me.

[cdeil]

The advantage of putting the tests in the package and installing them is that users can run them.

Users will just do pip install pyds9 or conda install pyds9 and not get the source folder and do python setup.py install manually.

When we get questions / report "why isn't pyds9 working for me?" we can tell them to run python -c 'import pyds9; pyds9.test()' and paste the output (which can include useful info like version numbers as well)'

Just as an example ... I did this with another package 5 min ago: https://gist.github.com/cdeil/76b3657af284f98767e4

And as far as I can see, there's no downside to having tests located in the package.

[montefra]

ok: you convinced me. Thanks for the reply

Updated directory structure

├── changelog
├── COPYING
├── docs
│     ├── source
│     │     ├── conf.py
│     │     ├── index.rst
│     │     └── [...]
│     └── Makefile
├── MANIFEST.in  # Define the non python files that need to be installed (like example files
├── pyds9
│     ├── __init__.py  # from pyds9 import * or only what we want a public interface. This way we maintain backward compatibility 
│     ├── data
│     │     ├── test.fits
│     ├── core.py
│     ├── tests
│     │    └──  [...]  # test modules and data
│     ├── xpa
│     │    └── [...]
│     └── xpa.py
├── README
└── setup.py

[cdeil]

I just thought of one more thing ...

What about the xpa folder and xpa.py?

Is xpa a bundled version of the xpa library? If so, it should probably be moved to pyds9/extern to make this clear and avoid possible confusion between pyds9/xpa.py and pyds9/xpa/?

This will also make it easier to package pyds9 in the future ... there's already an xpa package e.g. in Debian or Macports and at least Debian will not accept that pyds9 installs a separate bundled version, i.e. when packaging pyds9 for Debian (someone, some day), the bundled xpa has to be removed, and that's easiest if it's in extern (and ideally setup.py already contains an option to use the system version).

Again, my suggestion would be to just follow how Astropy does it, and put xpa in a cextern or extern folder.

[montefra]

In astropy cextern is on the top level.

About the installation: I guess that we can check if libxpa already exists and, if possible, if it's in a given range of version. If yes use it or copy it in the pyds9 directory. If not build it.

Would it make sense?

[cdeil]

I have to admit I never looked at how cextern is integrated in the Astropy build. I guess as long as python setup.py build puts what's needed in the _build folder it's OK, and even the tests could be in a separate folder in the source tree. The question is what happens when python setup.py build_ext --inplace is run, i.e. if you want to support that for pyds9? (Just to be clear ... whatever solution you choose is fine with me ... as simple as possible is good, i.e. IMO in-place builds don't have to be supported because they can get confusing very quickly.)

Concerning the question when to use the system XPA lib versus the bundled one. If you want to make this nice for the user, yes, you can implement checks.

IMO it would also be OK to leave this to the user, to configure this via the setup.py call. I.e. it's just important that an option like --use-system-xpa is available. Example how that's then used in distributions: https://trac.macports.org/browser/trunk/dports/python/py-astropy/Portfile#L29

[cdeil]

Actually @ericmandel ... is it a good idea to bundle XPA in pyds9 in the first place instead of putting it as a dependency? Doesn't this create extra maintenance work if more than one Python package uses it?

[montefra]

Ok. Thanks for the time and the pointers. I'll try to figure out how astropy does it.

[ericmandel]

IMO it's important that a Python package only installs stuff in one namespace, so :+1: to remove the ds9.py that's still there for backwards-compatibility. Although probably @ericmandel should decide if this can be done now or at which point it's OK to remove that.

If best practice implies the removal of ds9.py so that one cannot pollute other namespaces, I have no objection to this change. (It's not the same as changing the API while still providing get() and set() for backward compatibility.) The only caveat I would mention is that @montefra needs to be prepared to respond to "I can't do 'from pyds9 import *' anymore" issues. The last release of DS9 made a non-backwards-compatible change and the Chandra CIAO group actually had to put up a special Web page to deal with all of the complaints/queries from X-ray users.

You might want to tag the current directory setup as 1.7 and make the reorganization 1.8, just to make clear that its a non-trivial change.

[ericmandel]

re: MANIFEST vs. MANIFEST.in

I think MANIFEST was generated from MANIFEST.in at some stage of the documentation generation process. But I did this six years ago and recall the exact process. Again, your knowledge of current best practices will trump whatever I did by reading a book back then.

[montefra]

about version see this

We will try to make the from pyds9 import * work: shouldn't be too hard. import ds9 will stop working. But again there is a warning about this since one year: I would say that this is enough time

[ericmandel]

Is xpa a bundled version of the xpa library? If so, it should probably be moved to pyds9/extern to make this clear and avoid possible confusion between pyds9/xpa.py and pyds9/xpa/?

Yes, it's a bundled version, which I did not know how to deal with back then.

[montefra]

Ok. Then we should follow astropy conventions. They are probably widely used and known in the community and are fairly well documented.

[ericmandel]

About the installation: I guess that we can check if libxpa already exists and, if possible, if it's in a given range of version. If yes use it or copy it in the pyds9 directory. If not build it.

All XPA 2.1.x versions are compatible (this goes back to 2002!). Python also will need to be able to find the xpans (name server) program in the path, for cases where Python starts up DS9, and DS9 starts up xpans.

[ericmandel]

IMO it would also be OK to leave this to the user, to configure this via the setup.py call.

One caveat is that you have to ensure that the library matches Python properly. If you look at setup.py, you'll see that "-m32" is sometimes added to the link line to build a 32-bit version of XPA. So if the user built a 64-bit version by default, but is using a 32-bit version of Python, pyds9 has to build the XPA library to match. Perhaps this is no longer an issue?

[ericmandel]

Actually @ericmandel ... is it a good idea to bundle XPA in pyds9 in the first place instead of putting it as a dependency? Doesn't this create extra maintenance work if more than one Python package uses it?

See above regarding 32-bit vs 64-bit libraries ... six years ago this definitely was an issue, but perhaps it's gone away?

[montefra]

@ericmandel : I have no idea.

I propose that we migrate to the new directory structure, with xpa in cextern, but we build xpa as now did.

Once that is done and stable we can think about allowing to use system library and figuring out all the issues about what files are needed, which versions, which architectures, ... We will open a new issue when the time will come.

[ericmandel]

We will try to make the from pyds9 import * work: shouldn't be too hard. import ds9 will stop working. But again there is a warning about this since one year: I would say that this is enough time

Agreed

[ericmandel]

I propose that we migrate to the new directory structure, with xpa in cextern, but we build xpa as now did.

Agreed

[montefra]

@cdeil @ericmandel : I'm going to redo the directory structure using the astropy template before working on #18

[montefra]

done.