Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#16911 closed enhancement (fixed)

Update sagenb

Reported by: ppurka Owned by:
Priority: major Milestone: sage-6.4
Component: notebook Keywords:
Cc: kcrisman, Snark, fbissey Merged in:
Authors: Punarbasu Purkayastha Reviewers: Julien Puydt, Karl-Dieter Crisman
Report Upstream: N/A Work issues:
Branch: b84d502 (Commits, GitHub, GitLab) Commit:
Dependencies: #16396 Stopgaps:

Status badges

Description (last modified by kcrisman)

This ticket is for

  1. updating sagenb. Corresponding upstream ticket is https://github.com/sagemath/sagenb/pull/218
  2. updating documentation inside sage to reflect the new development model for sagenb
cd SAGE_ROOT/upstream
wget -c 'https://googledrive.com/host/0B2kYdLdBVFHIZEd4ekpjNUFBNTQ/sagenb-0.10.8.3.tar'
git checkout -b develop ticket/16911
git trac pull 16911
../sage -i sagenb
../sage -b

To @vbraun: When merging this ticket, merge both this and #16396 since sagenb upstream has changes to fix doctests in that ticket, and that ticket in turn depends on an updated sagenb so that it can pass all doctests. Also close #11469 as that is incorporated here.

Change History (47)

comment:1 Changed 7 years ago by ppurka

  • Branch set to u/ppurka/update_sagenb

comment:2 Changed 7 years ago by kcrisman

  • Cc kcrisman added
  • Commit set to 2d3d3e993bb1bef1f25fcf36dcd45cbc37d3c132

New commits:

2d3d3e9update sagenb developer guide with new dev instructions

comment:3 Changed 7 years ago by Snark

  • Cc Snark added

comment:4 Changed 7 years ago by git

  • Commit changed from 2d3d3e993bb1bef1f25fcf36dcd45cbc37d3c132 to fe9630cb38206b159300438adb3fa552090f8acf

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

fe9630cupdate sagenb version and checksum

comment:5 Changed 7 years ago by ppurka

  • Description modified (diff)

Updated to new version with the upstream sagenb changes made to sphinx. The method to check it out is in description.

Note: this is not the final version of the sagenb package, since I haven't changed the Changelog, and it doesn't contain the changes from upstream ticket https://github.com/sagemath/sagenb/pull/218

Still, I am setting it to needs review so that the package itself can be tested against #16396

Last edited 7 years ago by ppurka (previous) (diff)

comment:6 Changed 7 years ago by ppurka

  • Status changed from new to needs_review

comment:7 Changed 7 years ago by Snark

  • Status changed from needs_review to needs_work

Hmmmm... of course it fails to build since it's looking for upstream sources which we don't have... how shall I test this ticket and #16396 without it?

comment:8 Changed 7 years ago by ppurka

Installation instructions are in the ticket description. If you don't have wget then you can download the file manually from Google drive.

comment:9 Changed 7 years ago by fbissey

  • Cc fbissey added

comment:10 Changed 7 years ago by Snark

  • Status changed from needs_work to positive_review

Sigh... and I had actually checked if it wasn't in the description!

It compiles and passes all doctests. I haven't yet looked at what it gives with #16396, but for this ticket, it's a positive review.

comment:11 Changed 7 years ago by ppurka

  • Status changed from positive_review to needs_work

Thanks. It is not complete yet. I wanted to put it up here so that you can test your ticket.

Todo:

  1. merge upstream ticket about documentation, and
  2. update change log.

comment:12 Changed 7 years ago by Snark

I have checked that the ticket as-is passes all doctests, and makes #16396 also pass everything.

comment:13 Changed 7 years ago by kcrisman

Thanks for checking! Have you played with the notebook itself to make sure there aren't any weird regressions (like the notebook not working...) that we missed?

comment:14 Changed 7 years ago by Snark

Hmmmm... I hadn't used the notebook since long, but I just failed to get a plot! Is there something wrong with:

from pylab import *
xs=linspace(0,6)
ys=sin(xs)
plot(xs,ys)
show()

?

comment:15 follow-up: Changed 7 years ago by kcrisman

I'm not sure that ever worked. A figure has to be saved for it to show up, and I don't know that matplotlib does that.

savefig('hi.png')

should work, however - try that.

comment:16 in reply to: ↑ 15 Changed 7 years ago by kcrisman

I'm not sure that ever worked. A figure has to be saved for it to show up, and I don't know that matplotlib does that.

savefig('hi.png')

should work, however - try that.

To be fair, in the Ipython notebook I assume that your code does work :)

comment:17 Changed 7 years ago by Snark

Ah, yes, that's better... so it's a positive review?

comment:18 follow-up: Changed 7 years ago by Snark

