Remove unneeded imports

[robertwb]

As a first step in simplifying the import hierarchy, remove obviously unused imports.

---

The patch below is the result of applying Robert's original patch (which was based on Sage 4.7.1) to the (presumably) final version of Sage 4.7.2.alpha3, ignoring rejects, and with some manual corrections due to afterwards occurring import errors when trying to start up Sage or running ptestlong.

The current omissions will be dealt with on follow-up tickets.

---

Apply only trac_11749-remove_unneeded_imports-rebased_on_4.7.2.alpha3.reviewer.patch​ to the Sage library.

[kini]

I'm ptestlonging this on sage.math right now.

[kini]

Incidentally how did you generate this patch?

[kini]

ptestlong passes! Before I mark this positive review I would like to know how you generated the patch, though. Just tracing imports and finding those which are not used?

[leif]

Really nice! This patch will save a lot of disk space! :D

[robertwb]

I generated the patch with this code, plus a handful (less than a dozen) manual touch-ups where, e.g., symbols were used in eval() strings but not directly.

import symtable
import ast
import re

class GlobalImports(ast.NodeVisitor):
    
    func_depth = 0
    
    def __init__(self):
        self.modules = {}
        self.names = {}
        self.used = {}
    
    def visit_FunctionDef(self, node):
        self.func_depth += 1
        self.generic_visit(node)
        self.func_depth -= 1
        
    def visit_Import(self, node):
        if self.func_depth == 0:
            for alias in node.names:
                self.modules[alias.asname or alias.name] = alias.name
    
    def visit_ImportFrom(self, node):
        if self.func_depth == 0:
            for alias in node.names:
                if alias.name != '*':
                    self.names[alias.asname or alias.name] = (node.module, alias.name)
    
    def visit_Name(self, node):
        self.used[node.id] = True

def list_unused(file, source=None):
    source = source or open(file).read()
    tree = ast.parse(source)
    visitor = GlobalImports()
    visitor.visit(tree)
#    print visitor.modules
#    print visitor.names
#    print visitor.used
    all = dict(visitor.modules)
    all.update(visitor.names)
    for name in all:
        if name not in visitor.used:
            if '.' in name:
                if source.count(name) == 1:
                    print "Unused", name
            else:
                print "Unused", name
#    symtab = symtable. symtable(source, file, 'exec')
#    return symtab

def extract_used(source):
    visitor = GlobalImports()
    tree = ast.parse(source)
    visitor.visit(tree)
    return visitor.used

def prune_unused_one(file, source=None):
    if source is None:
        source = open(file).read()
    used = extract_used(source)
    import_matcher = re.compile(r'(from (\S+) import (.*))|(import (.*))').match
    
    bad_name = re.compile('[()#]').search
    lines = source.split('\n')
    for lineno, line in enumerate(lines):
        m = import_matcher(line)
        if m:
            g = m.groups()
            if g[0]:
                if g[1] == '__future__':
                    continue
                names = g[2].split(',')
            else:
                names = g[4].split(',')
            #print names
            for ix, name in enumerate(names):
                name = name.strip()
                if bad_name(name) or name in ('', '*', '\\'):
                    continue
                if ' as ' in name:
                    name, alias = name.split(" as ")
                    name = name.strip()
                    alias = alias.strip()
                else:
                    alias = name
                if alias not in used:
                    if "." in alias:
                        unused = source.count(alias) == 1
                    else:
                        unused = True
                    if unused:
                        print "Unused %s%s" % (g[1] + "." if g[1] else "", name)
                        names[ix] = None
            if None in names:
                new_names = ", ".join(name.strip() for name in names if name is not None)
                #print names, "->", new_names
                if new_names:
                    if g[0]:
                        lines[lineno] = "from %s import %s" % (g[1], new_names)
                    else:
                        lines[lineno] = "import %s" % new_names
                else:
                    lines[lineno] = None
                
    return "\n".join(line for line in lines if line is not None)

def prune_unused_all(sage_root):
    for root, dirs, files in os.walk('%s/devel/sage/sage/' % sage_root):
        for file in files:
            file = os.path.join(root, file)
            if file.endswith('.py'):
                if os.path.basename(file) in ('__init__.py', 'all.py', 'interpreter.py'):
                    continue
                source = open(file).read()
                try:
                    new_source = prune_unused_one(file, source)
                    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

