Opened 8 months ago

Closed 8 months ago

#16181 closed defect (fixed)

inject_shorthands broken since 6.2beta8

Reported by: darij Owned by:
Priority: critical Milestone: sage-6.2
Component: misc Keywords: globals, inject_shorthands, regression,
Cc: nthiery, vbraun Merged in:
Authors: Darij Grinberg Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 648da0d (Commits) Commit: 648da0d3ee5d408f210e3f36a66d6c4f5e1bffb6
Dependencies: Stopgaps:

Description

sage: NSym = NonCommutativeSymmetricFunctions(QQ)
sage: NSym.inject_shorthands()
Injecting S as shorthand for Non-Commutative Symmetric Functions over the Rational Field in the Complete basis
---------------------------------------------------------------------------
KeyError                                  Traceback (most recent call last)
<ipython-input-2-458277a04544> in <module>()
----> 1 NSym.inject_shorthands()

/home/darij/gitsage6.2/local/lib/python2.7/site-packages/sage/categories/sets_cat.pyc in inject_shorthands(self, verbose)
   1318                     if verbose:
   1319                         print 'Injecting %s as shorthand for %s'%(shorthand, realization)
-> 1320                     inject_variable(shorthand, realization)
   1321 
   1322             def realizations(self):

/home/darij/gitsage6.2/local/lib/python2.7/site-packages/sage/misc/misc.pyc in inject_variable(name, value)
   2356     # inject_variable is called not only from the interpreter, but
   2357     # also from functions in various modules.
-> 2358     G = get_main_globals()
   2359     if name in G:
   2360         warn("redefining global value `%s`"%name, RuntimeWarning, stacklevel = 2)

/home/darij/gitsage6.2/local/lib/python2.7/site-packages/sage/misc/misc.pyc in get_main_globals()
   2312     while True:
   2313         G = sys._getframe(depth).f_globals
-> 2314         if G["__name__"] == "__main__" and G["__package__"] is None:
   2315             break
   2316         depth += 1

KeyError: '__package__'

Same for Sym and QSym.

The get_main_globals method hasn't been changed since 2011, so I suspect something else is at fault. The attached branch gets rid of the bug, but I don't understand the code at hand and so it is not really merge-ready. Needs_review is to increase visibility.

Change History (24)

comment:1 Changed 8 months ago by darij

  • Status changed from new to needs_review

comment:2 Changed 8 months ago by darij

Oh, and apparently importing any(!) sage file beforehand keeps the bug from happening. Here is an example that does not throw errors:

sage: import sage.combinat.permutation
sage: NSym = NonCommutativeSymmetricFunctions(QQ)
sage: NSym.inject_shorthands()

comment:3 Changed 8 months ago by git

  • Commit changed from 67855fc66eed143172a9065928e52241d97edefe to 6e0f42f336ad394f05040e379252c18c5785967a

Branch pushed to git repo; I updated commit sha1. New commits:

6e0f42fAdded a doctest.

comment:4 Changed 8 months ago by tscrim

  • Cc vbraun added

Just doing

sage: sage.misc.misc.inject_variable('a', 0)

