Load six from system package or bundled copy

[sergiopasra]

This changes the way astropy.extern.six is imported. When astropy.extern.six is imported, the code in the module tries to import six from system packages. If is found and the version is equal or greater to the version of the bundled copy, then system six and six.moves are inserted in sys.modules, with the keys astropy.extern.six and astropy.six.moves

If it is not found or the version is lesser than the bundled copy, the bundled copy gets inserted in sys.modules instead.

With this PR, packagers of astropy do not have to worry about unbundling six, it will work with the system library if available.

Other bundled libraries will follow if this is approved

[mdboom]

👍 I'm not sure about the new directory "bundled" though, as everything in "extern" is already "bundled" as it were. Can you just put six.py in extern/six and import from there (if the system package is missing?)

[mdboom]

Also -- whatever solution we come up with here should be (ideally) generalizable to the other external dependencies we include (configobj and py.test). (Though the bundling of py.test is already kind of a special case -- that probably doesn't have to change).

[sergiopasra]

After trying different things I found that the best way is to put the bundled libraries in a different subpackge, not under the same package you are importing. At first I put the bundled six as a file _six.py under astropy/extern/six but I found it confusing. You would end up importing astropy.extern.six._six

What is possible is that I can use a six.py module instead of a six subpackage. I can test it.

If something along the line of this gets approved, I will do the same for the others, configobj and ply, yes

[mdboom]

@sergiopasra: That sounds like a good idea. Go ahead and update this PR, and I'm glad to have another look.

[sergiopasra]

@mdboom I have changed the six subpackage to a six module. The module does work as the subpackage

[mdboom]

Thanks for the update. It does seem like the Travis failure is somehow legitimate. Any thoughts about why that might be happening?

[sergiopasra]

No, no idea. It seems to fail in the only case where build_sphinx is run, Is that correct? But I can run build_sphinx -w in my system and it works.

[embray]

This seems unnecessarily complex and fragile to me. I'd rather just have modules in astropy perform imports of six from some subpackage, where the subpackage's __init__.py imports the module either from the locally bundled version, or with just import six.

This would require changing how some of Astropy's modules use. For example, from ..extern.six.moves import ... might not work, but that's okay.

An alternative might also be to make astropy.extern.six and actual package. Have astropy.extern.six.__init__ do from <actual six module> import * or even just copy its __dict__, and have an actual astropy.extern.six.moves which does the same thing. I would prefer that over manually messing with sys.modules which has too much potential to confuse the import system.

[sergiopasra]

@embray If you think this is complex, then you should see my first implementation, it used a CustomImporter class in sys.meta_path... I didn't even new there was a sys.meta_path

Anyway. My prefered solution would be to use always the system six library. This proposal exists only for:

• affilliated packages that require astropy.extern.something
• and use a packaged version (like Fedora's) that currently do not ship anything under astropy.extern

So we agree that the procedure is:

• you import astropy.extern.six
• the package/module in astropy.extern.six checks if system six exists and is suitable to be used (__version__ and/or defined symbols can be checked here)
• if it is, then astropy.extern.six is converted somehow to be the system six
• if it is not, then is converted somehow to be the bundled six

The discussion is about how you convert the imported six module into the real six module. Your approach has some disadvantages:

• from bla import * doesn't import everything always.
  • from six import * doesn't import six.moves, yes you can mimic it by creating a subpackage moves, but
  • from six.moves import * doesn't import the submodules inside, like urlib or tkinter. Creating a subpackage for each one, with basically repeated code, is going too far.
  • On the other hand, what I do is basically what six does: it creates an empty module object, the module is filled with other modules and then it is inserted in sys.modules. In this sense, I'm not doing anything different to what six does itself.
• You mention overwriting __dict__ for modules. That's explicitelly not recommended https://docs.python.org/2/library/stdtypes.html#modules

[embray]

Well, no, but neither is creating dummy modules and manually inserting them in to sys.modules. The fact that six does it is not a good thing--it's just what it has to do to accomplish its purpose. But okay, ultimately I won't stand in the way of this so long as we can figure out how to make it work. It's not obvious to me why it's failing in the docs build.

[embray]

I have another idea for this that might work. Hang tight...

[embray]

@sergiopasra I sent a PR to sergiopasra#1 with an alternative idea. It seems to work for me, but if you'd like to give it a try let me know how it works for you.

[embray]

One thing we might also want to add to this is that if the system version is found it should have some minimum __version__.

For example I just tried running setup.py build_sphinx using my patch from above and it blew up with

ImportError: cannot import name xmlrpc_client

My system version of six is 1.4.1. The bundled version is 1.5.2.

According to six's changelog it looks like for astropy we should currently require a minimum of 1.5.0.

[sergiopasra]

This last commit allows to check the __version__ of system sixmodule. Minimum is 1.5.0
@embray I agree that your solution sergiopasra#1 is simpler than mine, but how do you check the version there?

[embray]

imp.load_module can be called any number of times, really. So I think the thing to do would be to attempt loading the system module first, check its version, and then fall back on the bundled copy (if available). I'll see if I can update my PR.

[embray]

Okay, I rebased my PR and added a version check as well.

One thing I also made different is that in my version the bundled copy of 'six' is checked for first. The reason for this is that if users install Astropy themselves (via pip install, etc.) I'd much rather the bundled version be the default, rather than whatever "six" module happens to be floating around on the user's system which could be broken or not even the right thing (this is less of a problem on a more managed system like Debian or RH).

For the system package you could either change the order or, if the bundled copy is removed from the source RPM altogether then the check for the bundled copy can be removed entirely as well. Does that sound okay? If there's anything else that can be done to make this easier to tweak I'm all for it, just as long as the main Astropy source defaults to using the bundled copy.

[sergiopasra]

@embray thanks!, I have merged your PR. For the check order, I'm ok with it. With this configuration, to load the system module I just need to remove six.py from bundled, or change the order of the check with a patch. It's easier than patching than whole package as I was doing

[sergiopasra]

I have to document several things. For example, if newer features of six are used, the SIX_MEAN_VERSION has to be changed. Other thing worth mentioning is that if you remove bundled/six.py, astropy will try import system six. Are there recomendations for packagers in the documentation or perhaps a README inside bundled is enough?

[embray]

Maybe just a README directly in astropy/extern would be fine.

[embray]

This was marked for 1.0.0 but I moved it to 0.4.1. Things that make system packagers' lives easier can generally go in a bugfix release I think.

[embray]

I manually merged this PR via f7a0814 (strange that GitHub doesn't seem to be making the cross reference) to include a changelog entry and the aforementioned README.

[sergiopasra]

Ups, sorry, I haven't had the time to complete this. @embray thanks for doing it yourself

[mdmueller]

Wasn't sure if I should open a new issue for this: is there a reason why the bundled copy of six isn't the most recent version? I was recently testing out a feature involving pickling on Windows and came across an interesting error:

Traceback (most recent call last):
  File "test.py", line 2, in <module>
    ascii.read('a b c\n1 2 3', format='basic', guess=False, use_fast_reader=True, parallel=True)
  File "C:\Users\mmueller\astropy\astropy\io\ascii\ui.py", line 154, in read
    return fast_reader.read(table)
  File "C:\Users\mmueller\astropy\astropy\io\ascii\fastbasic.py", line 95, in read
    data = self.engine.read(try_int, try_float, try_string)
  File "cparser.pyx", line 330, in astropy.io.ascii.cparser.CParser.read (astropy\io\ascii\cparser.c:5614)
  File "cparser.pyx", line 394, in astropy.io.ascii.cparser.CParser._read_parallel (astropy\io\ascii\cparser.c:6783)
  File "C:\Users\mmueller\AppData\Local\Continuum\Anaconda\lib\multiprocessing\process.py", line 130, in start
    self._popen = Popen(self)
  File "C:\Users\mmueller\AppData\Local\Continuum\Anaconda\lib\multiprocessing\forking.py", line 277, in __init__
    dump(process_obj, to_child, HIGHEST_PROTOCOL)
  File "C:\Users\mmueller\AppData\Local\Continuum\Anaconda\lib\multiprocessing\forking.py", line 199, in dump
    ForkingPickler(file, protocol).dump(obj)
  File "C:\Users\mmueller\AppData\Local\Continuum\Anaconda\lib\pickle.py", line 225, in dump
    self.save(obj)
  File "C:\Users\mmueller\AppData\Local\Continuum\Anaconda\lib\pickle.py", line 332, in save
    self.save_reduce(obj=obj, *rv)
  File "C:\Users\mmueller\AppData\Local\Continuum\Anaconda\lib\pickle.py", line 420, in save_reduce
    save(state)
  File "C:\Users\mmueller\AppData\Local\Continuum\Anaconda\lib\pickle.py", line 287, in save
    f(self, obj) # Call unbound method with explicit self
  File "C:\Users\mmueller\AppData\Local\Continuum\Anaconda\lib\pickle.py", line 650, in save_dict
    self._batch_setitems(obj.iteritems())
  File "C:\Users\mmueller\AppData\Local\Continuum\Anaconda\lib\pickle.py", line 682, in _batch_setitems
    save(v)
  File "C:\Users\mmueller\AppData\Local\Continuum\Anaconda\lib\pickle.py", line 287, in save
    f(self, obj) # Call unbound method with explicit self
  File "C:\Users\mmueller\AppData\Local\Continuum\Anaconda\lib\pickle.py", line 740, in save_global
    module = whichmodule(obj, name)
  File "C:\Users\mmueller\AppData\Local\Continuum\Anaconda\lib\pickle.py", line 819, in whichmodule
    if name != '__main__' and getattr(module, funcname, None) is func:
  File "C:\Users\mmueller\astropy\astropy\extern\six.py", line 116, in __getattr__
    _module = self._resolve()
  File "C:\Users\mmueller\astropy\astropy\extern\six.py", line 105, in _resolve
    return _import_module(self.mod)
  File "C:\Users\mmueller\astropy\astropy\extern\six.py", line 76, in _import_module
    __import__(name)
ImportError: No module named gdbm

It turns out this is a known issue with six < 1.7.0 -- basically, six used to add MockModule objects to sys.modules, which broke the pickle system of searching through sys.modules for a given function in certain circumstances. The above example works with the most recent six version, and I'm not sure if there's an easy solution without updating six.

[astrofrog]

@amras1 - I think because they release versions fast and we haven't updated in the last few months ;) Feel free to update the bundled version with a PR!

[mdmueller]

Okay, sounds good!