[babel] [PATCH] Fix sh block execution in session

[babel] [PATCH] Fix sh block execution in session

[Julien Barnier]

Hi,

When evaluating shell code blocks in org-babel, the execution seems to hang
indefinitely. The following patch is trying to fix this problem by modifying
the way shell code is send to comint and the way the end-of-evaluation
indicator is looking for.

As I am far from an emacs lisp expert and as the patch modifies the widely use
org-babel-comint-with-output function, it certainly needs some testing. I've
made some tests with shell and R code blocks, but I'm not sure it's enough.

Thanks !

Julien


Fix sh block execution in a session

* lisp/ob-sh.el (org-babel-sh-evaluate) : when sending input to comint, wait
until previous line execution is finished
* lisp/ob-comint.el (org-babel-comint-with-output) : when looking for
end-of-evaluation indicator, search forward for the indicator before searching
forward for the prompt
---
 lisp/ob-comint.el |    4 ++--
 lisp/ob-sh.el     |    8 +++++++-
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/lisp/ob-comint.el b/lisp/ob-comint.el
index d12ed47..21ff0f6 100644
--- a/lisp/ob-comint.el
+++ b/lisp/ob-comint.el
@@ -93,9 +93,9 @@ or user `keyboard-quit' during execution of body."
 			  (goto-char comint-last-input-end)
 			  (not (save-excursion
 				 (and (re-search-forward
-				       comint-prompt-regexp nil t)
+				       (regexp-quote ,eoe-indicator) nil t)
 				      (re-search-forward
-				       (regexp-quote ,eoe-indicator) nil t)))))
+				       comint-prompt-regexp nil t)))))
 		   (accept-process-output (get-buffer-process (current-buffer)))
 		   ;; thought the following this would allow async
 		   ;; background running, but I was wrong...
diff --git a/lisp/ob-sh.el b/lisp/ob-sh.el
index 10c08d4..b2d1591 100644
--- a/lisp/ob-sh.el
+++ b/lisp/ob-sh.el
@@ -170,7 +170,13 @@ return the value of the last statement in BODY."
 	    (session org-babel-sh-eoe-output t body)
 	  (mapc
 	   (lambda (line)
-	     (insert line) (comint-send-input nil t) (sleep-for 0.25))
+	     (insert line)
+	     (comint-send-input nil t)
+	     (while (save-excursion
+		      (goto-char comint-last-input-end)
+		      (not (re-search-forward
+			    comint-prompt-regexp nil t)))
+	       (accept-process-output (get-buffer-process (current-buffer)))))
 	   (append
 	    (split-string (org-babel-trim body) "\n")
 	    (list org-babel-sh-eoe-indicator))))
-- 
1.7.5.1

Re: [babel] [PATCH] Fix sh block execution in session

[Eric Schulte]

Hi Julien,

Thanks for this patch, the current method of waiting for output in shell
code blocks could certainly use improvement.  After a series of simple
tests I can confirm that this does appear to work for me.  I have a
couple of questions.

1. do you have a minimal example of a shell code block which works after
   your patch but not before?

2. I assume that the changes to ob-comit were required for your ob-sh
   changes to work and were not intended to affect other languages?

3. When switching the order of the prompt and eoe check in ob-comint,
   did you try *only* looking for the eoe indicator?  It seems to me
   that once that is present the check for a followup prompt may not be
   required and it may make sense to simply remove that code.

All in all I agree that the ob-sh changes look like an improvement and
should be applied.  I'm less sure of the changes to ob-comint, simply
because I don't understand the mechanism through which swapping the
regexp checks changes behavior.  However if the ob-sh changes depend
upon the ob-comint changes there is no point in applying them piecemeal.

Situations like this (innocuous looking changes with potentially
wide-ranging potential implications) make me wish Babel had a more
mature and better-maintained test suite.

Thanks -- Eric

Julien Barnier <julien@nozav.org> writes:

> Hi,
>
> When evaluating shell code blocks in org-babel, the execution seems to hang
> indefinitely. The following patch is trying to fix this problem by modifying
> the way shell code is send to comint and the way the end-of-evaluation
> indicator is looking for.
>
> As I am far from an emacs lisp expert and as the patch modifies the widely use
> org-babel-comint-with-output function, it certainly needs some testing. I've
> made some tests with shell and R code blocks, but I'm not sure it's enough.
>
> Thanks !
>
> Julien
>
>
> Fix sh block execution in a session
>
> * lisp/ob-sh.el (org-babel-sh-evaluate) : when sending input to comint, wait
> until previous line execution is finished
> * lisp/ob-comint.el (org-babel-comint-with-output) : when looking for
> end-of-evaluation indicator, search forward for the indicator before searching
> forward for the prompt
> ---
>  lisp/ob-comint.el |    4 ++--
>  lisp/ob-sh.el     |    8 +++++++-
>  2 files changed, 9 insertions(+), 3 deletions(-)
>
> diff --git a/lisp/ob-comint.el b/lisp/ob-comint.el
> index d12ed47..21ff0f6 100644
> --- a/lisp/ob-comint.el
> +++ b/lisp/ob-comint.el
> @@ -93,9 +93,9 @@ or user `keyboard-quit' during execution of body."
>  			  (goto-char comint-last-input-end)
>  			  (not (save-excursion
>  				 (and (re-search-forward
> -				       comint-prompt-regexp nil t)
> +				       (regexp-quote ,eoe-indicator) nil t)
>  				      (re-search-forward
> -				       (regexp-quote ,eoe-indicator) nil t)))))
> +				       comint-prompt-regexp nil t)))))
>  		   (accept-process-output (get-buffer-process (current-buffer)))
>  		   ;; thought the following this would allow async
>  		   ;; background running, but I was wrong...
> diff --git a/lisp/ob-sh.el b/lisp/ob-sh.el
> index 10c08d4..b2d1591 100644
> --- a/lisp/ob-sh.el
> +++ b/lisp/ob-sh.el
> @@ -170,7 +170,13 @@ return the value of the last statement in BODY."
>  	    (session org-babel-sh-eoe-output t body)
>  	  (mapc
>  	   (lambda (line)
> -	     (insert line) (comint-send-input nil t) (sleep-for 0.25))
> +	     (insert line)
> +	     (comint-send-input nil t)
> +	     (while (save-excursion
> +		      (goto-char comint-last-input-end)
> +		      (not (re-search-forward
> +			    comint-prompt-regexp nil t)))
> +	       (accept-process-output (get-buffer-process (current-buffer)))))
>  	   (append
>  	    (split-string (org-babel-trim body) "\n")
>  	    (list org-babel-sh-eoe-indicator))))

