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

Python 3 preparation: Fix implicit relative imports (from sibling modules) #15985

Closed
wluebbe mannequin opened this issue Mar 20, 2014 · 27 comments
Closed

Python 3 preparation: Fix implicit relative imports (from sibling modules) #15985

wluebbe mannequin opened this issue Mar 20, 2014 · 27 comments

Comments

@wluebbe
Copy link
Mannequin

wluebbe mannequin commented Mar 20, 2014

Python 3 accepts only absolute imports and explicit relative imports.
To be Python 2 compatible all affected modules need from `future` import absolute_import.

lib2to3/fixes/fix_import.py does not add this __future__ import.

Therefore we intend to use an enhanced fixer from https://pypi.python.org/pypi/future/0.11.3.

The latest development version also handles imports of pyx modules.

Changes according to libfuturize/fixes/fix_absolute_imports.py:

If spam is being imported from the local directory, this import:
    from spam import eggs
becomes:
    from .spam import eggs

and this import:
    import spam
becomes:
    from . import spam

This ticket is tracked as a dependency of meta-ticket ticket:15980.

CC: @tscrim @jm58660 @jdemeyer

Component: python3

Author: Wilfried Luebbe

Branch: e2f59c2

Reviewer: Jeroen Demeyer

Issue created by migration from https://trac.sagemath.org/ticket/15985

@wluebbe wluebbe mannequin added this to the sage-6.2 milestone Mar 20, 2014
@wluebbe wluebbe mannequin added c: distribution and removed t: bug labels Mar 20, 2014
@wluebbe
Copy link
Mannequin Author

wluebbe mannequin commented Apr 1, 2014

Branch: u/wluebbe/ticket/15985

@wluebbe
Copy link
Mannequin Author

wluebbe mannequin commented Apr 1, 2014

New commits:

502ac5clocal run of futurize -w -f absolute_import

@wluebbe
Copy link
Mannequin Author

wluebbe mannequin commented Apr 1, 2014

Commit: 502ac5c

@wluebbe wluebbe mannequin added the s: needs review label Apr 1, 2014
@wluebbe
Copy link
Mannequin Author

wluebbe mannequin commented Apr 1, 2014

comment:3

