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

Already on GitHub? Sign in to your account

change cbook to relative import #1637

Merged
merged 2 commits into from Jan 10, 2013

Conversation

Projects
None yet
6 participants
Contributor

jakevdp commented Jan 5, 2013

In the current master, the following bug is present in python 3

 $ python3
Python 3.2.3 (default, Oct 19 2012, 20:10:41) 
[GCC 4.6.3] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import pylab
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/lib/python3.2/dist-packages/pylab.py", line 1, in <module>
    from matplotlib.pylab import *
  File "/usr/local/lib/python3.2/dist-packages/matplotlib/pylab.py", line 219, in <module>
    from cbook import flatten, is_string_like, exception_to_str, \
ImportError: No module named cbook

This PR changes cbook to a relative import, which works in both python 2.6+ and python 3+.

Member

pelson commented Jan 6, 2013

I'm not sure if there is a preference to using absolute vs relative imports in the mpl codebase. @mdboom - any preferences? @WeatherGod - I know you did some stuff with relative imports a while back, did we come up with a preferred standard to follow (i.e. .cbook vs matplotlib.cbook)?

Thanks for the fix @jakevdp.

Contributor

jakevdp commented Jan 6, 2013

I haven't been part of the discussion before, but with python 2.4 support dropped, is there any reason not to use relative imports within a module?

Member

WeatherGod commented Jan 7, 2013

My preference is for absolute imports.

@WeatherGod WeatherGod and 1 other commented on an outdated diff Jan 7, 2013

lib/matplotlib/pylab.py
@@ -216,7 +216,7 @@
from __future__ import print_function
import sys, warnings
-from cbook import flatten, is_string_like, exception_to_str, \
+from .cbook import flatten, is_string_like, exception_to_str, \
@WeatherGod

WeatherGod Jan 7, 2013

Member

Don't we have to import from future in order to do this in the 2.x series?

@jakevdp

jakevdp Jan 7, 2013

Contributor

No, relative imports are valid out-of-the-box in 2.5+.

Owner

mdboom commented Jan 7, 2013

My preference is for absolute imports (i.e. the "new" way and the default way in 3.x) as well. We have a somewhat commonly occurring problem in matplotlib, since it was a module called collections which conflicts with the stdlib module of the same name. The new absolute import behavior fixes this.

Contributor

jakevdp commented Jan 7, 2013

Where did you hear that absolute imports are the default way in python 3.x? I just reviewed a python 3 book by Dave Beazley where he recommends the opposite.

Member

WeatherGod commented Jan 7, 2013

This can be a bit confusing. What happens in py3k is that the default "interpretation" of an import changes from implicitly relative to implicitly absolute. With py25 and above, you gain the ability to explicitly state that an import is relative.

What a project chooses to do is up to the project (and I prefer absolutes), it is just now completely unambiguous in py3k what is meant.

Owner

mdboom commented Jan 7, 2013

The new behavior, which is the default in 2.7 and above, is turned on in older versions by saying

from __future__ import absolute_import

It's this "mode" that I was referring to and saying I preferred.

What this does is turn off the "look first locally, then globally" behaviour. So a import collections from a matplotlib module would import the stdlib module, not the one in matplotlib. To get the one in matplotlib, one would say from . import collections (which, yes, is a relative import).

It's all confusingly named -- we want to use the "absolute import by default" behavior of the new Pythons, with the result that most of our imports become relative.

Contributor

jakevdp commented Jan 7, 2013

Ah, I see.
So do you prefer from matplotlib.cbook import flatten or from .cbook import flatten? It's the latter construct that was recommended in Beazley's book, though he notes that both are valid code. It is unannotated relative imports like from cbook import flatten that cause an error in python 3 (and, I assume, in 2.x under the absolute_import mode) though it does not raise an error under 2.7.

Member

pelson commented Jan 7, 2013

Personally: the former, but I wouldn't be upset if we all agreed on the latter...

Just for context, it is worth reading the relevant PEP on this: http://www.python.org/dev/peps/pep-0328/#rationale-for-relative-imports

Contributor

NelleV commented Jan 7, 2013

I prefer the latter, but I wouldn't upset if we all agree on the former. In fact, I think the preferred way in matplotlib is the former.

Owner

mdboom commented Jan 7, 2013

I'm with @NelleV on this... I don't think there's a strong objective argument either way. While relative imports make some refactorings easier, it makes others harder. Once we reach consensus, though, let's not forget to update the coding standards.

Contributor

NelleV commented Jan 8, 2013

There are other modules that have implicite relative imports which are not supported in the future (the module table for examples) that need fixing. I'll submit a PR as soon as we've decided what is the best approach here.

Owner

efiring commented Jan 8, 2013

On 2013/01/08 1:30 AM, Varoquaux wrote:

There are other modules that have implicite relative imports which are
not supported in the future (the module table for examples) that need
fixing. I'll submit a PR as soon as we've decided what is the best
approach here.

I agree that there seems to be no strong argument we can use to decide
between relative and absolute. I think that standardizing on absolute
would involve fewer changes, although I haven't tried to quantify that.
I have a mild preference for absolute.

Member

pelson commented Jan 9, 2013

Ok, because I think we are better having some decision over none, and there is no compelling argument either way, I'm going to stick my head above the parapet and say: "Lets go with absolute imports".

@mdboom as bd please feel free to trump my statement if you would prefer we fell on the other side of the knife.

Owner

mdboom commented Jan 9, 2013

@pelson: Sure, let's go with that. It does seem to be the more common choice in the existing code base.

Contributor

jakevdp commented Jan 9, 2013

Great - I changed the commit to an absolute import. This should be OK

Member

pelson commented Jan 10, 2013

Thanks @jakevdp . Merging.

@pelson pelson added a commit that referenced this pull request Jan 10, 2013

@pelson pelson Merge pull request #1637 from jakevdp/rel_import
Fixed pylab for python3.
24c1417

@pelson pelson merged commit 24c1417 into matplotlib:master Jan 10, 2013

1 check failed

default The Travis build failed
Details
Member

pelson commented Jan 10, 2013

Post merge brainwave: presumably we do not test pylab at all if this was not picked up by travis / the python3 migration. I think we should consider adding such a test to at least import pylab. Any thoughts?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment