Opened 9 years ago

Closed 5 years ago

#10057 closed defect (fixed)

Upgrade sagenb

Reported by: jason Owned by: jason, was
Priority: blocker Milestone: sage-6.5
Component: packages: standard Keywords: sd35.5
Cc: jsrn Merged in:
Authors: Karl-Dieter Crisman Reviewers: Jeroen Demeyer
Report Upstream: Fixed upstream, in a later stable release. Work issues:
Branch: 14d3291 (Commits) Commit: 14d329164ce9731014d3d6e5a2ae1bbcc9a9b3f8
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

Upgrade to the latest version of sagenb. Tarball for upstream/ at http://sage.math.washington.edu/home/kcrisman/sagenb-0.11.2.tar

One useful fix is the fixing of various import locations: https://github.com/sagemath/sagenb/pull/280 We deprecate for real all these fixed sagenb imports in #17460.

This will also fix #11275, #8738, #8427, #17490.

See also #17482 and #17515 for Sage issues related to this update.

Attachments (1)

trac_10057.patch (632 bytes) - added by mhansen 9 years ago.

Download all attachments as: .zip

Change History (51)

comment:1 Changed 9 years ago by jason

  • Cc jsrn added

Changed 9 years ago by mhansen

comment:2 Changed 9 years ago by mhansen

  • Authors set to Mike Hansen
  • Status changed from new to needs_review

comment:3 follow-up: Changed 8 years ago by zimmerma

  • Keywords sd35.5 added
  • Reviewers set to Paul Zimmermann
  • Status changed from needs_review to needs_info

Mike,

I started to review that ticket, but I'm puzzled since

sage: from sage.misc.misc import decorator_defaults

still works. Shouldn't it fail?

Also, shouldn't the patch include a doctest?

Paul

comment:4 in reply to: ↑ 3 Changed 8 years ago by jsrn

Replying to zimmerma:

Mike,

I started to review that ticket, but I'm puzzled since

sage: from sage.misc.misc import decorator_defaults

still works. Shouldn't it fail?

The reason it doesn't fail is because sage.misc.misc contains an import statement, importing decorator_defaults among other (line 2735). This was introduced in #9907 for backwards compatibility, in case users assumed that these decorators would still be in sage.misc.misc. I assumed this would be a temporary thing, just as for keyword and attribute changes. #9907 was merged 14 months ago, so this is longer than the deprecation lifetime, but on the other hand, no deprecation warning has been issued to users doing the wrong thing for these past 14 months. I'm not sure what is usually done under such circumstances. Also, removing the import statement from sage.misc.misc will result in the pickle jar breaking, and I'm not sure how to remedy that and what consequences it might have. Need expert opinion here.

Also, shouldn't the patch include a doctest?

Maybe that's just me, but isn't it a bit too much including a doctest just for a relocation of a function? It's still the same function.

comment:5 Changed 8 years ago by zimmerma

  • Status changed from needs_info to needs_review

ok I understand. But then I don't know what a reviewer is supposed to do for such a patch. Anyway I put it back to needs review.

Paul

comment:6 Changed 6 years ago by kcrisman

  • Status changed from needs_review to needs_info

comment:7 Changed 5 years ago by jdemeyer

  • Authors Mike Hansen deleted
  • Milestone set to sage-duplicate/invalid/wontfix
  • Resolution set to invalid
  • Reviewers changed from Paul Zimmermann to Mike Hansen, Paul Zimmermann
  • Status changed from needs_info to closed

I'm working on a sagenb pull request.

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

  • Report Upstream changed from N/A to Not yet reported upstream; Will do shortly.
  • Resolution invalid deleted
  • Status changed from closed to new

Hey, only the release manager gets to close a ticket! ;-)

