Opened 13 years ago

Closed 13 years ago

Last modified 13 years ago

#7483 closed enhancement (fixed)

notebook: move preparsing to the worksheet process and out of the server (was: weird pointless line)

Reported by: was Owned by: boothby
Priority: major Milestone: sage-4.3.1
Component: notebook Keywords:
Cc: Merged in: sage-4.3.1.alpha0
Authors: William Stein Reviewers: Mitesh Patel
Report Upstream: N/A Work issues:
Branch: Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by was)

In worksheet.py (at the end of start_next_comp(self)) in sagenb we have these lines:

        self.__comp_is_running = True
        'exec _support_.preparse(base64.b64decode("%s"))'%base64.b64encode(input)
        self.sage().execute(input, os.path.abspath(self.data_directory()))

That 'exec ' line in the middle is just sitting there making a string that is just discarded?

The issue is that I thought I had fully implemented moving all preparsing to the worksheet process (out of the server). Unfortunately, I didn't -- in fact, I only figured out how to do it, but didn't finish.

Attachments (2)

sagenb_7483.patch (4.9 KB) - added by was 13 years ago.
apply to the sagenb spkg
sagelib-7483.patch (2.6 KB) - added by was 13 years ago.
apply to the core sage library

Download all attachments as: .zip

Change History (20)

comment:1 Changed 13 years ago by was

Description: modified (diff)

comment:2 Changed 13 years ago by was

I think I meant

input = 'exec _support_.preparse(base64.b64decode("%s"))'%base64.b64encode(input)

and to get rid of preparsing input at all done by the server. In fact, I know for a fact I implemented things that way so that this wouldn't be broken:

implicit_multiplication(True)
///
Traceback (most recent call last):
...
NotImplementedError: Implicit multiplication not implemented for the notebook.

But now it seems to be broken again.

I can only conclude that a serious mismerge or mangling has occurred to the code I originally wrote, since I definitely had the above working in an earlier version of sagenb.

Hence, this ticket.

comment:3 Changed 13 years ago by was

Description: modified (diff)
Summary: notebook: weird pointless linenotebook: move preparsing to the worksheet process and out of the server (was: weird pointless line)

comment:4 Changed 13 years ago by was

Type: defectenhancement

Changed 13 years ago by was

Attachment: sagenb_7483.patch added

apply to the sagenb spkg

Changed 13 years ago by was

Attachment: sagelib-7483.patch added

apply to the core sage library

comment:5 Changed 13 years ago by was

Status: newneeds_review

comment:6 Changed 13 years ago by was

So the attached patch moves all the preparsing from the notebook server to the worksheet process. I had thought I had made this change long ago, but I definitely hadn't. It's *extremely* important from a security/robustness/speed/flexibility point of view. Why? Because it means the possibly time consuming and definitely _dangerous_ preparsing work gets pushed off to the worksheet processes, which will often be running on some remote virtual machines. That's what we want!

This patch also makes it so the following are now fully supported in the notebook. Note that they both used to not work at all:

  • implicit_multiplication -- turning on and off implicit multiplication
  • preparser -- turning on and off the preparser

comment:7 Changed 13 years ago by was

I've put a new sagenb spkg with just this patch (and the one from 7483) here:

http://wstein.org/home/wstein/patches/sagenb/sagenb-0.4.3.p1.spkg

comment:8 Changed 13 years ago by mpatel

Authors: William Stein
Reviewers: Mitesh Patel
Status: needs_reviewpositive_review

The Selenium test results are unchanged in FF3.5.5 on Linux.

make ptest on sage.math passes.

comment:9 Changed 13 years ago by mpatel

Status: positive_reviewneeds_work

It seems that load and save are now broken in the notebook.

Were attach and detach already broken in the notebook?

comment:10 Changed 13 years ago by was

It seems that load and save are now broken in the notebook. Were attach and detach already broken in the notebook?

No, this broke them. I'll fix the problem now.

comment:11 Changed 13 years ago by was

Clarification: load and save still work on .sage files, but don't work on .py. This is related to #4474. But I'll fix it here now, hopefully.

comment:12 Changed 13 years ago by was

Status: needs_workneeds_review

OK, I went a bit crazy and spent 8 hours completely rewriting and unifying and refactoring all the load/save/attach code. This is at #7514. With that, the above mentioned issued is gone.

comment:13 Changed 13 years ago by mpatel

Report Upstream: N/A

Positive review, once #7514 passes.

Should we also move "docbrowser" generation to a worksheet process?

comment:14 Changed 13 years ago by was

Should we also move "docbrowser" generation to a worksheet process?

Definitely! The more that is done by worksheet processes, the better -- for scalability, security, etc. Every spec of work done by the server is a potential speed and security problem. The more that can be offloaded to worksheets, the better.

comment:15 Changed 13 years ago by mpatel

Status: needs_reviewpositive_review

comment:16 Changed 13 years ago by mhansen

I've merged the sagelib patch in 4.3.1.alpha0

comment:17 Changed 13 years ago by was

Resolution: fixed
Status: positive_reviewclosed

I've merged this into sagenb-0.4.8.

comment:18 Changed 13 years ago by kcrisman

Merged in: sage-4.3.1.alpha0
Note: See TracTickets for help on using tickets.