Though notice that I have the following in the logs:

/home/jpuydt/sage.git/local/lib/python2.7/site-packages/Crypto/Util/number.py:57: PowmInsecureWarning: Not using mpz_powm_sec.  You should rebuild using libgmp >= 5 to avoid timing attack vulnerability.
  _warn("Not using mpz_powm_sec.  You should rebuild using libgmp >= 5 to avoid timing attack vulnerability.", PowmInsecureWarning)

comment:19 Changed 7 years ago by kcrisman

That is known and unrelated. You can try it in a Sage without this and probably you'll see the same thing. I do in all my installs.

But as ppurka says here, there remains a little cosmetic work.

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

comment:20 Changed 7 years ago by ppurka

Thanks all. I will update the package by this weekend.

comment:21 in reply to: ↑ 18 Changed 7 years ago by fbissey

Replying to Snark:

Though notice that I have the following in the logs:

/home/jpuydt/sage.git/local/lib/python2.7/site-packages/Crypto/Util/number.py:57: PowmInsecureWarning: Not using mpz_powm_sec.  You should rebuild using libgmp >= 5 to avoid timing attack vulnerability.
  _warn("Not using mpz_powm_sec.  You should rebuild using libgmp >= 5 to avoid timing attack vulnerability.", PowmInsecureWarning)

Independent from this ticket. We have already seen that if I remember correctly and it won't go away for some time while we are using mpir to provide gmp.

comment:22 Changed 7 years ago by kcrisman

I'm happy with the instructions on the Sage side as well, or at least happier :)

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

comment:23 Changed 7 years ago by kcrisman

By the way, ppurka, there are a few other ready or almost-ready things that are actually fairly important upstream by migurehito, so if they come quick we should probably merge them too (things about admin errors).

comment:24 Changed 7 years ago by kcrisman

This should fix #11469.

comment:25 Changed 7 years ago by git

  • Commit changed from fe9630cb38206b159300438adb3fa552090f8acf to b84d5029fc38e5882d6e589481dbe17e4452860e

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

b84d502Update checksums for sagenb 0.10.8.3

comment:26 Changed 7 years ago by ppurka

  • Authors set to Punarbasu Purkayastha
  • Reviewers set to Julien Puydt, Karl-Dieter Crisman
  • Status changed from needs_work to positive_review

Ok. I have updated sagenb upstream, the sagenb.tar, and added the new checksum. This is good to go. Setting it to positive review.

comment:27 Changed 7 years ago by vbraun

  • Dependencies set to #16396

comment:28 Changed 7 years ago by Snark

Uh... Volker, why did you put #16396 as a dep of this ticket? That's the reverse as far as I know: sagenb's upgrade (#16911, here) blocks sphinx' upgrade (#16396), which blocks docutils' upgrade (#16733)... Or is there something I miss?

comment:29 Changed 7 years ago by vbraun

This ticket can be merged as soon as #16396 is done, so from an operational point of view we depend on #16396 and not the other way round.

comment:30 Changed 7 years ago by Snark

Uh... actually that's the reverse: the sphinx upgrade over at #16396 gives failing doctests until sagenb (#16911, here) has been upgraded.

Of course, if you merge both at the same time, that won't have any importance :-P

comment:31 Changed 7 years ago by vbraun

#16396 needs work, at least it says so on the ticket. So again, for ticket merging purposes, this ticket has to wait for #16396.

comment:32 Changed 7 years ago by kcrisman

Just FYI, François has updated #16396 and Julien seems to have confirmed it works, just need for that one to have the one last check and then we're good to go here.

comment:33 Changed 7 years ago by kcrisman

  • Description modified (diff)

comment:34 Changed 7 years ago by vbraun

  • Branch changed from u/ppurka/update_sagenb to b84d5029fc38e5882d6e589481dbe17e4452860e
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:35 Changed 7 years ago by gutow

  • Commit b84d5029fc38e5882d6e589481dbe17e4452860e deleted

How can this ticket be fixed without the following?

To @vbraun: When merging this ticket, merge both this and #16396 since sagenb upstream has changes to fix doctests in that ticket, and that ticket in turn depends on an updated sagenb so that it can pass all doctests. Also close #11469 as that is incorporated here.

comment:36 follow-up: Changed 7 years ago by vbraun

I merged #16396. Isn't that sufficient? Please please put dependencies in the dependency field, I can't read through every ticket description / discussion when merging.

comment:37 in reply to: ↑ 36 Changed 7 years ago by kcrisman

I merged #16396. Isn't that sufficient?

I think at the time it hadn't been merged, though I'm sure it was in your queue at that point.

Please please put dependencies in the dependency field, I can't read through every ticket description / discussion when merging.

