Sws cmdline

[gvol]

This is for trac #8473 which is meant to allow double-clicking sws (and related files) to open in a locally running Sage notebook, starting it if need be.

[AlexJuarez]

can this check be done with regexp instead?

[gvol]

On Feb 15, 2012, at 11:07 PM, AlexJuarez wrote:

response = redirect(worksheet_datafile.url_for(worksheet, name=name))

• if url != '':
• if url != '' and url[0:7] == 'file://':

•    f = file(dest, 'wb')
  

•    fin = url[7:]
  

•    if fin.startswith('localhost'):
  

can this check be done with regexp instead?

---

Reply to this email directly or view it on GitHub:
https://github.com/sagemath/sagenb/pull/31/files#r455079

Done. Now that the line is gone I don't know how to comment on it, so hopefully you get this. :-)

Thanks for taking the time to review this.

-Ivan

[kini]

I merged master into this branch - you can add my commit with git pull git@github.com/kini/sagenb sws-cmdline:sws-cmdline. Hopefully I did the merge correctly - you might want to check the two files mentioned in the commit message to make sure nothing funky happened. Or look at this diff to make sure it matches what you are trying to add.

[kini]

Sorry, there was a minor glitch. Look at this diff instead (I've updated the branch head as well).

[gvol]

The diff looks fine. Sorry I only recently got around to actually testing it though. Things seem to be in order. How do I update my pull request?

[gvol]

Actually never mind, I think I figured it out.

[kini]

Yup, you did :) Now for someone to test/review this pull request...

[kcrisman]

It works!!! But one has to have the absolute path, so annoying.

Can people at notebook days please check this out? The code seems okay but I'm unfamiliar with the new notebook so there might be other places to check.

[kcrisman]

Here is what happens when a server is already running (gvol pointed out that this check was gone in the code up here):

Please wait while the Sage Notebook server starts...
...
notebook(upload=r'''/Users/.../Denny345 - least squares.sws''')
The notebook files are stored in: sage_notebook.sagenb
Another Sage Notebook server is running, PID 43677.
Opening web browser at http://localhost:8000/ ...
Traceback (most recent call last):
  File "/Users/.../sage-5.1.beta1-flask/local/bin/sage-notebook", line 34, in <module>
exec cmd
  File "<string>", line 1, in <module>
  File "/Users/.../sage-5.1.beta1-flask/devel/sagenb/sagenb/notebook/notebook_object.py", line 206, in __call__
    return self.notebook(*args, **kwds)
  File "/Users/.../sage-5.1.beta1-flask/devel/sagenb/sagenb/notebook/run_notebook.py", line 449, in notebook_twisted
    return run(port)
  File "/Users/.../sage-5.1.beta1-flask/devel/sagenb/sagenb/notebook/run_notebook.py", line 364, in run
    browse_to(old_interface, old_port, old_secure, startpath)
NameError: global name 'startpath' is not defined

[gvol]

I merged, did some testing and updated the pull request. Sorry for the whitespace changes, I was too worried about merging it correctly to get rid of them. Anyway, I tested with

sage --notebook upload=/path/to/file.sws

and

sage --notebook upload=/path/to/file.sws server=flask

and things seemed to work okay except that the flask server will start up another instance. Since twistd is still the default (am I reading that right?) I thought I would let people review this rather than try and fix that myself. I did not test with uwsgi since I don't have it installed. It probably needs help too with starting a new server.

[kcrisman]

I cannot figure out how to take these changes that I can see at https://github.com/sagemath/sagenb/pull/31/files and merge them in my sagenb (either on Github or otherwise). I'd really like to test them, and the code seems ok, though I haven't looked at it as much in context as I would like.

There is the pull/31.patch file, but it's clearly the "old" version before the "massive changes" commit.

Kini, you have a lot of 'splaining left before I'm convinced that Github is a panacea.

