Opened 7 years ago

Closed 4 years ago

Last modified 3 years ago

#15985 closed enhancement (fixed)

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

Reported by: wluebbe Owned by:
Priority: major Milestone: sage-7.3
Component: python3 Keywords: python3
Cc: tscrim, jmantysalo, jdemeyer Merged in:
Authors: Wilfried Luebbe Reviewers: Jeroen Demeyer
Report Upstream: N/A Work issues:
Branch: e2f59c2 (Commits) Commit:
Dependencies: Stopgaps:

Description

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.

Change History (21)

comment:1 Changed 7 years ago by wluebbe

  • Type changed from defect to enhancement

comment:2 Changed 6 years ago by wluebbe

  • Branch set to u/wluebbe/ticket/15985
  • Commit set to 502ac5c98586116e74521bde30fb4c84edb507f5
  • Status changed from new to needs_review

New commits:

502ac5clocal run of futurize -w -f absolute_import

comment:3 Changed 6 years ago by wluebbe

  • Status changed from needs_review to needs_work

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

comment:4 Changed 6 years ago by git

  • Commit changed from 502ac5c98586116e74521bde30fb4c84edb507f5 to cde9b0dc8d749e32ce47ce34f3cc1131e4d3ef6e

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

cde9b0dlocal run of futurize -w -f absolute_import

comment:5 Changed 6 years ago by wluebbe

Rebased on 6.2.beta6 and resolved merge conflicts.

comment:6 Changed 6 years ago by ohanar

  • Authors set to Wilfried Luebbe
  • Branch changed from u/wluebbe/ticket/15985 to u/ohanar/relative_imports
  • Commit changed from cde9b0dc8d749e32ce47ce34f3cc1131e4d3ef6e to 19692cba6ef085f5563f4302f34d6cb5c9a281e5

New commits:

19692cbimports: fix a few automated mistakes

comment:7 Changed 6 years ago by ohanar

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.

comment:8 Changed 6 years ago by ohanar

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.

comment:9 Changed 6 years ago by wluebbe

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?

comment:10 Changed 6 years ago by ohanar

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).

comment:11 Changed 6 years ago by wluebbe

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.

comment:12 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.2 to sage-6.3

comment:13 Changed 6 years ago by vbraun_spam

  • Milestone changed from sage-6.3 to sage-6.4

comment:14 Changed 4 years ago by chapoton

  • Milestone changed from sage-6.4 to sage-7.3

Let us do that chunk by chunk.

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

Last edited 4 years ago by chapoton (previous) (diff)

comment:15 Changed 4 years ago by chapoton

  • Component changed from distribution to python3

comment:16 Changed 4 years ago by chapoton

Almost done, last step is #21036

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

comment:17 follow-up: Changed 4 years ago by chapoton

  • Branch changed from u/ohanar/relative_imports to public/15985
  • Cc tscrim jmantysalo jdemeyer added
  • Commit changed from 19692cba6ef085f5563f4302f34d6cb5c9a281e5 to e2f59c294fe0dd027b981e1945f21ae97bad6dfb
  • Status changed from needs_work to needs_review

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

comment:18 follow-up: Changed 4 years ago by jdemeyer

  • Reviewers set to Jeroen Demeyer
  • Status changed from needs_review to positive_review

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

comment:19 Changed 4 years ago by vbraun

  • Branch changed from public/15985 to e2f59c294fe0dd027b981e1945f21ae97bad6dfb
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:20 in reply to: ↑ 18 Changed 3 years ago by jdemeyer

  • Commit e2f59c294fe0dd027b981e1945f21ae97bad6dfb deleted

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.

comment:21 in reply to: ↑ 17 Changed 3 years ago by jdemeyer

Replying to chapoton:

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.

Note: See TracTickets for help on using tickets.