Opened 8 years ago

Closed 8 years ago

Last modified 8 years ago

#11762 closed enhancement (fixed)

More import cleanup

Reported by: robertwb Owned by: jason
Priority: major Milestone: sage-4.8
Component: misc Keywords:
Cc: leif, mderickx Merged in: sage-4.8.alpha1
Authors: Robert Bradshaw Reviewers: Maarten Derickx
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description

Importing all from __init__.py simplifies and stabilizes the import order. Unfortunately the current state of imports depends on packages being partially loaded at different points in time, but this patch changes things where it can without breaking sage.

Attachments (1)

11762-import-cleanup.patch (15.3 KB) - added by robertwb 8 years ago.

Download all attachments as: .zip

Change History (12)

Changed 8 years ago by robertwb

comment:1 Changed 8 years ago by robertwb

  • Status changed from new to needs_review

Generated with

import re
import ast
ignore = re.compile('(file.*no.*empty)|(no.*empty.*file)')

def add_import_all(source, pkg):
    if 'import all' in source:
        return source
    lines = []
    for line in source.strip().split('\n'):
        if line and line[0] == '#':
            if line.strip('#').strip() in ('', pkg) or ignore.search(line[1:]):
                continue
        lines.append(line)
    if lines:
        lines.append('')
    lines.append('import all')
    lines.append('')
    return '\n'.join(lines).lstrip()

def import_all(sage_root):
    for root, dirs, files in os.walk('%s/devel/sage/sage/' % sage_root):
        if '__init__.py' in files and 'all.py' in files:
            file = os.path.join(root, '__init__.py')
            source = open(file).read()
            try:
                new_source = add_import_all(source, os.path.basename(root))
                if new_source != source:
                    ast.parse(new_source)
                    print file
                    open(file, 'w').write(new_source)
                    r = os.system("%s/sage -b > /dev/null 2> /dev/null" % sage_root)
                    if r == 0:
                        r = os.system("%s/sage -python -c 'import sage.all'" % sage_root)
                    if r != 0:
                        print "Failed!"
                        open(file, 'w').write(source)
                    else:
                        print "Good!"
            except Exception, exn:
                print file, exn

comment:2 Changed 8 years ago by leif

  • Authors set to Robert Bradshaw

comment:3 Changed 8 years ago by mderickx

Cool, could you explain (maybe with a small example if that is not a crazy amount of work) what the precise difference is this makes for importing. I might be ably to figure it out on myself, but since you probably already know it , I don't want to waste my time on thinking to much about the subtle differences between our recursive all.py structure, and just doing import all from init.py?

Or is there really not much of a difference and is this just a first step to better use of init.py?

comment:4 Changed 8 years ago by robertwb

Sorry I didn't see your comment earlier.

Basically, when you do import a.b.c it first loads a/__init__.py, then a/b/__init__.py, and finally a/b/c.py (or a/b/c/__init__.py).

Importing all imposes no restraint, so one can import sage.rings.foo.fragile_module without loading sage.ring.all. If fragile_module depends on stable_module being loaded first, then sage.rings.all can enforce this order, but only if sage.rings.all is loaded before import sage.rings.foo.fragile_module. Even when things aren't fragile, this ensures that the submodules of sage.rings get loaded in the order specified in all.py, greatly reducing the chance that a change in imports in an unrelated portion of code can trigger a chain reaction, import things in a different order, and have result in circular import errors. This will also help with performance, as one can see the actual load time of sage.rings.all without wondering about how many submodules were "preloaded." So it's both a cleanup and a prelude to further cleanup.

comment:5 follow-up: Changed 8 years ago by jhpalmieri

Would something like this also make sense, in some cases:

  • sage/algebras/all.py

    diff --git a/sage/algebras/all.py b/sage/algebras/all.py
    a b from algebra_element import AlgebraEleme 
    2929from free_algebra import FreeAlgebra, is_FreeAlgebra
    3030from free_algebra_quotient import FreeAlgebraQuotient
    3131
    32 from steenrod.all import *
     32from steenrod import *
    3333
    3434from group_algebra_new import GroupAlgebra

and then just rename sage/algebras/steenrod/all.py to sage/algebras/steenrod/__init__.py?

comment:6 Changed 8 years ago by robertwb

Yes, but that would involve changing a lot more code (and people's behavior).

comment:7 Changed 8 years ago by mderickx

Thanks for the explanation. It seems that this will indeed make the import order more stable, so from a logical point of view I give it a positive review. Luckily the patch still applies without errors to the just released sage-4.7.2. I'm now running all doctests just to be sure.

comment:8 Changed 8 years ago by mderickx

  • Status changed from needs_review to positive_review

All test passed on sage.math!

comment:9 Changed 8 years ago by jdemeyer

  • Reviewers set to Maarten Derickx

comment:10 Changed 8 years ago by jdemeyer

  • Merged in set to sage-4.8.alpha1
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:11 in reply to: ↑ 5 Changed 8 years ago by mhansen

Replying to jhpalmieri:

Would something like this also make sense, in some cases:

  • sage/algebras/all.py

    diff --git a/sage/algebras/all.py b/sage/algebras/all.py
    a b from algebra_element import AlgebraEleme 
    2929from free_algebra import FreeAlgebra, is_FreeAlgebra
    3030from free_algebra_quotient import FreeAlgebraQuotient
    3131
    32 from steenrod.all import *
     32from steenrod import *
    3333
    3434from group_algebra_new import GroupAlgebra

and then just rename sage/algebras/steenrod/all.py to sage/algebras/steenrod/__init__.py?

Also, for things like "sage.plot.plot" it could be confusing since if you just did "sage.plot.plot" it would be referring to the function (currently, "sage.plot.all.plot"). However, to import that function directly, it'd be "from sage.plot.plot import plot" which would also be the same as "from sage.plot import plot".

Note: See TracTickets for help on using tickets.