Opened 6 years ago
Closed 6 years ago
#17280 closed defect (fixed)
Notebook commandline argparse
Reported by:  vbraun  Owned by:  

Priority:  blocker  Milestone:  sage6.4 
Component:  scripts  Keywords:  
Cc:  Merged in:  
Authors:  Volker Braun  Reviewers:  Clemens Heuberger, KarlDieter Crisman 
Report Upstream:  N/A  Work issues:  
Branch:  4a0fc71 (Commits, GitHub, GitLab)  Commit:  4a0fc71154e247533353b77ce09aefe8a0c5d1bb 
Dependencies:  Stopgaps: 
Description (last modified by )
 Do not require quotes in
sage notebook sagenb foo=bar
 Handle

to finish options:sage notebook  foo=bar
only passesfoo=bar
to sagenb and does not treat it as the value fornotebook
Change History (28)
comment:1 Changed 6 years ago by
 Component changed from PLEASE CHANGE to scripts
 Type changed from PLEASE CHANGE to defect
comment:2 Changed 6 years ago by
 Branch set to u/vbraun/notebook_commandline_argparse
comment:3 Changed 6 years ago by
 Commit set to ce9db90e99881ed20cda5a3a3d490365b912b7b2
comment:4 Changed 6 years ago by
 Status changed from new to needs_review
comment:5 Changed 6 years ago by
 Description modified (diff)
comment:6 Changed 6 years ago by
 Priority changed from major to blocker
comment:7 Changed 6 years ago by
Is there a way to doctest this parsing? If not, it's okay, just asking. Thanks for opening this.
comment:8 Changed 6 years ago by
The doctesting framework doesn't look into src/bin/
...
comment:9 Changed 6 years ago by
Oh, I meant in another file, perhaps in the notebook (though that is also hard to change quickly, sigh for it being this separate project...)
comment:10 Changed 6 years ago by
You can start a bunch of notebook servers with different options, but imho thats dog slow. The right way to do it is to look at doctests in src/bin
scripts.
comment:11 Changed 6 years ago by
That may be true.
But what I meant was just something that tested the parsing of the options, as a guard against when someone someday changes that to deal with another special case. That presumably is something that could be done, but maybe not without testing the scripts. Anyway, if it's not easy I don't want to hold this up  my apologies that I can't test it right now.
comment:12 Changed 6 years ago by
Once more: Not with the current doctest framework. Sure, there are all kinds of ugly hacks that one could do. But there is only one way to do it right, and we aren't set up for it.
comment:13 followup: ↓ 14 Changed 6 years ago by
From sagerelease, a suggestion to get this finalized:
 whether the option notebookdir is documented in ./sage h (or advanced); IMHO it should be
It is not, and that points us to
$ ./sage notebook help usage: sagenotebook [h] [log LOG] [notebook [NOTEBOOK]] The Sage notebook launcher is used to start the notebook, and allows you to choose between different implementations. Any further command line options are passed to the respective notebook. optional arguments: h, help show this help message and exit. Can be combined with "notebook=[...]" to see notebookspecific options log LOG one of [DEBUG, INFO, ERROR, WARNING, CRITICAL] notebook [NOTEBOOK], n [NOTEBOOK], notebook [NOTEBOOK] The notebook to run [one of: default, ipython, sagenb]. Default is sagenb
which also doesn't help, while that points us to
$ ./sage notebook=sagenb help
which unfortunately just gives the notebook? commands but doesn't tell how to use them with the notebookspecific options.
 on whether the old behaviour "n directory=sage.sagenb" is documented somewhere and whether this documentation has been adapted to the new situation.