More seriously, should we remove the 'deprecated' import or is the pickle jar issue mentioned above serious enough to not do that? Anyway, better to keep this as an "upstream tracking" ticket, as sagenb will still need to be upgraded for this (and whatever else - https://github.com/sagemath/sagenb/pull/267 could use review, for instance).

comment:9 in reply to: ↑ 8 Changed 5 years ago by jdemeyer

  • Component changed from notebook to packages: standard
  • Description modified (diff)
  • Milestone changed from sage-duplicate/invalid/wontfix to sage-6.5
  • Summary changed from Change import location for decorator_defaults in sagenb interact.py to Upgrade sagenb

OK, I'll rework the ticket.

comment:10 Changed 5 years ago by jdemeyer

  • Description modified (diff)
  • Report Upstream changed from Not yet reported upstream; Will do shortly. to Reported upstream. No feedback yet.

comment:11 Changed 5 years ago by jdemeyer

  • Description modified (diff)

comment:12 Changed 5 years ago by jdemeyer

  • Description modified (diff)
  • Report Upstream changed from Reported upstream. No feedback yet. to Reported upstream. Developers acknowledge bug.

comment:13 Changed 5 years ago by kcrisman

  • Report Upstream changed from Reported upstream. Developers acknowledge bug. to Fixed upstream, but not in a stable release.

Okay. Now if you can at least look at https://github.com/sagemath/sagenb/pull/282 for me (since you already have a working sagenb development environment) I would be grateful. Given its importance, I'd also appreciate a look at https://github.com/sagemath/sagenb/pull/267 but I know that one requires just slightly more testing, though in reality both patches are fairly trivial.

comment:14 Changed 5 years ago by jdemeyer

  • Branch set to u/jdemeyer/ticket/10057
  • Modified changed from 12/08/14 16:57:59 to 12/08/14 16:57:59

comment:15 Changed 5 years ago by jdemeyer

  • Branch u/jdemeyer/ticket/10057 deleted

comment:16 Changed 5 years ago by kcrisman

  • Description modified (diff)
  • Reviewers Mike Hansen, Paul Zimmermann deleted

comment:17 Changed 5 years ago by kcrisman

  • Description modified (diff)

comment:18 follow-up: Changed 5 years ago by kcrisman

  • Authors set to Karl-Dieter Crisman
  • Branch set to u/kcrisman/ticket/10057
  • Commit set to f5c559df46d9a8700a9479bc1244b1d943e02b4b
  • Status changed from new to needs_review

For reference to #17490, I used gnutar and not the standard Mac tar, so hopefully that is resolved temporarily with this sagenb.


New commits:

f5c559dUpgrade sagenb to version 0.11.2

comment:19 Changed 5 years ago by kcrisman

  • Description modified (diff)

comment:20 Changed 5 years ago by kcrisman

  • Report Upstream changed from Fixed upstream, but not in a stable release. to Fixed upstream, in a later stable release.

comment:21 Changed 5 years ago by jdemeyer

  • Description modified (diff)

comment:22 in reply to: ↑ 18 Changed 5 years ago by jdemeyer

Replying to kcrisman:

For reference to #17490, I used gnutar and not the standard Mac tar, so hopefully that is resolved temporarily with this sagenb.

At least I see

jdemeyer@tamiyo:/usr/local/src/sage-git/upstream$ file sagenb-*
sagenb-0.10.8.2.tar: POSIX tar archive (GNU)
sagenb-0.11.0.tar:   POSIX tar archive
sagenb-0.11.1.tar:   POSIX tar archive
sagenb-0.11.2.tar:   POSIX tar archive (GNU)

comment:23 Changed 5 years ago by kcrisman

I don't know that this will fix #17490, though I can fix the upstream release instructions. But that is a good sign.

Anyway, ready for review. Most stuff is either obvious fixes or stuff that doesn't affect day-to-day notebook operations (the translation stuff).

comment:24 Changed 5 years ago by jdemeyer

I guess a review will be mostly "does it build and pass doctests?", assuming that everything was actually reviewed upstream.

comment:25 Changed 5 years ago by kcrisman

I think it is worth checking whether there is any serious regression of behavior - make sure worksheets still work, test random functionality, etc. as a whole. Especially worth making sure the Sage tickets mentioned above are actually fixed by the upgrade. The only possible issue I can foresee is if one of the packages upstream of upstream changed a doctest - I had some bizarre Sage failure related to Babel but I don't see how it could have come from a 'standard' sagenb installation.

Which reminds me that #17482 is just waiting for your say-so, I made the change you requested.

comment:26 Changed 5 years ago by kcrisman

Can someone check whether this could cause

sage -t --long src/sage/symbolic/expression.pyx
Failed example:
    import sagenb.misc.support as s
Expected nothing
Got:
    doctest:14: ImportWarning: Not importing directory '/Users/...sage/local/lib/python2.7/site-packages/Babel-1.3-py2.7.egg/babel/localedata': missing __init__.py

or whether that is just because of my sagenb setup, not a problem per se? There is a similar failure in

sage -t --long src/sage/combinat/cluster_algebra_quiver/quiver.py
sage -t --long src/sage/misc/dev_tools.py

Also the usual

File "src/sage/repl/notebook_ipython.py", line 60, in sage.repl.notebook_ipython.have_prerequisites
Failed example:
    from sage.repl.notebook_ipython import have_prerequisites
Expected nothing
Got:
    doctest:57: PowmInsecureWarning: Not using mpz_powm_sec.  You should rebuild using libgmp >= 5 to avoid timing attack vulnerability.

also in

sage -t --long src/sage/repl/zmq_kernel.py

though I don't (yet) have beta4.

comment:27 Changed 5 years ago by jdemeyer

  • Status changed from needs_review to needs_work

Indeed, this is a regression:

sage -t --long src/sage/symbolic/expression.pyx
**********************************************************************
File "src/sage/symbolic/expression.pyx", line 10721, in sage.symbolic.expression.get_dynamic_class_for_function
Failed example:
    import sagenb.misc.support as s
Expected nothing
Got:
    doctest:14: ImportWarning: Not importing directory '/usr/local/src/sage-git/local/lib/python2.7/site-packages/Babel-1.3-py2.7.egg/babel/localedata': missing __init__.py
**********************************************************************

comment:29 follow-up: Changed 5 years ago by kcrisman

I have to admit I cannot figure out what change would have done this; we already have Babel 1.3 in previous Sage. Maybe an unintended side effect of the changes in imports w.r.t. sagenb.misc.support? Also notice this only happens in doctests, not at the command line.

comment:30 in reply to: ↑ 29 Changed 5 years ago by jdemeyer

Replying to kcrisman:

we already have Babel 1.3 in previous Sage

Yes, but the previous Notebook version didn't actually use babel.

Also notice this only happens in doctests, not at the command line.

Warnings in Python can be enabled/disabled. Apparently, this warning is enabled in doctests (which is probably a good thing) but not in the command line.

comment:31 follow-up: Changed 5 years ago by kcrisman

Okay, I think it is stuff like https://github.com/sagemath/sagenb/commit/5ffa976e0d76b674e09cd6bddd2d1bd6105f166c - it actually happens in other places we import babel (or flask.ext.babel) but those were not doctested in Sage.

Unfortunately, the util/fetch_deps.py in sagenb doesn't exactly make it easy to just apply patches.

comment:32 Changed 5 years ago by kcrisman

Yes, but the previous Notebook version didn't actually use babel.

Sure it did - e.g. https://github.com/sagemath/sagenb/commit/5511a2a01322b66fefdc942e41c4d3d95b1f1677

comment:33 in reply to: ↑ 31 Changed 5 years ago by kcrisman

Unfortunately, the util/fetch_deps.py in sagenb doesn't exactly make it easy to just apply patches.

Worst case we could put this file in the egg at the very end, but that is pretty hackish.

comment:34 Changed 5 years ago by kcrisman

By the way, if you or others could test the rest of the upgrade, or at any rate normal functioning of nb, then we could have just this issue to deal with.

comment:35 Changed 5 years ago by jdemeyer

Yes, I was sloppy. To be more precise: running the doctests with sagenb-0.11.1 didn't import the babel package.

To patch babel: it's probably best to remove babel from sagenb and install it like a normal package (this should really be done for all packages inside sagenb).

comment:36 Changed 5 years ago by jdemeyer

At least, the babel failure is the only doctest failure.

comment:37 Changed 5 years ago by kcrisman

To patch babel: it's probably best to remove babel from sagenb and install it like a normal package (this should really be done for all packages inside sagenb).

Yes, but easier said than done. I won't have time for this today, and I'm a little hazy on exactly how all the dependencies for sagenb might get properly imported from Sage itself.

It would also be nice if upstream were responsive at all to their many pull requests (this seems to be the case for several projects pocoo has "taken over").

comment:38 Changed 5 years ago by jdemeyer

Alternatively, if you just want to fix the doctest failure, you could import the babel stuff inside the method where you need it.

comment:39 follow-ups: Changed 5 years ago by kcrisman

Hmm, that might be easier.

comment:40 in reply to: ↑ 39 Changed 5 years ago by kcrisman

Hmm, that might be easier.

Not that I wouldn't be averse to shipping the dependencies separately again! I just feel like any time I have for sagenb is better used for actual bugs, of which there are already plenty.

comment:41 in reply to: ↑ 39 Changed 5 years ago by kcrisman

Hmm, that might be easier.

Yes, that fixes it - and since there is only that one function, it's not a problem to import it in that one place.

I'll have a fix and new package in a little bit. I hope.

Last edited 5 years ago by kcrisman (previous) (diff)

comment:42 Changed 5 years ago by git

  • Commit changed from f5c559df46d9a8700a9479bc1244b1d943e02b4b to 14d329164ce9731014d3d6e5a2ae1bbcc9a9b3f8

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

14d3291Upgrade sagenb to version 0.11.2

comment:43 Changed 5 years ago by kcrisman

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

Okay, it was a forced push but should be okay now. Same address for the updated spkg. Here is the diff - once this has positive review I'll push it all back upstream.

  • sagenb/misc/misc.py

    diff --git a/sagenb/misc/misc.py b/sagenb/misc/misc.py
    index 88b3eef..5a647f7 100644
    a b Check that github issue #195 is fixed:: 
    2020#############################################################################
    2121
    2222from pkg_resources import resource_filename
    23 from babel import Locale
    24 from babel.core import UnknownLocaleError
    2523
    2624def stub(f):
    2725    def g(*args, **kwds):
    def translations_path(): 
    517515    return os.path.join(SAGENB_ROOT, 'translations')
    518516
    519517def get_languages():
     518    from babel import Locale
     519    from babel.core import UnknownLocaleError
    520520    langs = []
    521521    dir_names = [lang for lang in os.listdir(translations_path())
    522522                       if lang != 'en_US']
Last edited 5 years ago by kcrisman (previous) (diff)

comment:44 Changed 5 years ago by jdemeyer

All long doctests pass this time.

comment:45 Changed 5 years ago by kcrisman

And I assume that it is still a gnu tar archive?

Okay, I think anyone can put this to positive review if they don't see any regressions in actual behavior.

comment:46 Changed 5 years ago by kcrisman

I'm tagging this upstream, so any additional problems will have to be 1) after Christmas and 2) version 0.11.3.

comment:47 Changed 5 years ago by jdemeyer

The tarball is made correctly, I don't get any warnings when extracting it.

I also checked that the tarball contents matches the git repo. I noticed a few missing files: https://github.com/sagemath/sagenb/issues/327 I consider this a serious issue (especially with the COPYING file missing), but not serious enough to block this ticket.

comment:48 Changed 5 years ago by jdemeyer

  • Status changed from needs_review to positive_review

comment:49 Changed 5 years ago by jdemeyer

  • Priority changed from major to blocker

comment:50 Changed 5 years ago by vbraun

  • Branch changed from u/kcrisman/ticket/10057 to 14d329164ce9731014d3d6e5a2ae1bbcc9a9b3f8
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.