Then

prune_unused_all('/Users/robertwb/sage/sage-4.7.1')

[kini]

Nice. Positive review from me.

BTW for future reference you can syntax-highlight python code in trac comments by putting #!python on the first line of the raw content inside triple braces (see the main page of Sage trac which I added some examples of this to).

[leif]

Ooops. I wrote this a few hours ago, but forgot to click the submit button.

Replying to robertwb:

I generated the patch with this code, plus a handful (less than a dozen) manual touch-ups where, e.g., symbols were used in eval() strings but not directly.

Ok, so it is rather conservative with the exception of eval() and friends.

We should perhaps especially inspect those files modified that have a low "coverage"(though I strongly doubt the doctests of others will cover most execution paths...).

I think more sophisticated detection of superfluous imports can be left for follow-up tickets, just like the inclusion of Cython files.

The patch is certainly a big step forwards as is.

---

P.S.: I'm pretty sure Robert knows about trac's wiki markup for source code etc. ;)

[kini]

I'm pretty sure Robert knows whatever I'm likely to say on trac, but hey :P someone reading might not know.

[leif]

Attachment comments shouldn't hurt either... ;-)

[robertwb]

Well, they do take time and screen real estate, and are 100% redundant when there's a single attached file to apply to the Sage library, but I won't complain too much :-).

[leif]

Of course^TM this patch bomb doesn't apply after only a few other patches have been applied.

Should we just merge those parts that do (changing the patch here later), and put the others onto a follow-up?

[robertwb]

Yes, simply ignore the rejects when you merge it and post then to a follow-up. 99% of the changes are completely independent of each other, and otherwise this will never actually get in.

[kini]

As the patch is more or less automatically generated, why not just merge it last? Generate a patch using Robert's code snippet above, inspect for less than a dozen manual fixes, and done. (Make sure of course to hg qrefresh -U "Robert Bradshaw <robertwb@math.washington.edu>" first :) ) Not sure how time consuming this manual inspection is, but maybe it's worth a shot? As opposed to losing some of the changes, I mean.

[robertwb]

Manual inspection takes quite a while, and the patch is not entirely automatically generated. I'd rather see (the bulk of) this patch merged in sooner rather than later, and especially avoid playing some right-before-the-release gymnastics. The changes won't be lost--we can record what the rejects were and even re-run the script at a later date (which should be much faster and produce a much smaller, easier to inspect output).

[kini]

Oh, OK then. Sounds good!

[leif]

Replying to robertwb:

Manual inspection takes quite a while, and the patch is not entirely automatically generated. I'd rather see (the bulk of) this patch merged in sooner rather than later, and especially avoid playing some right-before-the-release gymnastics. The changes won't be lost--we can record what the rejects were and even re-run the script at a later date (which should be much faster and produce a much smaller, easier to inspect output).

That's what I intended; i.e., merge the patch as soon as I've sorted out which tickets are really ready to get merged into Sage 4.7.2.alpha3, replace the patch on this ticket by what actually applied, and put the rejects on a follow-up for the next (devel) release. There we can rerun the script on the then current release.

I've only set it to "sage-pending" to back it out from my merge attempts, not to postpone it to another release.

[leif]

Reviewer patch. Robert's patch rebased on the "final preliminary" Sage 4.7.2.alpha3. Apply only this one.

[leif]

Fixed. :)

[leif]

We shouldn't forget to open a follow-up.

I'll attach the logs later, either here or to another ticket in case someone opened one then...

[kini]

Followup ticket is at #11811.

[nthiery]

I understand the rationale for that patch, but I hope you realize that such patch bombs inflict a lot of rebasing work. I just spent half a day, if not more, rebasing the sage-combinat patch queue. Please, please let us know in advance about it next time.

Cheers,

Nicolas

[kini]

Sorry Nicolas, I didn't know about your guys' hundreds of patches queue when I reviewed this ticket :( Next time I'll cc you to tickets I see with broad patches.

[nthiery]

Replying to kini:

Sorry Nicolas, I didn't know about your guys' hundreds of patches queue when I reviewed this ticket :( Next time I'll cc you to tickets I see with broad patches.

Thanks!

[mderickx]

Did you see the follow up of this ticket also Nicolas? It's #11762 it also touches quite a lot, but it are just init.py files so it should not conflict a lot with your patch queue.