Opened 8 years ago
Closed 8 years ago
#17280 closed defect (fixed)
Notebook commandline argparse
Reported by: | vbraun | Owned by: | |
---|---|---|---|
Priority: | blocker | Milestone: | sage-6.4 |
Component: | scripts | Keywords: | |
Cc: | Merged in: | ||
Authors: | Volker Braun | Reviewers: | Clemens Heuberger, Karl-Dieter 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 for--notebook
Change History (28)
comment:1 Changed 8 years ago by
Component: | PLEASE CHANGE → scripts |
---|---|
Type: | PLEASE CHANGE → defect |
comment:2 Changed 8 years ago by
Branch: | → u/vbraun/notebook_commandline_argparse |
---|
comment:3 Changed 8 years ago by
Commit: | → ce9db90e99881ed20cda5a3a3d490365b912b7b2 |
---|
comment:4 Changed 8 years ago by
Authors: | → Volker Braun |
---|---|
Status: | new → needs_review |
comment:5 Changed 8 years ago by
Description: | modified (diff) |
---|
comment:6 Changed 8 years ago by
Priority: | major → blocker |
---|
comment:7 Changed 8 years ago by
Is there a way to doctest this parsing? If not, it's okay, just asking. Thanks for opening this.
comment:9 Changed 8 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 8 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 8 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 8 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 follow-up: 14 Changed 8 years ago by
From sage-release, a suggestion to get this finalized:
- whether the option --notebook-dir is documented in ./sage -h (or --advanced); IMHO it should be
It is not, and that points us to
$ ./sage --notebook --help usage: sage-notebook [-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 notebook-specific 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 notebook-specific 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 follow-up: 15 Changed 8 years ago by
Replying to kcrisman:
which unfortunately just gives the notebook? commands but doesn't tell how to use them with the notebook-specific options.
Which part of "Any further command line options are passed to the respective notebook" is unclear?
If you want to add to the online-help 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 Changed 8 years ago by
which unfortunately just gives the notebook? commands but doesn't tell how to use them with the notebook-specific 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 online-help 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: sage-notebook [-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 notebook-specific 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 8 years ago by
Argparse auto-generates the help from the available options, you can't make options appear in the online help without handling them.
comment:17 Changed 8 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:19 Changed 8 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 8 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 --notebook-dir=/home/foo/bar # ipython notebook in custom dir
comment:21 follow-up: 23 Changed 8 years ago by
Thanks, I'll do something along those lines (was working on it but then was interrupted by something high-priority). I think the point was that one needed --option=foo
if one didn't say which type of notebook, right?
comment:22 Changed 8 years ago by
I don't want to commit until I'm sure I'm following standards for man-type 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'). Notebook-specific" 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, -build-and-notebook [...] -- 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/sage-notebook
diff --git a/src/bin/sage-notebook b/src/bin/sage-notebook 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 --notebook-dir=/home/foo/bar # ipython notebook in custom dir 80 85 """ 81 86 82 87 help_help = \
comment:23 Changed 8 years ago by
Status: | needs_review → 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/sage-6.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/sage-6.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 8 years ago by
Commit: | ce9db90e99881ed20cda5a3a3d490365b912b7b2 → 1baf6fe4bd539afe53254ecc0deb125d1c99c578 |
---|
comment:25 Changed 8 years ago by
Thanks, fixed. I also added the examples:
$ ./sage -n -h usage: sage-notebook [-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 notebook-specific 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 --notebook-dir=/home/foo/bar
comment:26 Changed 8 years ago by
Branch: | u/vbraun/notebook_commandline_argparse → u/kcrisman/17280 |
---|---|
Commit: | 1baf6fe4bd539afe53254ecc0deb125d1c99c578 → 4a0fc71154e247533353b77ce09aefe8a0c5d1bb |
Reviewers: | → Clemens Heuberger, Karl-Dieter Crisman |
Status: | needs_work → 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 8 years ago by
Status: | needs_review → positive_review |
---|
comment:28 Changed 8 years ago by
Branch: | u/kcrisman/17280 → 4a0fc71154e247533353b77ce09aefe8a0c5d1bb |
---|---|
Resolution: | → fixed |
Status: | positive_review → closed |
Branch pushed to git repo; I updated commit sha1. New commits:
Do not pass "--" option on to notebook