(before an import) is enough to cause the error. If I had to make a WAG, I'd say it's the IPython upgrade. I've added this as a doctest, so if you're happy this (and I'm okay with fixing the symptoms), positive review.

Last edited 8 months ago by tscrim (previous) (diff)

comment:5 Changed 8 months ago by darij

  • Cc vbraun removed

Unfortunately this doctest works even without the patch :(

And moving it to the very front of the file doesn't help either...

comment:6 Changed 8 months ago by darij

  • Cc vbraun added

Oops, sorry for removing you from cc, Volker (I really should report this as a trac bug -- I started writing my post before you made yours -- but I'm too lazy).

comment:7 Changed 8 months ago by tscrim

Hmm...there must be something going on within the doctesting framework that prevents this from begin executed before any imports. We can leave it in there as a human warning, but honestly noone is really going to look at that test (much less run it manually)...

comment:8 Changed 8 months ago by darij

BTW: when I do sys._getframe(0).f_globals, I get the following cruft:

/home/darij/gitsage6.2/local/lib/python2.7/site-packages/IPython/core/interactiveshell.py:2834: DeprecationWarning: this is being removed from the global namespace. Use crystals.elementary.<tab> instead
See http://trac.sagemath.org/15882 for details.
  exec code_obj in self.user_global_ns, self.user_ns
/home/darij/gitsage6.2/local/lib/python2.7/site-packages/IPython/core/interactiveshell.py:2834: DeprecationWarning: this is being removed from the global namespace. Use crystals.<tab> instead
See http://trac.sagemath.org/15882 for details.
  exec code_obj in self.user_global_ns, self.user_ns
/home/darij/gitsage6.2/local/lib/python2.7/site-packages/IPython/core/interactiveshell.py:2834: DeprecationWarning: this is being removed from the global namespace. Use crystals.AlcovePath instead
See http://trac.sagemath.org/15882 for details.
  exec code_obj in self.user_global_ns, self.user_ns
/home/darij/gitsage6.2/local/lib/python2.7/site-packages/IPython/core/interactiveshell.py:2834: DeprecationWarning: this is being removed from the global namespace. Use crystals.FastRankTwo instead
See http://trac.sagemath.org/15882 for details.
  exec code_obj in self.user_global_ns, self.user_ns
/home/darij/gitsage6.2/local/lib/python2.7/site-packages/IPython/core/interactiveshell.py:2834: DeprecationWarning: this is being removed from the global namespace. Use crystals.infinity.Tableaux instead
See http://trac.sagemath.org/15882 for details.
  exec code_obj in self.user_global_ns, self.user_ns
/home/darij/gitsage6.2/local/lib/python2.7/site-packages/IPython/core/interactiveshell.py:2834: DeprecationWarning: this is being removed from the global namespace. Use crystals.TensorProductOfKirillovReshetikhinTableaux instead
See http://trac.sagemath.org/15882 for details.
  exec code_obj in self.user_global_ns, self.user_ns
/home/darij/gitsage6.2/local/lib/python2.7/site-packages/IPython/core/interactiveshell.py:2834: DeprecationWarning: this is being removed from the global namespace. Use crystals.KyotoPathModel instead
See http://trac.sagemath.org/15882 for details.
  exec code_obj in self.user_global_ns, self.user_ns
/home/darij/gitsage6.2/local/lib/python2.7/site-packages/IPython/core/interactiveshell.py:2834: DeprecationWarning: this is being removed from the global namespace. Use crystals.Letters instead
See http://trac.sagemath.org/15882 for details.
  exec code_obj in self.user_global_ns, self.user_ns
/home/darij/gitsage6.2/local/lib/python2.7/site-packages/IPython/core/interactiveshell.py:2834: DeprecationWarning: this is being removed from the global namespace. Use crystals.DirectSum instead
See http://trac.sagemath.org/15882 for details.
  exec code_obj in self.user_global_ns, self.user_ns
/home/darij/gitsage6.2/local/lib/python2.7/site-packages/IPython/core/interactiveshell.py:2834: DeprecationWarning: this is being removed from the global namespace. Use crystals.KirillovReshetikhin with model='KR' instead
See http://trac.sagemath.org/15882 for details.
  exec code_obj in self.user_global_ns, self.user_ns
/home/darij/gitsage6.2/local/lib/python2.7/site-packages/IPython/core/interactiveshell.py:2834: DeprecationWarning: this is being removed from the global namespace. Use crystals.KirillovResetikhin instead
See http://trac.sagemath.org/15882 for details.
  exec code_obj in self.user_global_ns, self.user_ns
/home/darij/gitsage6.2/local/lib/python2.7/site-packages/IPython/core/interactiveshell.py:2834: DeprecationWarning: this is being removed from the global namespace. Use crystals.HighestWeight instead
See http://trac.sagemath.org/15882 for details.
  exec code_obj in self.user_global_ns, self.user_ns
/home/darij/gitsage6.2/local/lib/python2.7/site-packages/Babel-1.3-py2.7.egg/babel/core.py:14: ImportWarning: Not importing directory '/home/darij/gitsage6.2/local/lib/python2.7/site-packages/Babel-1.3-py2.7.egg/babel/localedata': missing __init__.py
  from babel import localedata

And this is just the error output. Here is the actual dictionary: https://www.dropbox.com/s/6hzuqm6jf9jo9jt/outtext.txt

I can't believe this all needs to be in the globals?

comment:9 Changed 8 months ago by tscrim

Those deprecations are from #15882 (ex. CrystalOfTableaux) where I used #14275 to (lazily) import them with a deprecation message and there was no good alternative that I could find (because I didn't want to write some 20 functions where all they did was deprecate something with a [semi]specific message). Moreover, I'd be surprised if they had any bearing on this issue.

comment:10 Changed 8 months ago by darij

Yeah, I now see it's pretty much orthogonal to what we're doing here. Good to know we've got a delsarte_bound_additive_hamming_space function in our global namespace, though :)

comment:11 Changed 8 months ago by darij

From sage/doctest/forker.py:

                # Make the right set of globals available to doctests
                if basename.startswith("sagenb."):
                    import sage.all_notebook as sage_all
                else:
                    import sage.all_cmdline as sage_all
                sage_namespace = RecordingDict(sage_all.__dict__)
                sage_namespace['__name__'] = '__main__'
                sage_namespace['__package__'] = None
                doctests, extras = self.source.create_doctests(sage_namespace)
                timer = Timer().start()

I'm wondering if we should just axe the line with the __package__ here?

comment:12 Changed 8 months ago by darij

OK, axing doesn't work, since this line is there to undo the (apparently Sphinx-specific) setting of __package__ to sage. Let me try del...

comment:13 Changed 8 months ago by darij

EDIT: ignore this post.

Last edited 8 months ago by darij (previous) (diff)

comment:14 Changed 8 months ago by git

  • Commit changed from 6e0f42f336ad394f05040e379252c18c5785967a to cebac21b1ba3372d561760260d24442a40e75ada

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

cebac21fixing the broken inject_shorthands methods, and modifying doctest framework so that this fix can be tested for

comment:15 Changed 8 months ago by darij

OK, forget about the second branch; I've just hijacked the main branch with what I think is a full solution. Warning: forced push!

Last edited 8 months ago by darij (previous) (diff)

comment:16 Changed 8 months ago by git

  • Commit changed from cebac21b1ba3372d561760260d24442a40e75ada to 0b8ed458f20f651b1087da7eae3d182c5801e540

Branch pushed to git repo; I updated commit sha1. New commits:

0b8ed45same change in toric varieties code

comment:17 Changed 8 months ago by tscrim

  • Authors set to Darij Grinberg
  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

LGTM.

comment:18 Changed 8 months ago by darij

Thank you, Travis!!

*blush* I guess I really should start reviewing...

comment:19 Changed 8 months ago by vbraun

  • Status changed from positive_review to needs_info
  • Can we not leave commented-out lines lying around?
  • IMHO the check should just be
    if G.get("__name__", None) == "__main__":
    
    and not look at __package__. The topmost '__main__' frame is our guy, duh. Also this won't trip over similar issues if some frame doesn't have __name__.
  • The new doctest is just like doctests that we already have, it doesn't add anything new.

comment:20 Changed 8 months ago by git

  • Commit changed from 0b8ed458f20f651b1087da7eae3d182c5801e540 to 94a1e8bbb2d67f5474d17aec16bbe83c2884167c

Branch pushed to git repo; I updated commit sha1. New commits:

94a1e8bfixes suggested by Volker

comment:21 Changed 8 months ago by git

  • Commit changed from 94a1e8bbb2d67f5474d17aec16bbe83c2884167c to 648da0d3ee5d408f210e3f36a66d6c4f5e1bffb6

Branch pushed to git repo; I updated commit sha1. New commits:

648da0doops

comment:22 Changed 8 months ago by darij

ad 1: True; that was a habit from pre-git times. Removed.

ad 2: I have changed it now but I'm not 100% sure (what happens if we are doctesting a package?). I don't have time to run all the doctests now...

ad 3: I can't follow this. It is import-less and it stands at the beginning of the file. At least the first of these properties is crucial!

Last edited 8 months ago by darij (previous) (diff)

comment:23 Changed 8 months ago by darij

  • Status changed from needs_info to needs_review

comment:24 Changed 8 months ago by vbraun

  • Branch changed from public/combinat/inject_shorthands to 648da0d3ee5d408f210e3f36a66d6c4f5e1bffb6
  • Resolution set to fixed
  • Status changed from needs_review to closed
Note: See TracTickets for help on using tickets.