Opened 9 years ago

Closed 8 years ago

Last modified 8 years ago

#10652 closed enhancement (fixed)

Add support for uploading static html doc page as a worksheet in the notebook

Reported by: nthiery Owned by: nthiery
Priority: critical Milestone: sage-4.7
Component: notebook Keywords: days28, Sphinx, upload, static html documentation
Cc: jason, hivert, slabbe Merged in: sage-4.7.alpha4
Authors: Nicolas M. Thiéry Reviewers: Jason Grout
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by jdemeyer)

This patch adds (experimental) support for uploading a static html doc page as a worksheet in the notebook. As a side effect, it:

  • allows for uploading a .txt file from a URL (was broken)
  • allows for uploading a zip file containing .txt and .html files

This little feature would give a very simple workflow for distributing tutorial worksheets during our Sage(-Combinat) workshops, saving a lot of hassle and time to both us and the participants (more detail upon request). It would be great if this feature could be in the sage stable release by early May for the next Sage-Combinat workshop in Acadia.

I wrote the attached small patch as a proof of concept; however this is the first time I am touching the notebook code and I am not planning to invest in this direction in the short term. So I would be extremely glad if some notebook expert could take over this patch, clean it up (look for TODO/FIXME), and rebase it upon #10521 with whom it probably conflicts lightly (this should be a breeze).

Little issues:

  • The produced worksheet does not look as nice as a usual worksheet or the live documentation (in particular, there are some useless navigation links on top). That's probably a question of style file. Or maybe the navigation links should be stripped out.

Future extensions:

  • Add support for attachements, non text outputs, etc (they are currently lost during the upload)

Related tickets:

  • #4825: Uploading a pdf which contains an attached .sws will extract the .sws
  • #7441: notebook: make it possible to upload from the url of a published worksheet
  • #9875: Can't upload from a notebook link, only from a .sws file
  • #10521: notebook upload of zipped worksheets can't deal with multiple worksheets with the same name

Apply in the sagenb repository:

  1. trac_10652-upload-static-documentation-as-worksheet-nt.patch
  2. trac-10652-reviewer.patch
  3. trac_10652-upload-static-documentation-as-worksheet-advertise-nt.patch

Nicolas

Attachments (3)

trac_10652-upload-static-documentation-as-worksheet-nt.patch (6.4 KB) - added by nthiery 8 years ago.
trac-10652-reviewer.patch (3.6 KB) - added by jason 8 years ago.
apply on top of previous patches
trac_10652-upload-static-documentation-as-worksheet-advertise-nt.patch (816 bytes) - added by nthiery 8 years ago.

Download all attachments as: .zip

Change History (40)

comment:1 Changed 9 years ago by nthiery

  • Description modified (diff)
  • Status changed from new to needs_review

comment:2 Changed 9 years ago by jason

  • Cc jason added

comment:3 Changed 9 years ago by jason

  • Description modified (diff)

Added #4825 to the list of related tickets. That ticket proposes that uploading pdf files with attached .sws files would extract the .sws file and put it in the users list of worksheets. I could see this also being useful for tutorials, books, articles, etc.

comment:4 follow-up: Changed 9 years ago by jason

This little feature would give a very simple workflow for distributing tutorial worksheets during our Sage(-Combinat) workshops, saving a lot of hassle and time to both us and the participants (more detail upon request).

I'm curious about how you plan on using this. I'd love an easy way to distribute lots of tutorials.

comment:5 Changed 9 years ago by hivert

  • Cc hivert added

+1 !!! We badly need it !!!

Florent

comment:6 in reply to: ↑ 4 Changed 9 years ago by nthiery

  • Owner changed from jason, mpatel, was to nthiery

Hi Jason!

I'm curious about how you plan on using this. I'd love an easy way to distribute lots of tutorials.

So much for being lazy :-) Here it is!

We write our tutorials as rst files in the reference manual (exact location under discussion), and share them using our Sage-Combinat patch server ([1]). On any server, we compile the html documentation, and make it available to the participants ([2]).

Finaly, we point the participants to [3]. They can then click on any of the static tutorials; if they like it, they can just copy paste the url [4] from their brower into the "Upload" field of their notebook.

Note: in case the server runs a notebook server, or if one has the sage-combinat patches installed, then one can directly use the live documentation.

Cheers,

Nicolas