Ticket needs work :-(

make failed while building html-doc:

byte-compiling /home/wluebbe/sage-6.2.beta4/local/lib/python2.7/site-packages/sage/probability/random_variable.py to random_variable.pyc
running install_egg_info
Removing /home/wluebbe/sage-6.2.beta4/local/lib/python2.7/site-packages/sage-6.2.beta5-py2.7.egg-info
Writing /home/wluebbe/sage-6.2.beta4/local/lib/python2.7/site-packages/sage-6.2.beta5-py2.7.egg-info

real	10m2.009s
user	35m33.530s
sys	0m48.657s
[ -f local/etc/sage-started.txt ] || local/bin/sage-starts
build/pipestatus "./sage --docbuild --no-pdf-links all html  2>&1" "tee -a logs/dochtml.log"
Traceback (most recent call last):
  File "/home/wluebbe/sage-6.2.beta4/src/doc/common/builder.py", line 1465, in <module>
    import sage.all
  File "/home/wluebbe/sage-6.2.beta4/local/lib/python2.7/site-packages/sage/all.py", line 87, in <module>
    from sage.misc.all       import *         # takes a while
  File "/home/wluebbe/sage-6.2.beta4/local/lib/python2.7/site-packages/sage/misc/all.py", line 94, in <module>
    from .functional import (additive_order,
  File "/home/wluebbe/sage-6.2.beta4/local/lib/python2.7/site-packages/sage/misc/functional.py", line 37, in <module>
    from sage.rings.complex_double import CDF
  File "integer.pxd", line 9, in init sage.rings.complex_double (sage/rings/complex_double.c:17880)
  File "integer.pyx", line 179, in init sage.rings.integer (sage/rings/integer.c:40148)
  File "/home/wluebbe/sage-6.2.beta4/local/lib/python2.7/site-packages/sage/rings/infinity.py", line 203, in <module>
    import sage.rings.rational
  File "fast_arith.pxd", line 5, in init sage.rings.rational (sage/rings/rational.c:29096)
  File "fast_arith.pyx", line 51, in init sage.rings.fast_arith (sage/rings/fast_arith.c:8036)
  File "integer_ring.pyx", line 65, in init sage.rings.integer_ring (sage/rings/integer_ring.c:12457)
  File "/home/wluebbe/sage-6.2.beta4/local/lib/python2.7/site-packages/sage/rings/rational_field.py", line 54, in <module>
    from . import rational
ImportError: cannot import name rational
make: *** [doc-html] Error 1

The problem occurs while trying to import the cython module ration.pyx. But rational.sois in the same subdir. So the explicit relative import should succeed.

wluebbe@Willis-Ubuntu:~/sage-6.2.beta4/local/lib/python2.7/site-packages/sage/rings$ ls -l ratio*
-rw-r--r-- 1 wluebbe wluebbe   28433 Apr  1 10:58 rational_field.py
-rw-r--r-- 1 wluebbe wluebbe   34240 Apr  1 11:08 rational_field.pyc
-rwxr-xr-x 1 wluebbe wluebbe 1666692 Apr  1 11:06 rational.so

@wluebbe wluebbe mannequin added s: needs work and removed s: needs review labels Apr 1, 2014
@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 3, 2014

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

cde9b0dlocal run of futurize -w -f absolute_import

@sagetrac-git
Copy link
Mannequin

sagetrac-git mannequin commented Apr 3, 2014

Changed commit from 502ac5c to cde9b0d

@wluebbe
Copy link
Mannequin Author

wluebbe mannequin commented Apr 3, 2014

comment:5

Rebased on 6.2.beta6 and resolved merge conflicts.

@ohanar
Copy link
Member

ohanar commented Apr 3, 2014

New commits:

19692cbimports: fix a few automated mistakes

@ohanar
Copy link
Member

ohanar commented Apr 3, 2014

Author: Wilfried Luebbe

@ohanar
Copy link
Member

ohanar commented Apr 3, 2014

Changed commit from cde9b0d to 19692cb

@ohanar
Copy link
Member

ohanar commented Apr 3, 2014

Changed branch from u/wluebbe/ticket/15985 to u/ohanar/relative_imports

@ohanar
Copy link
Member

ohanar commented Apr 3, 2014

comment:7

So it looks like the issue that you are getting into is the wonder that is affectionately referred to as Sage's import hell :).

Looking at the traceback, it looks like the issue is a circular import that mysteriously works right now: sage.rings.rational imports something from sage.rings.fast_arith which imports something from sage.rings.integer_ring which imports something from sage.rings.rational_field which imports sage.rings.rational.

This is one of the big reasons why Sage is not truly usable as a python library, because the order in which you import Sage is incredibly delicate, and seems to magically work (and as you can see from this patch which shouldn't change anything, it truly is magical). There hasn't been much effort to fix this, but I would say that getting Python 3 working is a pretty big motivator.

@ohanar
Copy link
Member

ohanar commented Apr 4, 2014

comment:8

One thing to try (which still might break) is removing all relative imports in favor of absolute imports, rather than just converting them to explicit relative imports.

@wluebbe
Copy link
Mannequin Author

wluebbe mannequin commented Apr 4, 2014

comment:9

Some numbers:

lines with "import" 26,3231
- thereof lines with "from sage" 19,581
- thereof lines with "import sage" 1,650
leaving lines with imports from Python
standard library and implicit relative import
(and 1 explicit relative import :-)
about 5,000

Questions:

  • Is there agreement in the Sage community that absolute import is the way to go?
  • Can it help with the above ImportError? And similar ones?
  • Other benefits or drawbacks?

@ohanar
Copy link
Member

ohanar commented Apr 4, 2014

comment:10

As your numbers indicate (and from my personal experience), there is definitely an unofficial convention to use absolute from..import statements in the sage library, i.e. from sage.foo import bar; however this is one of (if not the main) culprit behind the circular import issues of the Sage library. A lot of the circular import issues could be resolved by just importing a module, and not particular attributes of the module (i.e. using import sage.foo or import sage.foo as bar and using sage.foo.baz/bar.baz instead of from sage.foo import baz).

@wluebbe
Copy link
Mannequin Author

wluebbe mannequin commented Apr 4, 2014

comment:11

Can I read this proposal as:

Do the (absolute) imports as import sage.somewhere.amodule as am and then use it as am.a_function(a_parm).

Perhaps the module could recommend its "standard" abbreviation (here am). Such a convention might help while reading code that uses the module.

@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.2, sage-6.3 May 6, 2014
@sagetrac-vbraun-spam sagetrac-vbraun-spam mannequin modified the milestones: sage-6.3, sage-6.4 Aug 10, 2014
@fchapoton
Copy link
Contributor

comment:14

Let us do that chunk by chunk.

See #20958, #20960, #20955, #20945, #20947 for the first steps

@fchapoton fchapoton modified the milestones: sage-6.4, sage-7.3 Jul 6, 2016
@fchapoton
Copy link
Contributor

comment:16

Almost done, last step is #21036

there just remains a small problem in src/sage/misc/sagedoc.py

@fchapoton
Copy link
Contributor

Changed branch from u/ohanar/relative_imports to public/15985

@fchapoton
Copy link
Contributor

comment:17

Here is the very small last step. Hopefully the doctests will pass.

For the reviewer: to check, see that (after #21036 is closed)

futurize -w -f absolute_import src/sage

does nothing.


New commits:

e2f59c2trac 15985 last old-style import in misc

@fchapoton
Copy link
Contributor

Changed commit from 19692cb to e2f59c2

@jdemeyer
Copy link

Reviewer: Jeroen Demeyer

@jdemeyer
Copy link

comment:18

It's not really the last step, since there are still a lot of implicit relative imports in Cython files.

@vbraun
Copy link
Member

vbraun commented Jul 27, 2016

Changed branch from public/15985 to e2f59c2

@jdemeyer
Copy link

Changed commit from e2f59c2 to none

@jdemeyer
Copy link

comment:20

Replying to @jdemeyer:

It's not really the last step, since there are still a lot of implicit relative imports in Cython files.

See #22806 and #22808.

@jdemeyer
Copy link

comment:21

Replying to @fchapoton:

Here is the very small last step.

How did you determine that this is the "last step"? In other words, how did you determine which modules need to be fixed?

There are still plenty of .py and .pyx files which do not have from future import absolute_import.

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

No branches or pull requests

4 participants