At this point I can't even tell whether I have the new notebook or not - supposedly I do, I followed the instructions at #11080... sigh.

[kini]

I cannot figure out how to take these changes that I can see at https://github.com/sagemath/sagenb/pull/31/files and merge them in my sagenb (either on Github or otherwise).   I'd really like to test them, and the code seems ok, though I haven't looked at it as much in context as I would like.

There is the pull/31.patch file, but it's clearly the "old" version before the "massive changes" commit.

You're right, that file is only generated from the initial post in the
pull-request thread (i.e. the first six commits that Ivan had posted
here). Where did you even find that? I can't see it linked anywhere on
the page. It's pretty useless anyway, as you found out.

Kini, you have a lot of 'splaining left before I'm convinced that Github is a panacea.

Crisman, when I tried to offer you a dose of git 'splaining at SD40.5
you emphatically didn't want to hear it, so I'm not sure why you
suddenly want 'splaining from me. For the record, I'm not the one who
switched the Sage notebook to git, so I'm also not sure why you want
it from me in particular.

As for the Sage library's git switchover, we will try our best to
make sure you can work on the Sage library with as little
contamination with 'splaining as possible by the time we move that to
git, which you will note we have not yet done. And in any case, we
have no plans to move development of the Sage library to github.

At this point I can't even tell whether I have the new notebook or not - supposedly I do, I followed the instructions at #11080... sigh.

That is simple. Look at the sagenb directory. If it has flask_version
in it, you have some version of the flask notebook. If it doesn't, you
don't. If you want to find out exactly what version of the flask
notebook you have, you'll need to read the history graph in your local
repository a bit, i.e. use git.

Honestly, a review from someone who can't figure out whether they have
correctly obtained the code they are trying to review, and are even
confusing it with versions of the code which are more than 18 months
old, is not a very useful review, IMHO. Thanks for trying, at least.

Anyway, thank you very much, Ivan, for resolving the merge conflict.

[kcrisman]

Well, we can all day long at each other. It remains a fact that I only mentioned kini because he was on this ticket. I guess I should have put a ;-) in that comment, if the 'splaining didn't make it clear enough. (Pull request #61 was another story.)

Serious talk:

Yes, of course I have flask_version in it. But when I run it I get

2012-06-13 21:16:17-0400 [-] twistd 12.0.0 (/Users/.../sage-5.1.beta1-flask/local/bin/python 2.7.2) starting up.
2012-06-13 21:16:17-0400 [-] reactor class: twisted.internet.selectreactor.SelectReactor.

and no one has mentioned having to do anything special to get flask to be used.

Now I see that 1174ddc is the commit where this was explained - that was post the spkg used in #11080. In fact, apparently one can't even use flask until then? See 1174ddc#L1R308 At any rate, I can't figure it out, so I can't (yet) test this, not to mention

Is is possible (I ask fearfully) to just drop the sagenb code from the Github versions and replace the old devel/sagenb directory with this (possibly modulo some soft links)? Otherwise this is already based on code far enough ahead of #11080 that I'm not sure how to even keep up. That said, I do indeed want to review it.

[kini]

That commit is about using flask as a wsgi container, not just as a web application framework. If you have the flask_version, you are using a version of sagenb which uses flask as a web application framework, which is what "the flask notebook" means. The version of sagenb on trac 11080 uses flask for a web application framework but twisted for a wsgi container (which is a much more hands-off role to fill, which is why we can swap it out for any of two different options with simply a keyword argument, as seen in the line of code you linked).

[kini]

Is is possible (I ask fearfully) to just drop the sagenb code from the Github versions and replace the old devel/sagenb directory with this (possibly modulo some soft links)? Otherwise this is already based on code far enough ahead of #11080 that I'm not sure how to even keep up. That said, I do indeed want to review it.

Yes, that works, depending on what you mean by "the Github versions", and assuming you mean "replace the old devel/sagenb directory with this and apply all the patches on trac 11080".

Of course, the correct way to do it is to have a clone of the sagenb repo and fetch and check out this commit into it.

[kcrisman]

I see, so the flask notebook doesn't run flask by default as the "server" application that starts up with sage -n.

Yes, I did mean to replace that directory with this (or a symlink); the patches at Trac 11080 have already been applied. I may try this so I can test gvol's changes better.

How did you comment on my comments? I can't figure out how to do that within the web interface. Probably one has to use the email interface?

[kini]

I just copied and pasted segments of your comment and put a > before them :) Github comments use Markdown syntax, the same syntax that's used on reddit comments, if you're familiar with that.

