Opened 6 years ago

Closed 6 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:

Status badges

Description (last modified by vbraun)

  • Do not require quotes in sage --notebook sagenb foo=bar
  • Handle -- to finish options: sage --notebook -- foo=bar only passes foo=bar to sagenb and does not treat it as the value for --notebook

Change History (28)

comment:1 Changed 6 years ago by vbraun

  • Component changed from PLEASE CHANGE to scripts
  • Type changed from PLEASE CHANGE to defect

comment:2 Changed 6 years ago by vbraun

  • Branch set to u/vbraun/notebook_commandline_argparse

comment:3 Changed 6 years ago by git

  • Commit set to ce9db90e99881ed20cda5a3a3d490365b912b7b2

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

ce9db90Do not pass "--" option on to notebook

comment:4 Changed 6 years ago by vbraun

  • Authors set to Volker Braun
  • Status changed from new to needs_review

comment:5 Changed 6 years ago by vbraun

  • Description modified (diff)

comment:6 Changed 6 years ago by vbraun

  • Priority changed from major to blocker

comment:7 Changed 6 years ago by kcrisman

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 vbraun

The doctesting framework doesn't look into src/bin/...

comment:9 Changed 6 years ago by kcrisman

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 vbraun

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 kcrisman

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 vbraun

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: Changed 6 years ago by kcrisman

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 in reply to: ↑ 13 ; follow-up: Changed 6 years ago by vbraun

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 in reply to: ↑ 14 Changed 6 years ago by 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?

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 6 years ago by vbraun

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 6 years ago by kcrisman

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 follow-up: Changed 6 years ago by vbraun

Yes, you can put anything in the description.

comment:19 in reply to: ↑ 18 Changed 6 years ago by kcrisman

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 vbraun

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: Changed 6 years ago by kcrisman

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 6 years ago by kcrisman

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() { 
    7272    ####  1.......................26..................................................78
    7373    ####  |.....................--.|...................................................|
    7474    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"
    7880    echo "  -bn, -build-and-notebook [...] -- build the Sage library then start"
    7981    echo "                         the Sage notebook"
    8082    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'.)"
    8386
    8487    echo
    8588    ####  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 = \ 
    7777The Sage notebook launcher is used to start the notebook, and allows
    7878you to choose between different implementations. Any further command
    7979line options are passed to the respective notebook.
     80
     81Examples:
     82
     83sage -n default port=1234                              # pass port=1234 to default notebook
     84sage --notebook=ipython --notebook-dir=/home/foo/bar   # ipython notebook in custom dir
    8085"""
    8186
    8287help_help = \

comment:23 in reply to: ↑ 21 Changed 6 years ago by cheuberg

  • 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/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 6 years ago by git

  • Commit changed from ce9db90e99881ed20cda5a3a3d490365b912b7b2 to 1baf6fe4bd539afe53254ecc0deb125d1c99c578

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

f868e97also catch SyntaxError when parsing
1baf6feadd examples

comment:25 Changed 6 years ago by vbraun

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 6 years ago by kcrisman

  • Branch changed from u/vbraun/notebook_commandline_argparse to u/kcrisman/17280
  • Commit changed from 1baf6fe4bd539afe53254ecc0deb125d1c99c578 to 4a0fc71154e247533353b77ce09aefe8a0c5d1bb
  • Reviewers set to Clemens Heuberger, Karl-Dieter 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:

4a0fc71Add details to sage --advanced

comment:27 Changed 6 years ago by vbraun

  • Status changed from needs_review to positive_review

comment:28 Changed 6 years ago by vbraun

  • Branch changed from u/kcrisman/17280 to 4a0fc71154e247533353b77ce09aefe8a0c5d1bb
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.