-- 
Eric Schulte
http://cs.unm.edu/~eschulte/

Re: [babel] [PATCH] Fix sh block execution in session

[julien Barnier]

Hi Eric,

> 1. do you have a minimal example of a shell code block which works after
>    your patch but not before?

Well, in fact it seems that almost any shell block cause problems when
executed in a session. This one, for example :

#+begin_src sh :session test :results output
echo "foo"
#+end_src

> 2. I assume that the changes to ob-comit were required for your ob-sh
>    changes to work and were not intended to affect other languages?

Yes, I would have preferred no to touch to ob-comint, but it seems that I was
forced too.

The changes to ob-sh have only one aim : to be sure that the commands in the
shell block are send one at a time, and that we wait for the previous command
to be completed before sending the following one. The current org-sh code only
uses a "(sleep-for 0.25)" delay, which is not always sufficient.

The changes to ob-comint modify the way the end of activity indicator is
looking for. The current code is :

#+begin_src emacs-lisp
  (goto-char comint-last-input-end)
  (not (save-excursion
         (and (re-search-forward
               comint-prompt-regexp nil t)
              (re-search-forward
               (regexp-quote ,eoe-indicator) nil t))))
#+end_src

From my initial sh code block, the execution ended with this :

#+begin_example
julien@L018198:~/org/perso$ echo "foo"
foo
julien@L018198:~/org/perso$ echo 'org_babel_sh_eoe'
org_babel_sh_eoe
julien@L018198:~/org/perso$ 
#+end_example

With the current code, the "(goto-char comint-last-input-end)" would put the
point at the beginning of the "org_babel_sh_eoe" line. Then, the
"(re-search-forward comint-prompt-regexp nil t)" would put the point after the
$ on the last line. So the following "(re-search-forward (regexp-quote
,eoe-indicator) nil t)" will never find anything, as the eoe-indicator is now
behind the point.

That's why I feel that the two searches must be switched.

> 3. When switching the order of the prompt and eoe check in ob-comint,
>    did you try *only* looking for the eoe indicator?  It seems to me
>    that once that is present the check for a followup prompt may not be
>    required and it may make sense to simply remove that code.

It seems to work if you omit the prompt check (at least for shell blocks), but
this prompt check could be useful, as it ensures that the interpreter is ready
to get another command, and as it places the point at the right place for
that. But I'm not sure this check is necessary.

This change doesn't seem to affect evaluation on R source blocks. I've tried
to test it with ruby, but session evaluation seems broken for this language ?

#+begin_src R :results output :session rtest
y <- 2
for (i in 1:100000) i
y
#+end_src