[kcrisman]

I just copied and pasted segments of your comment and put a > before them :)

Oh, I never dreamed it would have to be by hand :)
Actually, this is the same syntax Trac and some others use, right? I do notice one needs a blank line between, which you don't on Trac.

[kcrisman]

Okay, now I have it, thanks to some extra hints at another ticket.

Sweet! All of the following work:

• full path in quotes
• full path using escaped spaces not in quotes
• local path
• expands ~ on Mac, at least
• opens second window fine in default server of twisted AND with server=flask, as long as I don't invoke the second time with server=flask. That is to say, in general one shouldn't do sage -n server=flask and then repeat that command; sage -n extra_args="foo" should be enough the second time whether you are uploading files or not. I would not view this as a bug, certainly not with this patch - excuse me, pull request ;)
• Non-ASCII characters like ./sage-5.1.beta1-flask/sage -n upload=~/Downloads/Testцук더ücharacter\ setes.sws

ALL of this from the sage -n syntax, so I presume that all the same would work, ceteris paribus? Or does that need testing as well? Ivan, what else can you think of to test? It would be very hard for me to test it on any other system (Linux, e.g.) but I don't see how that can make a difference.

I'm going to look at the code now. Some of it can obviously remain positive review, like the stuff in notebook_object.py.

[kini]

~ is expanded by your shell, by the way. Thanks for reviewing this!

[kcrisman]

~ is expanded by your shell, by the way.
Ah, but in previous versions of this it wasn't, for whatever reason, which is why I point it out.

Question.

if (kw['automatic_login'] or kw['upload'])

So if for some reason one didn't have automatic login but still had upload, it would load? Is that in theory a security flaw? Or should we leave that as

if kw['automatic_login'] 

I just don't know whether the original reasoning would hold, given the new way this is written with the kw and so forth, maybe this would be worse than in the past. Of course, Ivan has pointed out before that this will only work on localhost anyway, right?

Very dumb comments/questions:

• Any difference between urllib and urllib2 with respect to this?
• Everything translates fine with the characters, but I notice I get things like \xd1\x86\xd1\x83\xd0\xba\xe1\x84\x83 (which is UTF-8?) instead of Unicode. I guess that's ok, but just pointing it out.

Finally, one actual bug. In line 87 or wherever of run_notebook.py, in prepare_kwds, in the elif clause, I see kw['open_page'] = kw['open_page']+ ... but kw['open_page'] might not be set there. That code makes sense in the else clause, but I'm not sure if it works otherwise.

Indeed,

$ ./sage-5.1.beta1-flask/sage -n automatic_login=False upload=FLOTEXAMPLE.sws
Please wait while the Sage Notebook server starts...
...
notebook(automatic_login=False,upload=r'''FLOTEXAMPLE.sws''')
The notebook files are stored in: sage_notebook.sagenb
Traceback (most recent call last):
    kw['open_page'] = kw['open_page']+ "from sagenb.misc.misc import open_page; open_page('%(hostname)s', %(port)s, %(secure)s, %(start_path)s)" % kw
KeyError: 'open_page'

So with respect to my earlier comment, I guess that it there isn't a security hole (yet), because you're guaranteed to get a KeyError, but someone that's not what I think was intended.

[kcrisman]