[1] http://combinat.sagemath.org/hgwebdir.cgi/patches/file/tip/sage-demos-and-tutorials-nt.patch [2] http://combinat.sagemath.org/doc/reference/demos/ [3] http://combinat.sagemath.org/doc/reference/demos/2011-01-17-SageDays28.html [4] http://combinat.sagemath.org/doc/reference/demos/demo-algebraic-combinatorics-short.html

comment:7 Changed 9 years ago by nthiery

  • Description modified (diff)

comment:8 Changed 9 years ago by slelievre

  • Keywords days28 added

comment:9 Changed 9 years ago by slabbe

  • Cc slabbe added

comment:10 follow-up: Changed 9 years ago by ddrake

This is great work. It should definitely be possible to upload anything sensible to the notebook and have it give you a worksheet.

Given the upcoming changes in the notebook, perhaps this ticket should be viewed as a feature request for the new Flask-based notebook.

comment:11 in reply to: ↑ 10 Changed 9 years ago by nthiery

Hi Dan!

Replying to ddrake:

This is great work.

:-)

Given the upcoming changes in the notebook, perhaps this ticket should be viewed as a feature request for the new Flask-based notebook.

This sounds natural indeed!

However, since the feature is needed very soon, and the change minor, I still would appreciate if this patch (or some improved version) was merged in soon in the current notebook.

comment:12 Changed 9 years ago by nthiery

  • Priority changed from major to critical

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

Hi nthiery,

This would be a nice change. See also sws2tex and tex2sws for related projects.

I am sorry that I can't review this and test it properly right now, though the code seems ok in the main. What does the "assert False" do - I know what assert usually does, but I assume this is some special use of that statement?

Also, it would perhaps be better to us os.path or something like that

sage: a = "http://www.me.com/mysheet.sws"
sage: os.path.splitext(a)
('http://www.me.com/mysheet', '.sws')

in twist.py and then just check the result[1] for the extensions that are allowed all at once, keeping that for glueing on later, also with os.path?

By the way, you could perhaps just use this on your combinat server - sometimes people put custom patches on servers, it's not hard to do if you're the admin. That would keep it from being quite as critical to the rest of Sage.

comment:14 in reply to: ↑ 13 Changed 8 years ago by nthiery

Replying to kcrisman:

This would be a nice change. See also sws2tex and tex2sws for related projects.

I am sorry that I can't review this and test it properly right now, though the code seems ok in the main. What does the "assert False" do - I know what assert usually does, but I assume this is some special use of that statement?

Just a big fat advertisement that the execution flow is not supposed to get there :-)

A proper assertion should be raised instead, but as the comment states, I would need to have some feed back from the notebook people to know which assertion would be most appropriate here.

Ok, I guess I'll just raise an RunTimeError? for the moment.

Also, it would perhaps be better to us os.path or something like that

sage: a = "http://www.me.com/mysheet.sws"
sage: os.path.splitext(a)
('http://www.me.com/mysheet', '.sws')

in twist.py and then just check the result[1] for the extensions that are allowed all at once, keeping that for glueing on later, also with os.path?

Will do now.

By the way, you could perhaps just use this on your combinat server - sometimes people put custom patches on servers, it's not hard to do if you're the admin. That would keep it from being quite as critical to the rest of Sage.

The point is that we want to use this feature with the very first tutorials. In particuliar with complete newbies. If using Sage starts by having to apply a patch, we are going to scare them away :-) And many of them won't have the required development tools to install the Sage-Combinat queue, etc.

Thanks for your comments,

Cheers,

Nicolas

comment:15 Changed 8 years ago by kcrisman

I see, you mean you want them to be able to upload them to their own local Sage installations or servers. I understand.

I'd raise some sort of input error or value error myself, with message about the URL file ending not one that we support (perhaps with a list? though such lists can get long as we support more and more extensions...).

Again, sorry I can't do a full review. Can any of the other sage-combinat people do it? Of course, even if we made a new sagenb spkg (perhaps with jmol and a couple other things that are about ready) there is no guarantee that Jeroen will put it in 4.7, since he already has made four alphas.

comment:16 follow-up: Changed 8 years ago by jason

The code mainly looks okay to me, but here are a few points that worry me. I haven't tested this, so this is not a full review (yet).

  1. We of course don't want the notebook to crash if someone uploads a file other than the right type. The assert statement in checking this worries me. I'd say a ValueError? would be appropriate (we got something that was the wrong value).
  1. In the _import_worksheet_html function, there is a return statement, but then there are two more lines of code. What are those two lines doing after the return statement?

