Opened 13 years ago

Closed 10 years ago

#693 closed enhancement (fixed)

Script to spawn a browser / start notebook.

Reported by: boothby Owned by: boothby
Priority: major Milestone: sage-4.7
Component: notebook Keywords:
Cc: was, mpatel, wjp, acleone, mhansen, jdemeyer, mvngu Merged in: sage-4.7.alpha3
Authors: Tim Dumol, Mitesh Patel Reviewers: Ivan Andrus
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Description (last modified by kcrisman)

I've had an icon sitting on my desktop for about a week now. When I click on it, and it starts a notebook in a background terminal and spawns a browser. I'd like to be able to click it a second time, and open another browser window, instead of the current behavior of attempting to start another notebook.

Should work something like this:

if not notebook_is_running:
    notebook(settings from commandline, open_browser=True)
else:
    open_browser(settings from commandline)

Apply trac_693-spawn_notebook.3.patch

Attachments (7)

trac_693-spawn-browser-start-nb.patch (2.8 KB) - added by timdumol 11 years ago.
Modifies sage -notebook to launch a browser window if the notebook is already started. Also adds a sage -nb shortcut.
trac_693-spawn-browser-start-nb.2.patch (3.5 KB) - added by timdumol 11 years ago.
Checks if the notebook server is running too, instead of just checking if the PID exists.
trac_693-spawn-browser-start-nb.3.patch (3.5 KB) - added by timdumol 11 years ago.
Fixes bug with arguments.
trac_693-spawn-browser-start-nb.4.patch (3.1 KB) - added by timdumol 11 years ago.
Same thing, without actually checking if the Twistd process is running
trac_693-spawn-nb.patch (2.4 KB) - added by timdumol 11 years ago.
Changes run_notebook.py to launch a browser to the notebook page should an instance in the directory exist. Apply to sagenb repo. Apply this patch only.
trac_693-spawn-nb.2.patch (2.0 KB) - added by timdumol 11 years ago.
Uses signals only to check if the process exists (as Twisted does)
trac_693-spawn_notebook.3.patch (16.4 KB) - added by mpatel 11 years ago.
Open browser if server running and open_viewer=True. pep8 clean-ups. Rebased for queue in comment. Replaces previous.

Download all attachments as: .zip

Change History (28)

Changed 11 years ago by timdumol

Modifies sage -notebook to launch a browser window if the notebook is already started. Also adds a sage -nb shortcut.

comment:1 Changed 11 years ago by timdumol

  • Authors set to Tim Dumol
  • Cc was mpatel added
  • Status changed from new to needs_review

This patch should do the trick. Apply to scripts repository.

Changed 11 years ago by timdumol

Checks if the notebook server is running too, instead of just checking if the PID exists.

comment:2 Changed 11 years ago by timdumol

I am not sure which method is preferrable, since checking if the notebook server is running does not work if the user has no permission to read /proc and to send signals. Please feel free to approve of either patch.

Changed 11 years ago by timdumol

Fixes bug with arguments.

Changed 11 years ago by timdumol

Same thing, without actually checking if the Twistd process is running

comment:3 Changed 11 years ago by was

  • Status changed from needs_review to needs_work

I don't get it. If I do

$  sage -notebook directory=foo port=8001 & 
$  sage -notebook directory=bar port=8002 

then when I execute the second line it might just pop up a notebook server pointed at port 8001. Actually, given the line:

cmd = "notebook(" + ",".join([wrap(v) for v in sys.argv[1:]]) + ",port=" + SAGENB_PORT + ")"

I think it would give an error, since port= is specified twice.

This is because you introduced a new environment variable SAGENB_PORT which isn't documented. I don't know why it is there. I think you should get the port from the port= option on the command line.

I think you should get port= from the command line and get rid of the SAGENB_PORT environment variable.

Also, you use:

DOT_SAGENB = os.environ.get('DOT_SAGENB', os.path.join(os.environ['HOME'], '.sage', 'sage_notebook.sagenb'))

but actually, you need to use the file os.path.join(D, 'twistd.pid') where D is the option specified in directory= in the command line.

Finally, I think this code should be in the notebook(...) command in Sage itself. It's wrong putting it here in this shell script, because it only half way fixes the problem. E.g., imagine a user that types the following and leaves that in a console:

sage: notebook()

Then in another console, they type

sage: notebook()

Instead of giving an error, it should just given them the notebook. If you put this code that you've just written in the notebook command, then both problems are automatically solved, whereas with the current code location, only half the problem is really solved.

Also, there is a notebook(fork=True) option, so one can do

sage: notebook(fork=True)
The notebook files are stored in: sage_notebook.sagenb
**************************************************
*                                                *
* Open your web browser to http://localhost:8001 *
*                                                *
**************************************************
<pexpect.spawn instance at 0x10bb78bd8>
sage: notebook()
# get some notebook

William

Changed 11 years ago by timdumol

Changes run_notebook.py to launch a browser to the notebook page should an instance in the directory exist. Apply to sagenb repo. Apply this patch only.