Okay, the rest of the code looks good. Got a good education in the current notebook design - certainly easier to read, good work!

So the above issue with automatic_login=False and upload "needs work", I'll put a pointer to that at the Trac ticket as well. Otherwise I think the rest is ok.

[kcrisman]

Forgot this:

By the way, although this shouldn't be Ivan's job, at least on Mac I was able to get a PID by using ps and looking for sage_notebook.sagenb/run_flask, so maybe that todo should be todone elsewhere.

But not on this ticket, clearly not an issue unless someone specifically asks for server=flask. If eventually this is fixed, then one may want to abstract out that part of the code that handles this case.

A final dumb thing. What if someone passed

sage -n upload="/path/to/foo.sws /path/to/bar.sws" 

or even

sage -n upload=/path/to/foo.sws /path/to/bar.sws

and expected both to show up? The regex match would only get the first one, I think. I think that's okay, since the doc says nothing about two of them, but wanted to mention it.

[gvol]

I commented there already. They should be fine.

[kini]

It's odd to me that the mac app is even in a Sage repo (the data/extcode repo). I don't see why it isn't just its own SPKG, with an upstream project administrated by you and with issues tracked on github or bitbucket or something.

[kini]

Looks good enough to me.

[kcrisman]

It's odd to me that the mac app is even in a Sage repo (the data/extcode repo). I don't see why it isn't just its own SPKG, with an upstream project administrated by you and with issues tracked on github or bitbucket or something.

You'd have to ask Ivan about that. Personally, I feel that that code etc. has no purpose for existing outside of Sage, so it makes sense to have it be part of Sage. After all, we do have the extcode repo out there. It also makes it easier to administer - no forking outside repos and stuff like that, just do stuff within Sage.

[kini]

Well, sure, but since Ivan is the only person who ever edits it, is there even anyone qualified to review his patches? It seems a little silly for him to have to make patches and wait for review, just to make edits to an app which nobody else has ever written code for. I checked the logs in the data/extcode Sage repo, and the only time anyone other than Ivan has ever edited the mac app is for some trivial thing like changing a copyright notice or something.

[kini]

By the way, I did ask Ivan about it. @gvol is Ivan :)

[gvol]

At the time, it seemed like a good idea. Looking back, I'm not so sure because it means I have to get everything reviewed instead just making a new release when I want. :-) Of course that means it gets tested, so I suppose it's not all bad.

[kcrisman]

By the way, I did ask Ivan about it. @gvol is Ivan :)

Oh, I didn't realize you didn't know that. Sorry - Ivan has many aliases :)

It's true that no one else has committed to it, but I do try to read the code as thoroughly as I can. I don't claim to understand every single line, but I do understand the overall structure and try to test it in ways that deal with whatever change is being made. I hope Ivan's been happy with my work :)

[gvol]

I've been very happy. In fact, I've decided to double what I'm paying you. :-)

[jhpalmieri]

I'm having problems with this. I'm using OS X 10.7, and I've installed the sagenb spkg from [http://trac.sagemath.org/sage_trac/ticket/13121](which includes this change), along with all of its prerequisites. When I run sage -n upload="/full/path/to/worksheet.sws", most of the time it doesn't work: two browser windows are opened, both to my list of active worksheets, neither showing the uploaded worksheet. If I pass automatic_login=False as another command-line argument, then I sometimes get a single browser window asking me to log in, and once I log in, I get to the uploaded worksheet. But I'm not sure that happens all of the time, either. Any suggestions?

Also, using a tilde in the path (upload="~/Downloads/new.sws") doesn't work. That's a little annoying, and shouldn't be hard to fix.

[gvol]

Expanding the tilde is done by the shell so it will depend entirely upon how you quote it:

upload="~/Downloads/new.sws"

won't be expanded, but any of

upload=~/"Downloads/new.sws"
upload=~/Downloads/"new.sws"
upload=~/Downloads/new.sws