comment:17 in reply to: ↑ 16 Changed 8 years ago by kcrisman

Replying to jason:

The code mainly looks okay to me, but here are a few points that worry me. I haven't tested this, so this is not a full review (yet).

  1. We of course don't want the notebook to crash if someone uploads a file other than the right type. The assert statement in checking this worries me. I'd say a ValueError? would be appropriate (we got something that was the wrong value).

So that's what assert False would do? Okay, then I definitely would raise an error. Jason's suggestion sounds fine.

comment:18 Changed 8 years ago by nthiery

  • Description modified (diff)

comment:19 follow-ups: Changed 8 years ago by nthiery

Hi!

Thanks for your further comments.

In the updated patch, the assert false is replaced by an HTMLResponse; please check that the message is meaningful enough. The extension guessins is now done with urlparse + splitext to get the extension (though there are comments on the web that we should use the Content-Type answer of the web server; but that was not very practical here). The two lines after the return are removed; they were just accidental residue of some former experimentation. Finally, I wrote the missing doctest for _import_worksheet_html. I still have a problem there, since the doctest does not pass (there is a single garbled cell, instead of several), whereas running it by hand in the sage interpreter just works fine. Help welcome!

Cheers,

Nicolas

comment:20 in reply to: ↑ 19 Changed 8 years ago by jason

Replying to nthiery:

Hi!

Thanks for your further comments.

In the updated patch, the assert false is replaced by an HTMLResponse; please check that the message is meaningful enough. The extension guessins is now done with urlparse + splitext to get the extension (though there are comments on the web that we should use the Content-Type answer of the web server; but that was not very practical here). The two lines after the return are removed; they were just accidental residue of some former experimentation. Finally, I wrote the missing doctest for _import_worksheet_html. I still have a problem there, since the doctest does not pass (there is a single garbled cell, instead of several), whereas running it by hand in the sage interpreter just works fine. Help welcome!

This is what I got when I ran the doctest by hand:

sage: nb = sagenb.notebook.notebook.Notebook(tmp_dir()+'.sagenb')
sage: name = tmp_filename() + '.html'
sage: fd = open(name,'w')
sage: fd.write('\n'.join(
....: ['<html><head><title>Test Notebook</title></head>',
....: '<body>',
....: '<div class="highlight-python"><div class="highlight"><pre>',
....: '<span class="gp">sage: </span><span class="mi">1</span><span class="o">+</span><span class="mi">1</span>',
....: '<span class="go">2</span>',
....: '</pre></div></div>',
....: '</body></html>']))
sage: fd.close()
sage: W = nb._import_worksheet_html(name, 'admin')
sage: W.name()
u'Test Notebook'
sage: 
sage: W.owner()
'admin'
sage: W.cell_list()
[TextCell 1: Test Notebook
system:sage

<div class="highlight-python">, Cell 0; in=1+1, out=
2
, TextCell 2: </div]
sage: cell = W.cell_list()[1]
sage: cell.input_text()
u'1+1'
sage: cell.output_text()
u'<pre class="shrunk">2</pre>'

Cheers,

Nicolas

comment:21 in reply to: ↑ 19 Changed 8 years ago by jason

Replying to nthiery:

Hi!

Thanks for your further comments.

In the updated patch, the assert false is replaced by an HTMLResponse; please check that the message is meaningful enough. The extension guessins is now done with urlparse + splitext to get the extension (though there are comments on the web that we should use the Content-Type answer of the web server; but that was not very practical here). The two lines after the return are removed; they were just accidental residue of some former experimentation. Finally, I wrote the missing doctest for _import_worksheet_html. I still have a problem there, since the doctest does not pass (there is a single garbled cell, instead of several), whereas running it by hand in the sage interpreter just works fine. Help welcome!

The problem is that the doctesting system replaces the "sage: " *inside* your html file with ">>>", which of course messes everything up.

I have a patch coming up.

Changed 8 years ago by jason

apply on top of previous patches

comment:22 follow-up: Changed 8 years ago by jason

  • Reviewers set to Jaosn Grout

nthiery: positive review on your patch, modulo the changes in my patch. Can you review my patch?