comment:4 Changed 11 years ago by timdumol

  • Status changed from needs_work to needs_review

All your points make sense. I have implemented the changes in run_notebook.py. I've decided to check if the process is running, since that's also what twistd does.

comment:5 Changed 11 years ago by was

  • Report Upstream set to N/A
  • Status changed from needs_review to needs_work

On OS X this doesn't work at all. Depending on what I do either I get two notebook servers running simultaneously on the same directory (bad), or I get "Another twistd server is running, PID 40940".

On OS X there is no /proc filesystem. However, when I run this code from this patch:

        try:
            print 1
            # First check using /proc directory
            if os.path.exists('/proc/%d'  % twistd_pid):
                launch_browser_to_nb()
                return
            else:
                remove_pidfile(twistd_pid_path)                
        except:
            print 2

I don't see "2" printed, i.e., no exception is raised. That's clear if you read the code -- you don't raise an exception.

Changed 11 years ago by timdumol

Uses signals only to check if the process exists (as Twisted does)

comment:6 Changed 11 years ago by timdumol

  • Status changed from needs_work to needs_review

I have removed the /proc check, since it's what Twisted does anyways.

comment:7 Changed 11 years ago by mpatel

I guess we need to update the patch to take advantage of #2779?

Changed 11 years ago by mpatel

Open browser if server running and open_viewer=True. pep8 clean-ups. Rebased for queue in comment. Replaces previous.

comment:8 Changed 11 years ago by mpatel

  • Cc wjp added

V3:

  • If a server is running and open_viewer=True, get the settings from the old twistedconf.tac and browse to the server's home page.
  • Use return instead of sys.exit, in case of command-line invocation.
  • Some pep8 changes.
  • Rebased for this queue
    sagenb-0.7 / #8051
    trac_7784-hgignore_update.2.patch
    trac_5712-interrupt-notification.6.patch
    trac_6069-missing_pub_ws.2.patch
    trac_8038-email_plus_addressing_v2.patch
    trac_7506-notebook_object-documentation.2.patch
    trac_693-spawn_notebook.3.patch
    

comment:9 Changed 11 years ago by timdumol

  • Cc acleone added
  • Reviewers set to Tim Dumol

comment:10 Changed 11 years ago by timdumol

  • Authors changed from Tim Dumol to Tim Dumol, Mitesh Patel
  • Reviewers Tim Dumol deleted

comment:11 Changed 10 years ago by mpatel

  • I think #8473 is related.
  • V3 applies almost cleanly to SageNB 0.8:
    applying trac_693-spawn_notebook.3.patch
    patching file sagenb/notebook/run_notebook.py
    Hunk #12 succeeded at 434 with fuzz 2 (offset 3 lines).
    now at: trac_693-spawn_notebook.3.patch
    

comment:12 Changed 10 years ago by mhansen

  • Cc mhansen added

comment:13 Changed 10 years ago by iandrus

Apply trac_693-spawn_notebook.3.patch

comment:14 Changed 10 years ago by kcrisman

I'd like to test #8473, which depends on this, but I'm reluctant to do so until someone who knows something about the notebook takes a look at this. Bug days folks?

comment:15 follow-up: Changed 10 years ago by amog2011

Open browser if server running and open_viewer=True. pep8 clean-ups. Rebased for queue in comment. Replaces previous.

air jordan

comment:16 in reply to: ↑ 15 ; follow-ups: Changed 10 years ago by kcrisman

  • Cc jdemeyer added

Replying to amog2011:

Open browser if server running and open_viewer=True. pep8 clean-ups. Rebased for queue in comment. Replaces previous.

This is a spam comment. Can someone please remove this 'user'? I don't know who has permissions for this.

comment:17 in reply to: ↑ 16 Changed 10 years ago by jdemeyer

  • Cc mvngu added

Replying to kcrisman:

Replying to amog2011:

Open browser if server running and open_viewer=True. pep8 clean-ups. Rebased for queue in comment. Replaces previous.

This is a spam comment. Can someone please remove this 'user'? I don't know who has permissions for this.

Minh has.

comment:18 in reply to: ↑ 16 Changed 10 years ago by mvngu

Replying to kcrisman:

This is a spam comment. Can someone please remove this 'user'? I don't know who has permissions for this.

amog2011 is already banned.

comment:19 follow-up: Changed 10 years ago by iandrus

  • Status changed from needs_review to positive_review

I have been using this (with #8473) for some time without problems. I have also reviewed the code and it looks okay given my limited understanding of the notebook.

comment:20 in reply to: ↑ 19 Changed 10 years ago by kcrisman

  • Description modified (diff)
  • Reviewers set to Ivan Andrus

Replying to iandrus:

I have been using this (with #8473) for some time without problems. I have also reviewed the code and it looks okay given my limited understanding of the notebook.

Thanks. It applies just as cleanly (one hunk misses with fuzz) to current SageNB as in the comment above. This could be added in a new spkg with the Jmol updates.

Apply trac_693-spawn_notebook.3.patch

comment:21 Changed 10 years ago by jdemeyer

  • Merged in set to sage-4.7.alpha3
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.