will be. Also, a relative path should be okay as well.

Are you starting the notebook server, or do you have it (or worse, an old version) already running? Or rather, does it not work in both cases, or just one? If you start it from the command line is there any error logging?

[jhpalmieri]

For the tilde, I was thinking that you might use Python's os.path.expanduser method. Relative paths are working just fine, by the way.

As far as the notebook server: I don't have one running until I give the command from the shell. The output at the command line looks essentially identical (except when it repeats the arguments) if I pass automatic_login=False or not, so there is no error logging.

I just tried again with a new .sage directory, but that didn't help.

[kini]

Does it work if you check out the latest commit in this branch before the
merge? I can't test this at the moment.

achtung: sent from my phone, may be unduly terse

[jhpalmieri]

I took my existing installation and undid the one-line change in the latest commit. That doesn't fix the problem, but it breaks automatic_login=False, I think as expected.

[gvol]

I think what he meant was check out this branch in case the merge into mainline was the cause. I'll try to reproduce this tomorrow or Monday, but I have to go to bed right now. What version of Sage are you using?

If a server is already running, does it work then?

We could probably use os.path.expanduser, but then having an actual tilde in the filename might cause problems. I don't really have an opinion on it either way since I don't plan on using tildes in my filenames anytime soon.

[jhpalmieri]

I'm using Sage 5.1.beta5, plus #13121. I'll try merging just this branch to see if that helps. It would be nice if someone else could reproduce this, though.

I never use Sage with the server already running. Do I still use the syntax sage -n upload=...?

I think that os.path.expanduser should behave well, and will be less surprising to users than not having tildes expanded. But that should probably go in a later version.

[jhpalmieri]

I see the same problem with just this branch. (I merged the new notebook and all of the other changes from #11080, and then cloned a new version of the sagenb git repository, checked out this branch, and pointed the link devel/sagenb to the resulting sagenb directory.)

[kcrisman]

I'm sorry for the trouble; thanks for checking into this, John. In the previous incarnation of this, I had a lot of similar (though not identical problems) to this, and really had a lot of trouble breaking it this time around. I guess it's good that you were able to break it... ? I wonder what is different in this case. Ivan, could his computer not be logging him in quickly enough to the notebook when he uploads the worksheet? That would possibly cause the problem here.

By the way, you're right that in the current state it's not that exciting to have a notebook already running and then do this at the command line instead of just using the upload dialogue, but we have a lot of anecdotal evidence that many people would love the next step, the double-click sws.

[jhpalmieri]

I see: if I have a server already running and try sage -n upload=..., it works, telling me

Another Sage Notebook server is running, PID 3582.
Opening web browser at http://localhost:8080/upload_worksheet?url=file:///Users/palmieri/Downloads/extreme.sws ...

[kini]

OK, I can confirm what @jhpalmieri sees. If a server is not already running, the browser opens twice and the sws file does not get loaded. If a server is already running, everything works fine.

[gvol]

I have been able to reproduce it intermittently--though usually it works. It seems that when it doesn't work the sws file is uploaded, but it just opens the page to the worksheet list instead of editing the worksheet.

My guess is that it's a timing issue with not being logged in. That is something I was worried about, but since it worked in my testing and it redirects correctly when not logged in (e.g. with automatic_login=False), I didn't worry too much about it. I guess I'll have to find out what's going wrong. Should I open a trac ticket, or just a github issue?

[kini]

Since the code resides entirely in the notebook, you should open a github issue, and then put the text "fix #n" (replace n with the issue number) in the commit message of the commit in which you fix the issue :)

[gvol]

I created issue 76 for the non-uploading bug and 77 for expanding tildes.

[gvol]

Actually, I think I was able to fix the issue for #76 already. I didn't expect it to be that easy. :-) I don't think there will be any negative repercussions to always using the next parameter, but I haven't tested it thoroughly.