> All in all I agree that the ob-sh changes look like an improvement and
> should be applied.  I'm less sure of the changes to ob-comint, simply
> because I don't understand the mechanism through which swapping the
> regexp checks changes behavior.  However if the ob-sh changes depend
> upon the ob-comint changes there is no point in applying them piecemeal.

Well, I can't assure you that the change in ob-comint won't affect any other
language, but I can assure you that the change in ob-sh is dependant from the
one in ob-comint.

Thanks a lot for your answer, and I'm available for any other question or
testing request.

Sincerely,

Julien

Re: [babel] [PATCH] Fix sh block execution in session

[Eric Schulte]

Hi julien,

julien Barnier <julien@nozav.org> writes:

> Hi Eric,
>
>> 1. do you have a minimal example of a shell code block which works after
>>    your patch but not before?
>
> Well, in fact it seems that almost any shell block cause problems when
> executed in a session. This one, for example :
>
> #+begin_src sh :session test :results output
> echo "foo"
> #+end_src
>

Ah, I see, thanks.

>
>> 2. I assume that the changes to ob-comit were required for your ob-sh
>>    changes to work and were not intended to affect other languages?
>
> Yes, I would have preferred no to touch to ob-comint, but it seems that I was
> forced too.
>
> The changes to ob-sh have only one aim : to be sure that the commands in the
> shell block are send one at a time, and that we wait for the previous command
> to be completed before sending the following one. The current org-sh code only
> uses a "(sleep-for 0.25)" delay, which is not always sufficient.
>
> The changes to ob-comint modify the way the end of activity indicator is
> looking for. The current code is :
>
> #+begin_src emacs-lisp
>   (goto-char comint-last-input-end)
>   (not (save-excursion
>          (and (re-search-forward
>                comint-prompt-regexp nil t)
>               (re-search-forward
>                (regexp-quote ,eoe-indicator) nil t))))
> #+end_src
>
> From my initial sh code block, the execution ended with this :
>
> #+begin_example
> julien@L018198:~/org/perso$ echo "foo"
> foo
> julien@L018198:~/org/perso$ echo 'org_babel_sh_eoe'
> org_babel_sh_eoe
> julien@L018198:~/org/perso$ 
> #+end_example
>
> With the current code, the "(goto-char comint-last-input-end)" would put the
> point at the beginning of the "org_babel_sh_eoe" line. Then, the
> "(re-search-forward comint-prompt-regexp nil t)" would put the point after the
> $ on the last line. So the following "(re-search-forward (regexp-quote
> ,eoe-indicator) nil t)" will never find anything, as the eoe-indicator is now
> behind the point.
>

I see, thanks for clearing this up.  Knowing the reason behind this
change I'm inclined to commit your patch (which I've just done).  If
anyone runs into problems hopefully we'll hear about them on list and
can handle them as they arise.

>
> That's why I feel that the two searches must be switched.
>
>> 3. When switching the order of the prompt and eoe check in ob-comint,
>>    did you try *only* looking for the eoe indicator?  It seems to me
>>    that once that is present the check for a followup prompt may not be
>>    required and it may make sense to simply remove that code.
>
> It seems to work if you omit the prompt check (at least for shell blocks), but
> this prompt check could be useful, as it ensures that the interpreter is ready
> to get another command, and as it places the point at the right place for
> that.

Moving the cursor forward does seem to be a worthwhile reason to retain
this prompt search.

> But I'm not sure this check is necessary.
>
> This change doesn't seem to affect evaluation on R source blocks. I've tried
> to test it with ruby,

I've tested this patch with ruby sessions and it does work.

> but session evaluation seems broken for this language ?
>

You are probably missing some of the requirements for interactive ruby
evaluation (inf-ruby or somesuch).

>
> 
> #+begin_src R :results output :session rtest
> y <- 2
> for (i in 1:100000) i
> y
> #+end_src
> 
>> All in all I agree that the ob-sh changes look like an improvement and
>> should be applied.  I'm less sure of the changes to ob-comint, simply
>> because I don't understand the mechanism through which swappineg the
>> regexp checks changes behavior.  However if the ob-sh changes depend
>> upon the ob-comint changes there is no point in applying them piecemeal.
> 
> Well, I can't assure you that the change in ob-comint won't affect any other
> language, but I can assure you that the change in ob-sh is dependant from the
> one in ob-comint.
> 
> Thanks a lot for your answer, and I'm available for any other question or
> testing request.
> 

Thanks for the patch Julien.  I've just pushed it up to the core (this
is the development version after all, so we're allowed to take risks).

Best -- Eric

> 
> Sincerely,
> 
> Julien

-- 
Eric Schulte
http://cs.unm.edu/~eschulte/