It was documented in the following way, in sage advanced
n, notebook [...]  start the Sage notebook (options are the same as for the notebook command in Sage)
So I would say that since I do agree that anyone using this option probably is savvy enough to actually read the help, perhaps documenting exactly how the options are to be called in sage advanced
and/or sage notebook help
would be enough to resolve this issue. (Changing sage notebook=sagenb help
would require more annoyance than worth.) This should be pretty easy to do in the script itself, and given that we never said exactly how to *call* the options before I think that as long as we clearly document this (not necessarily with zillions of examples, but maybe two) then this would work okay.
One could at the same time add the info about the default directory, incidentally.
comment:14 in reply to: ↑ 13 ; followup: ↓ 15 Changed 6 years ago by
Replying to kcrisman:
which unfortunately just gives the notebook? commands but doesn't tell how to use them with the notebookspecific options.
Which part of "Any further command line options are passed to the respective notebook" is unclear?
If you want to add to the onlinehelp then just post a commit. IMHO it is clear and concise. There is always a tradeoff between longer and buried in a text wall.
comment:15 in reply to: ↑ 14 Changed 6 years ago by
which unfortunately just gives the notebook? commands but doesn't tell how to use them with the notebookspecific options.
Which part of "Any further command line options are passed to the respective notebook" is unclear?
Because user X will know to read help, but may NOT understand the arcane syntax for doing so! Namely, I would think this continues to allow
sage notebook foo=bar
because I had never even heard of the sage notebook  foo=bar
syntax. Yes, I'm stupid and uneducated. Well, so will many users be. Especially since I was wrong  I said
It was documented in the following way,
but it still is documented this way.
If you want to add to the onlinehelp then just post a commit. IMHO it is clear and concise. There is always a tradeoff between longer and buried in a text wall.
Yes, of course. Though Let me see if I can cook something up; my concern is that I will make a stupid mistake, while you speak this syntax natively, as it were. For instance, is this a way to spice this up correctly?
usage: sagenotebook [h] [log LOG] [notebook [NOTEBOOK]] [option=foo ...] The Sage notebook launcher is used to start the notebook, and allows you to choose between different implementations. Any further command line options are passed to the respective notebook. optional arguments: h, help show this help message and exit. Can be combined with "notebook=[...]" to see notebookspecific options log LOG one of [DEBUG, INFO, ERROR, WARNING, CRITICAL] notebook [NOTEBOOK], n [NOTEBOOK], notebook [NOTEBOOK] The notebook to run [one of: default, ipython, sagenb]. Default is sagenb}}} option=foo Additional options to be sent to the notebook can be added using this syntax
comment:16 Changed 6 years ago by
Argparse autogenerates the help from the available options, you can't make options appear in the online help without handling them.
comment:17 Changed 6 years ago by
Oh.
BUT we do have
usage_advanced() { .... }
and at the very least
echo " n, notebook [...]  start the Sage notebook (options are the same" echo " as for the notebook command in Sage)"
could be fixed somehow. From looking at src/bin/sage
it looks like this only runs the default notebook.
And would this be acceptable?
description = \ """ The Sage notebook launcher is used to start the notebook, and allows you to choose between different implementations. Any further command line options, in the form `option=foo`, are passed to the respective notebook. """
comment:18 followup: ↓ 19 Changed 6 years ago by
Yes, you can put anything in the description.
comment:19 in reply to: ↑ 18 Changed 6 years ago by
Yes, you can put anything in the description.
Okay, I'll do that. But is it correct? That's what I meant, sorry. I'll do that and sage advanced
and that should resolve the last bit here, assuming someone looks at and tests your actual change.
comment:20 Changed 6 years ago by
It doesn't have to have the form option=foo
, anything else is passed to the notebook. E.g. port=666
without dashes. How about just giving two examples
sage n default port=1234 # pass port=1234 to default notebook sage notebook=ipython notebookdir=/home/foo/bar # ipython notebook in custom dir
comment:21 followup: ↓ 23 Changed 6 years ago by
Thanks, I'll do something along those lines (was working on it but then was interrupted by something highpriority). I think the point was that one needed option=foo
if one didn't say which type of notebook, right?
comment:22 Changed 6 years ago by
I don't want to commit until I'm sure I'm following standards for mantype help... here is a potential commit.

