Opened 12 years ago

Closed 8 years ago

#10057 closed defect (fixed)

Upgrade sagenb

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

Status badges

Description (last modified by Jeroen Demeyer)

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 Mike Hansen 12 years ago.

Download all attachments as: .zip

Change History (51)

comment:1 Changed 12 years ago by Jason Grout

Cc: Johan Rosenkilde added

Changed 12 years ago by Mike Hansen

Attachment: trac_10057.patch added

comment:2 Changed 12 years ago by Mike Hansen

Authors: Mike Hansen
Status: newneeds_review

comment:3 Changed 11 years ago by Paul Zimmermann

Keywords: sd35.5 added
Reviewers: Paul Zimmermann
Status: needs_reviewneeds_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 11 years ago by Johan Rosenkilde

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 11 years ago by Paul Zimmermann

Status: needs_infoneeds_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 9 years ago by Karl-Dieter Crisman

Status: needs_reviewneeds_info

comment:7 Changed 8 years ago by Jeroen Demeyer

Authors: Mike Hansen
Milestone: sage-duplicate/invalid/wontfix
Resolution: invalid
Reviewers: Paul ZimmermannMike Hansen, Paul Zimmermann
Status: needs_infoclosed

I'm working on a sagenb pull request.

comment:8 Changed 8 years ago by Karl-Dieter Crisman

Report Upstream: N/ANot yet reported upstream; Will do shortly.
Resolution: invalid
Status: closednew

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 8 years ago by Jeroen Demeyer

Component: notebookpackages: standard
Description: modified (diff)
Milestone: sage-duplicate/invalid/wontfixsage-6.5
Summary: Change import location for decorator_defaults in sagenb interact.pyUpgrade sagenb

OK, I'll rework the ticket.

comment:10 Changed 8 years ago by Jeroen Demeyer

Description: modified (diff)
Report Upstream: Not yet reported upstream; Will do shortly.Reported upstream. No feedback yet.

comment:11 Changed 8 years ago by Jeroen Demeyer

Description: modified (diff)

comment:12 Changed 8 years ago by Jeroen Demeyer

Description: modified (diff)
Report Upstream: Reported upstream. No feedback yet.Reported upstream. Developers acknowledge bug.

comment:13 Changed 8 years ago by Karl-Dieter Crisman

Report Upstream: Reported upstream. Developers acknowledge bug.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 8 years ago by Jeroen Demeyer

Branch: u/jdemeyer/ticket/10057
Modified: Dec 8, 2014, 4:57:59 PMDec 8, 2014, 4:57:59 PM

comment:15 Changed 8 years ago by Jeroen Demeyer

Branch: u/jdemeyer/ticket/10057

comment:16 Changed 8 years ago by Karl-Dieter Crisman

Description: modified (diff)
Reviewers: Mike Hansen, Paul Zimmermann

comment:17 Changed 8 years ago by Karl-Dieter Crisman

Description: modified (diff)

comment:18 Changed 8 years ago by Karl-Dieter Crisman

Authors: Karl-Dieter Crisman
Branch: u/kcrisman/ticket/10057
Commit: f5c559df46d9a8700a9479bc1244b1d943e02b4b
Status: newneeds_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 8 years ago by Karl-Dieter Crisman

Description: modified (diff)

comment:20 Changed 8 years ago by Karl-Dieter Crisman

Report Upstream: Fixed upstream, but not in a stable release.Fixed upstream, in a later stable release.

comment:21 Changed 8 years ago by Jeroen Demeyer

Description: modified (diff)

comment:22 in reply to:  18 Changed 8 years ago by Jeroen Demeyer

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 8 years ago by Karl-Dieter Crisman

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 8 years ago by Jeroen Demeyer

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

comment:25 Changed 8 years ago by Karl-Dieter Crisman

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 8 years ago by Karl-Dieter Crisman

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 8 years ago by Jeroen Demeyer

Status: needs_reviewneeds_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 Changed 8 years ago by Karl-Dieter Crisman

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 8 years ago by Jeroen Demeyer

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 Changed 8 years ago by Karl-Dieter Crisman

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 8 years ago by Karl-Dieter Crisman

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 8 years ago by Karl-Dieter Crisman

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 8 years ago by Karl-Dieter Crisman

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 8 years ago by Jeroen Demeyer

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 8 years ago by Jeroen Demeyer

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

comment:37 Changed 8 years ago by Karl-Dieter Crisman

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 8 years ago by Jeroen Demeyer

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 Changed 8 years ago by Karl-Dieter Crisman

Hmm, that might be easier.

comment:40 in reply to:  39 Changed 8 years ago by Karl-Dieter Crisman

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 8 years ago by Karl-Dieter Crisman

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 8 years ago by Karl-Dieter Crisman (previous) (diff)

comment:42 Changed 8 years ago by git

Commit: f5c559df46d9a8700a9479bc1244b1d943e02b4b14d329164ce9731014d3d6e5a2ae1bbcc9a9b3f8

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 8 years ago by Karl-Dieter Crisman

Reviewers: Jeroen Demeyer
Status: needs_workneeds_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 8 years ago by Karl-Dieter Crisman (previous) (diff)

comment:44 Changed 8 years ago by Jeroen Demeyer

All long doctests pass this time.

comment:45 Changed 8 years ago by Karl-Dieter Crisman

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 8 years ago by Karl-Dieter Crisman

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 8 years ago by Jeroen Demeyer

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 8 years ago by Jeroen Demeyer

Status: needs_reviewpositive_review

comment:49 Changed 8 years ago by Jeroen Demeyer

Priority: majorblocker

comment:50 Changed 8 years ago by Volker Braun

Branch: u/kcrisman/ticket/1005714d329164ce9731014d3d6e5a2ae1bbcc9a9b3f8
Resolution: fixed
Status: positive_reviewclosed
Note: See TracTickets for help on using tickets.