Three substantial changes:

  1. I changed the doctest to use a bit more of the actual generated documentation file structure.
  1. I changed the linebreaks in the doctest so that the string "sage: " did not appear as consecutive characters in input (this works around the bug in the doctesting system which replaces "sage:" with ">>>")
  1. I deleted the line which prepended the title and system to the text of the worksheet. Those don't do anything but show up as the first part of the first text cell, which is silly. The default system is already 'sage', and the title is set when the worksheet is created.

Can you review my reviewer patch? If you think it's good, set this ticket to positive review.

comment:23 Changed 8 years ago by jason

  • Reviewers changed from Jaosn Grout to Jason Grout

comment:24 follow-up: Changed 8 years ago by ddrake

This looks good. We should somehow document or advertise this functionality. Perhaps something should be added to the upload page? That could be a separate ticket; we don't want to bog this one down.

comment:25 Changed 8 years ago by nthiery

  • Status changed from needs_review to positive_review

comment:26 in reply to: ↑ 22 Changed 8 years ago by nthiery

  • Authors changed from Nicolas M. Thiéry, ... to Nicolas M. Thiéry

Replying to jason:

nthiery: positive review on your patch, modulo the changes in my patch. Can you review my patch?

Three substantial changes:

  1. I changed the doctest to use a bit more of the actual generated documentation file structure.
  1. I changed the linebreaks in the doctest so that the string "sage: " did not appear as consecutive characters in input (this works around the bug in the doctesting system which replaces "sage:" with ">>>")

Ah ah! Good catch.

  1. I deleted the line which prepended the title and system to the text of the worksheet. Those don't do anything but show up as the first part of the first text cell, which is silly. The default system is already 'sage', and the title is set when the worksheet is created.

Can you review my reviewer patch? If you think it's good, set this ticket to positive review.

It's all good. Thanks much for the review (on behalf of the Sage Days 30 participants)!

Cheers,

Nicolas

comment:27 Changed 8 years ago by nthiery

  • Description modified (diff)

comment:28 in reply to: ↑ 24 Changed 8 years ago by nthiery

  • Description modified (diff)

Replying to ddrake:

This looks good. We should somehow document or advertise this functionality. Perhaps something should be added to the upload page? That could be a separate ticket; we don't want to bog this one down.

I uploaded an extra mini patch doing that. Please add it to the Apply list if you are happy with it. I left the positive review so as not to prevent Jeroen to merge the two first patches in the mean time. If this happens, we will just open a new ticket.

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

Wouldn't a new sagenb package need to be produced first for Jeroen to merge it? Or is it okay for the release manager to just do hg_sagenb.apply or whatever and be done with it?

comment:30 Changed 8 years ago by jdemeyer

  • Description modified (diff)

comment:31 in reply to: ↑ 29 ; follow-up: Changed 8 years ago by jdemeyer

Replying to kcrisman:

Wouldn't a new sagenb package need to be produced first for Jeroen to merge it?

I consider sagenb to be part of Sage; hence I merge patches just like I merge extcode patches for example. So you don't need to produce any sagenb spkg.

comment:32 in reply to: ↑ 31 ; follow-up: Changed 8 years ago by jason

Replying to jdemeyer:

I consider sagenb to be part of Sage; hence I merge patches just like I merge extcode patches for example. So you don't need to produce any sagenb spkg.

Thanks!

comment:33 in reply to: ↑ 32 Changed 8 years ago by kcrisman

Replying to jason:

Replying to jdemeyer:

I consider sagenb to be part of Sage; hence I merge patches just like I merge extcode patches for example. So you don't need to produce any sagenb spkg.

As my daughter would say, "I love that a googol!"

+oo

In that case I will definitely get to work on other sagenb stuff pronto. What great news!

comment:34 follow-up: Changed 8 years ago by jdemeyer

  • Status changed from positive_review to needs_work

Patch trac_10652-upload-static-documentation-as-worksheet-advertise-nt.patch needs a proper commit message (use hg qrefresh -e for that, keep in mind that the ticket number should be on the first line)>

comment:35 in reply to: ↑ 34 Changed 8 years ago by nthiery

  • Status changed from needs_work to positive_review

Replying to jdemeyer:

Patch trac_10652-upload-static-documentation-as-worksheet-advertise-nt.patch needs a proper commit message (use hg qrefresh -e for that, keep in mind that the ticket number should be on the first line)>

Done!

comment:36 Changed 8 years ago by jdemeyer

  • Merged in set to sage-4.7.alpha4
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:37 Changed 8 years ago by nthiery

Yippee! Thanks Jeroen!

Note: See TracTickets for help on using tickets.