src/bin/sage
diff git a/src/bin/sage b/src/bin/sage index 06dfef8..f3551d5 100755
a b usage_advanced() { 72 72 #### 1.......................26..................................................78 73 73 #### ......................................................................... 74 74 echo "Running the notebook:" 75 echo " notebook=[...]  start the Sage notebook (valid options are" 76 echo " 'default', 'sagenb', and 'ipython'). See the output" 77 echo " 'of sage notebook help for more details" 75 echo " notebook[=default] [...]  start the Sage notebook (valid options are" 76 echo " 'default', 'sagenb', and 'ipython'). Notebookspecific" 77 echo " options are passed on; if notebook is not specified, they" 78 echo " must be in the form 'option=foo' or ' option=foo'." 79 echo " See sage notebook help for more details" 78 80 echo " bn, buildandnotebook [...]  build the Sage library then start" 79 81 echo " the Sage notebook" 80 82 echo " inotebook [...]  start the *insecure* Sage notebook (deprecated)" 81 echo " n, notebook [...]  start the Sage notebook (options are the same" 82 echo " as for the notebook command in Sage)" 83 echo " n, notebook [...]  start the default Sage notebook. (Options are the same" 84 echo " as for the notebook command in Sage, passed in the form" 85 echo " 'option=foo'.)" 83 86 84 87 echo 85 88 #### 1.......................26..................................................78 
src/bin/sagenotebook
diff git a/src/bin/sagenotebook b/src/bin/sagenotebook index d2a6c93..042b72f 100755
a b description = \ 77 77 The Sage notebook launcher is used to start the notebook, and allows 78 78 you to choose between different implementations. Any further command 79 79 line options are passed to the respective notebook. 80 81 Examples: 82 83 sage n default port=1234 # pass port=1234 to default notebook 84 sage notebook=ipython notebookdir=/home/foo/bar # ipython notebook in custom dir 80 85 """ 81 86 82 87 help_help = \
comment:23 in reply to: ↑ 21 Changed 6 years ago by
 Status changed from needs_review to needs_work
I tested a few things.
Does not work as expected:
 Directory with slashes
./sage n default directory=/tmp/test Traceback (most recent call last): ... File "/local/sage/sage6.4.rc1/local/lib/python/ast.py", line 37, in parse return compile(source, filename, mode, PyCF_ONLY_AST) File "<unknown>", line 1 /tmp/test ^ SyntaxError: invalid syntax
This worked in 6.3:/local/sage/sage6.3/sage n directory=/tmp/test
Work as expected:
./sage n ./sage n default ./sage n  directory=test ./sage n default directory=test.sagenb ./sage n sagenb "directory='/tmp/test'" ./sage n  "directory='/tmp/test'" ./sage n default port=1234 ./sage n  port=1234
Replying to kcrisman:
I think the point was that one needed
option=foo
if one didn't say which type of notebook, right?
I do not see what you mean here; I could not get any of directory=test
or option=directory=test
to work, but I did not really expect it previously; but as I said, I do not see what you mean with option=foo
.
comment:24 Changed 6 years ago by
 Commit changed from ce9db90e99881ed20cda5a3a3d490365b912b7b2 to 1baf6fe4bd539afe53254ecc0deb125d1c99c578
comment:25 Changed 6 years ago by
Thanks, fixed. I also added the examples:
$ ./sage n h usage: sagenotebook [h] [log LOG] [notebook [NOTEBOOK]] The Sage notebook launcher is used to start the notebook, and allows you to choose between different implementations. Any further command line options are passed to the respective notebook. optional arguments: h, help show this help message and exit. Can be combined with "notebook=[...]" to see notebookspecific options log LOG one of [DEBUG, INFO, ERROR, WARNING, CRITICAL] notebook [NOTEBOOK], n [NOTEBOOK], notebook [NOTEBOOK] The notebook to run [one of: default, ipython, sagenb]. Default is sagenb EXAMPLES: * Run default notebook on port 1234. Note that the first argument after "n" will be interpreted as notebook name unless you stop processing with "": sage n default port=1234 sage n  port=1234 # equivalent sage n port=1234 # ERROR: invalid notebook name * Run IPython notebook in custom directory: sage notebook=ipython notebookdir=/home/foo/bar
comment:26 Changed 6 years ago by
 Branch changed from u/vbraun/notebook_commandline_argparse to u/kcrisman/17280
 Commit changed from 1baf6fe4bd539afe53254ecc0deb125d1c99c578 to 4a0fc71154e247533353b77ce09aefe8a0c5d1bb
 Reviewers set to Clemens Heuberger, KarlDieter Crisman
 Status changed from needs_work to needs_review
Thanks for the feedback, Clemens, that's very helpful. I checked that this fixed it, and it certainly shouldn't break anything.
Volker  oh, that makes great sense, and I like the way you did it. I hope the following reviewer patch is acceptable for src/bin/sage
.
New commits:
4a0fc71  Add details to sage advanced

comment:27 Changed 6 years ago by
 Status changed from needs_review to positive_review
comment:28 Changed 6 years ago by
 Branch changed from u/kcrisman/17280 to 4a0fc71154e247533353b77ce09aefe8a0c5d1bb
 Resolution set to fixed
 Status changed from positive_review to closed
Branch pushed to git repo; I updated commit sha1. New commits:
Do not pass "" option on to notebook