Yes, and the sagenb tickets aren't the only ones where this happens. Perhaps a gentle reminder on sage-devel would be helpful, I'll do that.

comment:38 follow-ups: Changed 7 years ago by jhpalmieri

Was sagenb 0.11.0 merged as part of this ticket? For what it's worth, the tarball was not prepared correctly: it has two copies of most of the component pieces (like Babel-1.3.tar.gz). When you unpack it, it has at the top level another tarball called sagenb-0.11.0.tar.gz which contains a second copy of everything. This is why the "upstream" tarball is twice the size of the one for sagenb-0.10.8.2.

comment:39 in reply to: ↑ 38 Changed 7 years ago by kcrisman

Was sagenb 0.11.0 merged as part of this ticket?

It wasn't supposed to be. That would have been #16004.

For what it's worth, the tarball was not prepared correctly: it has two copies of most of the component pieces (like Babel-1.3.tar.gz). When you unpack it, it has at the top level another tarball called sagenb-0.11.0.tar.gz which contains a second copy of everything. This is why the "upstream" tarball is twice the size of the one for sagenb-0.10.8.2.

I just followed the instructions at https://gist.github.com/ppurka/d10f668db113c2d1ddd4 as recommended. I wouldn't be surprised if I did something wrong, though.

I'd be happy to try to fix that but would need very explicit instructions; I don't want to mess around trying to guess the right thing to do.

comment:40 follow-up: Changed 7 years ago by jhpalmieri

I think that it can probably wait until the next sagenb update. By the way, I just noticed that since sagenb-0.10.8.2, I see the following message in the installation log file:

Note: Bypassing http://github.com/mitsuhiko/flask-oldsessions/tarball/master#egg=flask-oldsessions-0.10 (disallowed host; see http://bit.ly/1dg9ijs for details).

What does this mean?

comment:41 Changed 7 years ago by kcrisman

I also noticed that. I believe it means we are supposed to download (see e.g. https://github.com/sagemath/sagenb/blob/master/util/fetch_deps.py) that but didn't because somewhere we don't allow this host. I don't know exactly where we use easy_install, though, so I couldn't easily debug that.

comment:42 in reply to: ↑ 40 Changed 7 years ago by kcrisman

I think that it can probably wait until the next sagenb update.

That may come sooner than you think; see #17292.

comment:43 in reply to: ↑ 38 Changed 7 years ago by kcrisman

Was sagenb 0.11.0 merged as part of this ticket? For what it's worth, the tarball was not prepared correctly: it has two copies of most of the component pieces (like Babel-1.3.tar.gz). When you unpack it, it has at the top level another tarball called sagenb-0.11.0.tar.gz which contains a second copy of everything. This is why the "upstream" tarball is twice the size of the one for sagenb-0.10.8.2.

Looks like we will indeed be releasing a new one, so if you have an easy fix for this, I would appreciate that - you can communicate at https://github.com/sagemath/sagenb/issues or here or at #17292 or in private communication.

comment:44 follow-up: Changed 7 years ago by jhpalmieri

I don't have an easy fix. It turns out that the big tarball contained a smaller tarball sagenb-0.11.0.tar.gz which actually was just the right thing: you could just keep the smaller one and use it in the upstream directory. I guess I would say, keep an eye on the file sizes. If it's much bigger than the previous one (for 0.10.8.2 or whatever it was), then something might be wrong.

comment:45 Changed 7 years ago by kcrisman

Hmm, then I guess the problem is that the instructions have an extra line. Because python setup.py sdist already makes a .tar.gz, and then we in addition do

tar cf sagenb-<version>.tar sagenb-<version>

which presumably leads to the tar file you found. So that step is probably extra.

comment:46 in reply to: ↑ 44 Changed 7 years ago by kcrisman

I don't have an easy fix. It turns out that the big tarball contained a smaller tarball sagenb-0.11.0.tar.gz which actually was just the right thing: you could just keep the smaller one and use it in the upstream directory. I guess I would say, keep an eye on the file sizes. If it's much bigger than the previous one (for 0.10.8.2 or whatever it was), then something might be wrong.

I dunno, I'm not convinced now. I just repackaged a new one (it's not 100% ready, but just to test) and got

$ du sagenb-0.11.1.tar 
35824	sagenb-0.11.1.tar
$ du upstream/sagenb-0.10.8.2.tar 
33224	upstream/sagenb-0.10.8.2.tar

Given that there is a new jQuery and other goodies, this seems like a small difference. I can confirm that the previous one was wrong, but I don't know how this happened. I'll check in the future, though.

comment:47 Changed 7 years ago by jhpalmieri

I think that's probably okay. It's not twice the size, which is what happened before.

Note: See TracTickets for help on using tickets.