Opened 11 years ago

Closed 11 years ago

Last modified 11 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 11 years ago.
apply to the sagenb spkg
sagelib-7483.patch (2.6 KB) - added by was 11 years ago.
apply to the core sage library

Download all attachments as: .zip

Change History (20)

comment:1 Changed 11 years ago by was

  • Description modified (diff)

comment:2 Changed 11 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 11 years ago by was

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

comment:4 Changed 11 years ago by was

  • Type changed from defect to enhancement

Changed 11 years ago by was

apply to the sagenb spkg

Changed 11 years ago by was

apply to the core sage library

comment:5 Changed 11 years ago by was

  • Status changed from new to needs_review

comment:6 Changed 11 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 11 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 11 years ago by mpatel

  • Authors set to William Stein
  • Reviewers set to Mitesh Patel
  • Status changed from needs_review to positive_review

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

make ptest on sage.math passes.

comment:9 Changed 11 years ago by mpatel

  • Status changed from positive_review to needs_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 11 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 11 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 11 years ago by was

  • Status changed from needs_work to needs_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 11 years ago by mpatel

  • Report Upstream set to N/A

Positive review, once #7514 passes.

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

comment:14 Changed 11 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 11 years ago by mpatel

  • Status changed from needs_review to positive_review

comment:16 Changed 11 years ago by mhansen

I've merged the sagelib patch in 4.3.1.alpha0

comment:17 Changed 11 years ago by was

  • Resolution set to fixed
  • Status changed from positive_review to closed

I've merged this into sagenb-0.4.8.

comment:18 Changed 11 years ago by kcrisman

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