[PATCH] async process in R

[PATCH] async process in R

[Jeremie Juste]

[-- Attachment #1: Type: text/plain, Size: 916 bytes --]

Hello,

I have integrated some of the ob-session-async package in ob-R.el
(finally). Most of the heavy lifting has been made by Jack.


So with the patch async evaluation is  expected to work out of the box
for :result value

#+begin_src R :session *R*  :results value :async :colnames yes
Sys.sleep(10)
as.list(1:5)
#+end_src

#+RESULTS:
| x |
|---|
| 1 |
| 2 |
| 3 |
| 4 |
| 5 |

But for the time being result output produces the following output.

#+begin_src R :session *R*  :results output :async
Sys.sleep(1)
print(1:5)
#+end_src

#+RESULTS:
: > Sys.sleep(1)
: > print(1:5)
: [1] 1 2 3 4 5
: > 'ob_comint_async_R_end_53c0a42fed17cf78f5508e5293029430'


From my understanding the async command of python does not suffer from
this issue. I'm not sure if the issue needs to be solve at the ob-R.el
or at the ob-core.el. I welcome any comments on this patch or any idea
to move it forward.

Best regards,
Jeremie


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: async in ob-R --]
[-- Type: text/x-diff, Size: 10655 bytes --]

From 51e1efb75e15fab348fd5a2c8b5fb5c1dbbf4d1c Mon Sep 17 00:00:00 2001
From: Jeremie Juste <djj@debian-BULLSEYE-live-builder-AMD64>
Date: Sun, 26 Sep 2021 18:25:23 +0200
Subject: [PATCH] lisp/ob-R: Async evaluation in R

* lisp/ob-R.el `ob-session-async-R-indicator': Add constant
representing a prefix R to identity session.
(ob-session-async-org-babel-R-evaluate-session):New
function to evaluate R src block
asynchrously.  (ob-session-async-R-value-callback): New function that
callback the result of the asynchronous evaluation.
(org-babel-R-evaluate): Add `async' parameter and call
ob-session-async-org-babel-R-evaluate-session if `async' parameter is
present.  (org-babel-execute:R): Call (org-babel-comint-use-async) to
check if async is among `params' and add async parameter to (org-babel-R-evaluate).

* testing/lisp/test-ob-R.el: Add 7 more tests for async evaluations,
also taken from ob-session-async package.

This is almost a carbon copy of the package of ob-session-async
of Jack Kamm. The original source code can be found
https://github.com/jackkamm/ob-session-async

Please refer to the following thread to trace back the discussion on
async evaluation in R.
https://list.orgmode.org/87eed9g9p6.fsf@gmail.com/
---
 lisp/ob-R.el              | 90 +++++++++++++++++++++++++++++++++++--
 testing/lisp/test-ob-R.el | 94 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 181 insertions(+), 3 deletions(-)

diff --git a/lisp/ob-R.el b/lisp/ob-R.el
index 2d9073cee..299ccdf1d 100644
--- a/lisp/ob-R.el
+++ b/lisp/ob-R.el
@@ -158,6 +158,7 @@ This function is called by `org-babel-execute-src-block'."
   (save-excursion
     (let* ((result-params (cdr (assq :result-params params)))
 	   (result-type (cdr (assq :result-type params)))
+           (async (org-babel-comint-use-async params))
            (session (org-babel-R-initiate-session
 		     (cdr (assq :session params)) params))
 	   (graphics-file (and (member "graphics" (assq :result-params params))
@@ -184,7 +185,8 @@ This function is called by `org-babel-execute-src-block'."
 		  (cdr (assq :colname-names params)) colnames-p))
 	     (or (equal "yes" rownames-p)
 		 (org-babel-pick-name
-		  (cdr (assq :rowname-names params)) rownames-p)))))
+		  (cdr (assq :rowname-names params)) rownames-p))
+             async)))
       (if graphics-file nil result))))
 
 (defun org-babel-prep-session:R (session params)
@@ -371,11 +373,14 @@ Has four %s escapes to be filled in:
 4. The name of the file to write to")
 
 (defun org-babel-R-evaluate
-  (session body result-type result-params column-names-p row-names-p)
+  (session body result-type result-params column-names-p row-names-p async)
   "Evaluate R code in BODY."
   (if session
+      (if async
+          (ob-session-async-org-babel-R-evaluate-session
+           session body result-type result-params column-names-p row-names-p)
       (org-babel-R-evaluate-session
-       session body result-type result-params column-names-p row-names-p)
+       session body result-type result-params column-names-p row-names-p))
     (org-babel-R-evaluate-external-process
      body result-type result-params column-names-p row-names-p)))
 
@@ -468,6 +473,85 @@ Insert hline if column names in output have been requested."
 	(error "Could not parse R result"))
     result))
 
+
+;;; async evaluation
+
+(defconst ob-session-async-R-indicator "'ob_comint_async_R_%s_%s'")
+
+(defun ob-session-async-org-babel-R-evaluate-session
+    (session body result-type result-params column-names-p row-names-p)
+  "Asynchronously evaluate BODY in SESSION.
+Returns a placeholder string for insertion, to later be replaced
+by `org-babel-comint-async-filter'."
+  (org-babel-comint-async-register
+   session (current-buffer)
+   "^\\(?:[>.+] \\)*\\[1\\] \"ob_comint_async_R_\\(.+?\\)_\\(.+\\)\"$"
+   'org-babel-chomp
+   'ob-session-async-R-value-callback)
+  (cl-case result-type
+    (value
+     (let ((tmp-file (org-babel-temp-file "R-")))
+       (with-temp-buffer
+         (insert
+          (org-babel-chomp body))
+         (let ((ess-local-process-name
+                (process-name (get-buffer-process session))))
+           (ess-eval-buffer nil)))
+       (with-temp-buffer
+	 (insert
+	  (mapconcat
+           'org-babel-chomp
+           (list (format org-babel-R-write-object-command
+                         (if row-names-p "TRUE" "FALSE")
+                         (if column-names-p
+                             (if row-names-p "NA" "TRUE")
+                           "FALSE")
+                         ".Last.value"
+                         (org-babel-process-file-name tmp-file 'noquote))
+                 (format ob-session-async-R-indicator
+                         "file" tmp-file))
+           "\n"))
+	 (let ((ess-local-process-name
+		(process-name (get-buffer-process session))))
+	   (ess-eval-buffer nil)))
+       tmp-file))
+    (output
+     (let ((uuid (md5 (number-to-string (random 100000000))))
+           (ess-local-process-name
+            (process-name (get-buffer-process session))))
+       (with-temp-buffer
+         (insert (format ob-session-async-R-indicator
+					  "start" uuid))
+         (insert "\n")
+         (insert body)
+         (insert "\n")
+         (insert (format ob-session-async-R-indicator
+					  "end" uuid))
+         (ess-eval-buffer nil))
+       uuid))))
+
+(defun ob-session-async-R-value-callback (params tmp-file)
+  "Callback for async value results.
+Assigned locally to `ob-session-async-file-callback' in R
+comint buffers used for asynchronous Babel evaluation."
+  (let* ((graphics-file (and (member "graphics" (assq :result-params params))
+			     (org-babel-graphical-output-file params)))
+	 (colnames-p (unless graphics-file (cdr (assq :colnames params)))))
+    (org-babel-R-process-value-result
+     (org-babel-result-cond (assq :result-params params)
+       (with-temp-buffer
+         (insert-file-contents tmp-file)
+         (org-babel-chomp (buffer-string) "\n"))
+       (org-babel-import-elisp-from-file tmp-file '(16)))
+     (or (equal "yes" colnames-p)
+	 (org-babel-pick-name
+	  (cdr (assq :colname-names params)) colnames-p)))))
+
+
+
+;;; ob-session-async-R.el ends here
+
+
 (provide 'ob-R)
 
 ;;; ob-R.el ends here
diff --git a/testing/lisp/test-ob-R.el b/testing/lisp/test-ob-R.el
index c36bac9e6..1e60ae903 100644
--- a/testing/lisp/test-ob-R.el
+++ b/testing/lisp/test-ob-R.el
@@ -117,6 +117,8 @@ x
 ))))
 
 
+
+
 ;; (ert-deftest test-ob-r/output-with-error ()
 ;;   "make sure angle brackets are well formatted"
 ;;     (let (ess-ask-for-ess-directory ess-history-file)
@@ -151,6 +153,98 @@ log10(10)
 #+end_src"     
   (org-babel-execute-src-block))))))
 
+
+(ert-deftest ob-session-async-R-simple-session-async-value ()
+  (let (ess-ask-for-ess-directory
+        ess-history-file
+        (org-babel-temporary-directory "/tmp")
+        (org-confirm-babel-evaluate nil))
+    (org-test-with-temp-text
+     "#+begin_src R :session R :async yes\n  Sys.sleep(.1)\n  paste(\"Yep!\")\n#+end_src\n"
+     (should (let ((expected "Yep!"))
+	       (and (not (string= expected (org-babel-execute-src-block)))
+		    (string= expected
+			     (progn
+			       (sleep-for 0 200)
+			       (goto-char (org-babel-where-is-src-block-result))
+			       (org-babel-read-result)))))))))
+
+(ert-deftest ob-session-async-R-simple-session-async-output ()
+  (let (ess-ask-for-ess-directory
+        ess-history-file
+        (org-babel-temporary-directory "/tmp")
+        (org-confirm-babel-evaluate nil))
+    (org-test-with-temp-text
+     "#+begin_src R :session R :results output :async yes\n  Sys.sleep(.1)\n  1:5\n#+end_src\n"
+     (should (let ((expected "[1] 1 2 3 4 5"))
+	       (and (not (string= expected (org-babel-execute-src-block)))
+		    (string= expected
+			     (progn
+			       (sleep-for 0 200)
+			       (goto-char (org-babel-where-is-src-block-result))
+			       (org-babel-read-result)))))))))
+
+(ert-deftest ob-session-async-R-named-output ()
+  (let (ess-ask-for-ess-directory
+        ess-history-file
+        (org-babel-temporary-directory "/tmp")
+        org-confirm-babel-evaluate
+        (src-block "#+begin_src R :async :session R :results output\n  1:5\n#+end_src")
+        (results-before "\n\n#+NAME: foobar\n#+RESULTS:\n: [1] 1")
+        (results-after "\n\n#+NAME: foobar\n#+RESULTS:\n: [1] 1 2 3 4 5\n"))
+    (org-test-with-temp-text
+     (concat src-block results-before)
+     (should (progn (org-babel-execute-src-block)
+                    (sleep-for 0 200)
+                    (string= (concat src-block results-after)
+                             (buffer-string)))))))
+
+(ert-deftest ob-session-async-R-named-value ()
+  (let (ess-ask-for-ess-directory
+        ess-history-file
+        org-confirm-babel-evaluate
+        (org-babel-temporary-directory "/tmp")
+        (src-block "#+begin_src R :async :session R :results value\n  paste(\"Yep!\")\n#+end_src")
+        (results-before "\n\n#+NAME: foobar\n#+RESULTS:\n: [1] 1")
+        (results-after "\n\n#+NAME: foobar\n#+RESULTS:\n: Yep!\n"))
+    (org-test-with-temp-text
+     (concat src-block results-before)
+     (should (progn (org-babel-execute-src-block)
+                    (sleep-for 0 200)
+                    (string= (concat src-block results-after)
+                             (buffer-string)))))))
+
+(ert-deftest ob-session-async-R-output-drawer ()
+  (let (ess-ask-for-ess-directory
+        ess-history-file
+        org-confirm-babel-evaluate
+        (org-babel-temporary-directory "/tmp")
+        (src-block "#+begin_src R :async :session R :results output drawer\n  1:5\n#+end_src")
+        (result "\n\n#+RESULTS:\n:results:\n[1] 1 2 3 4 5\n:end:\n"))
+    (org-test-with-temp-text
+     src-block
+     (should (progn (org-babel-execute-src-block)
+                    (sleep-for 0 200)
+                    (string= (concat src-block result)
+                             (buffer-string)))))))
+
+(ert-deftest ob-session-async-R-value-drawer ()
+  (let (ess-ask-for-ess-directory
+        ess-history-file
+        org-confirm-babel-evaluate
+        (org-babel-temporary-directory "/tmp")
+        (src-block "#+begin_src R :async :session R :results value drawer\n  1:3\n#+end_src")
+        (result "\n\n#+RESULTS:\n:results:\n1\n2\n3\n:end:\n"))
+    (org-test-with-temp-text
+     src-block
+     (should (progn (org-babel-execute-src-block)
+                    (sleep-for 0 200)
+                    (string= (concat src-block result)
+                             (buffer-string)))))))
+
+
+
+
 (provide 'test-ob-R)
 
 ;;; test-ob-R.el ends here
-- 
2.30.2

Re: [PATCH] async process in R

[Greg Minshall]

Jeremie,

a question.  in

> #+begin_src R :session *R* :results value :async :colnames yes
> Sys.sleep(10)
> as.list(1:5)
> #+end_src

i was surprised by =:async= standing alone, i.e., with no following
"yes" or "no".  is that an org-mode "idiom"?  i.e., unadorned header
arguments default to (some form of) "yes"?

cheers, Greg

Re: [PATCH] async process in R

[Jeremie Juste]

Hello Greg,
>
> i was surprised by =:async= standing alone, i.e., with no following
> "yes" or "no".  is that an org-mode "idiom"?  i.e., unadorned header
> arguments default to (some form of) "yes"?

Many thanks for the feedback, assigning yes or no to async will work as expected.

#+begin_src R :session *R* :results value :async yes :colnames yes
Sys.sleep(10)
as.list(1:5)
#+end_src

No async process in R

#+begin_src R :session *R* :results value :async no :colnames yes
Sys.sleep(10)
as.list(1:5)
#+end_src

:async without parameters will default to yes.

#+begin_src R :session *R* :results value :async :colnames yes
Sys.sleep(10)
as.list(1:5)
#+end_src

I am not sure if it is a desirable feature or not. 

Best regards,
Jeremie

Re: [PATCH] async process in R

[Greg Minshall]

hi, Jeremie,

> Many thanks for the feedback, assigning yes or no to async will work
> as expected.

thanks for the answer.

> I am not sure if it is a desirable feature or not.

if this is not already idiomatic for org mode, i'd vote to require the
"yes" or "no".  just my 2 cents.

cheers, Greg

Re: [PATCH] async process in R

[Bastien]

Hi Greg and Jeremie,

Greg Minshall <minshall@umich.edu> writes:

> if this is not already idiomatic for org mode, i'd vote to require the
> "yes" or "no".  just my 2 cents.

Agreed: even if a syntax is allowed, let's use the idiomatic form in
examples.

2 cts,

-- 
 Bastien

Re: [PATCH] async process in R

[Bastien]

Hi Jeremie,

Jeremie Juste <jeremiejuste@gmail.com> writes:

> I have integrated some of the ob-session-async package in ob-R.el
> (finally). Most of the heavy lifting has been made by Jack.

When this is reliable enough, feel free to commit and push.

You can also enhance it later on.

Thanks,

-- 
 Bastien

Re: [PATCH] async process in R

[Berry, Charles via General discussions about Org-mode.]

Jeremie,

> On Sep 26, 2021, at 10:13 AM, Jeremie Juste <jeremiejuste@gmail.com> wrote:
> 
> But for the time being result output produces the following output.
> 
> #+begin_src R :session *R*  :results output :async
> Sys.sleep(1)
> print(1:5)
> #+end_src
> 
> #+RESULTS:
> : > Sys.sleep(1)
> : > print(1:5)
> : [1] 1 2 3 4 5
> : > 'ob_comint_async_R_end_53c0a42fed17cf78f5508e5293029430'
> 
> 
> From my understanding the async command of python does not suffer from
> this issue. I'm not sure if the issue needs to be solve at the ob-R.el
> or at the ob-core.el. I welcome any comments on this patch or any idea
> to move it forward.


It looks like you have `(setq ess-eval-visibly t)' here. I think that is a default setting.

With (setq ess-eval-visibly nil), I get 

#+RESULTS:
: > 
: [1] 1 2 3 4 5
: > >


which is better, but the prompts still need cleaning along the lines of `org-babel-R-evaluate-session'.

It seems like this is an ob-R.el issue.

HTH,
Chuck

Re: [PATCH] async process in R

[Jeremie Juste]

Hello,

On Monday, 27 Sep 2021 at 08:48, Bastien wrote:
> Hi Greg and Jeremie,
>
> Greg Minshall <minshall@umich.edu> writes:
>
>> if this is not already idiomatic for org mode, i'd vote to require the
>> "yes" or "no".  just my 2 cents.
>
> Agreed: even if a syntax is allowed, let's use the idiomatic form in
> examples.

Many thanks for the feedback, I'll keep that in mind and use the idomatic
form in examples.

For the parameter  :async without any values assigned to it. I'm
coordinating with ob-python.el and the orginal package
https://github.com/jackkamm/ob-session-async.

Jack do you see the need to change it and expect :async to have the
value yes or no?

Best regards,
Jeremie

Re: [PATCH] async process in R

[Jeremie Juste]

Hello Chuck,

On Monday, 27 Sep 2021 at 18:28, Berry, Charles wrote:
>
> It looks like you have `(setq ess-eval-visibly t)' here. I think that is a default setting.
>

Many thanks for the feedback.
>
> which is better, but the prompts still need cleaning along the lines of `org-babel-R-evaluate-session'.
>
> It seems like this is an ob-R.el issue.

I'll look deeper into ob-R.el

Best regards,
Jeremie

Re: [PATCH] async process in R

[Jeremie Juste]

[-- Attachment #1: Type: text/plain, Size: 707 bytes --]

Hello Chuck,

On Monday, 27 Sep 2021 at 18:28, Berry, Charles wrote:
>
> It looks like you have `(setq ess-eval-visibly t)' here. I think that is a default setting.

Thanks again for your suggestion. The following patch to be applied on
top of the previous one, solves the issue.


#+begin_src R :session *R*  :results output :async yes
Sys.sleep(5)
1:5
#+end_src

#+RESULTS:
: [1] 1 2 3 4 5


The function (ess-eval-buffer), when the parameter VIS is nil,  misled me in the sense that it inverses
the value of ess-eval-visibly.

Note as well that all the tests in test-ob-R.el passed including the
tests for async evaluation from [1] ob-session-async.

[1]: https://github.com/jackkamm/ob-session-async/.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch-2 --]
[-- Type: text/x-diff, Size: 1077 bytes --]

From 795cc0ebe637aa4ff148c495cf5403ba2baec242 Mon Sep 17 00:00:00 2001
From: Jeremie Juste <djj@debian-BULLSEYE-live-builder-AMD64>
Date: Mon, 27 Sep 2021 22:02:17 +0200
Subject: [PATCH] ob-R.el: Patch async evaluation when :results output

* lisp/ob-R.el (ob-session-async-org-babel-R-evaluate-session): Make
sure that 'ess-eval-visibly' is nil before evaluating the temporary
buffer, but return ess-eval-visibly to it's original state afterwards.
---
 lisp/ob-R.el | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/lisp/ob-R.el b/lisp/ob-R.el
index 299ccdf1d..188b9ac8f 100644
--- a/lisp/ob-R.el
+++ b/lisp/ob-R.el
@@ -526,8 +526,11 @@ by `org-babel-comint-async-filter'."
          (insert body)
          (insert "\n")
          (insert (format ob-session-async-R-indicator
-					  "end" uuid))
+			 "end" uuid))
+         (setq tmp ess-eval-visibly)
+         (setq ess-eval-visibly nil)
          (ess-eval-buffer nil))
+         (setq ess-eval-visibly tmp)
        uuid))))
 
 (defun ob-session-async-R-value-callback (params tmp-file)
-- 
2.30.2


[-- Attachment #3: Type: text/plain, Size: 70 bytes --]


Thanks again to Jack for this useful feature.

Best regards,
Jeremie

Re: [PATCH] async process in R

[Berry, Charles via General discussions about Org-mode.]

Jeremie,


There is something in my init that doesn't play nice with this.  

IOW, emacs -q and then load the minimal stuff works OK, but my usual startup does not.

#+begin_src R :session *R*  :results output :async yes
Sys.sleep(5)
1:5
#+end_src

#+RESULTS:
: > > 
: [1] 1 2 3 4 5
: >

It may take me a while to figure out what it is.

:-(

HTH,

Chuck


> On Sep 27, 2021, at 1:28 PM, Jeremie Juste <jeremiejuste@gmail.com> wrote:
> 
> Hello Chuck,
> 
> On Monday, 27 Sep 2021 at 18:28, Berry, Charles wrote:
>> 
>> It looks like you have `(setq ess-eval-visibly t)' here. I think that is a default setting.
> 
> Thanks again for your suggestion. The following patch to be applied on
> top of the previous one, solves the issue.
> 
> 
> #+begin_src R :session *R*  :results output :async yes
> Sys.sleep(5)
> 1:5
> #+end_src
> 
> #+RESULTS:
> : [1] 1 2 3 4 5
> 
> 
> The function (ess-eval-buffer), when the parameter VIS is nil,  misled me in the sense that it inverses
> the value of ess-eval-visibly.
> 
> Note as well that all the tests in test-ob-R.el passed including the
> tests for async evaluation from [1] ob-session-async.
> 
> [1]: https://urldefense.com/v3/__https://github.com/jackkamm/ob-session-async/__;!!LLK065n_VXAQ!wfTTIQ1fHIH2f6FmnUYZow4BoKA9_bQyOgBdm4tgLfJxbDF0vVs4sTbSQ0Zm-7YjVg$ .
> 
> <0001-ob-R.el-Patch-async-evaluation-when-results-output.patch>
> Thanks again to Jack for this useful feature.
> 
> Best regards,
> Jeremie

Re: [PATCH] async process in R

[Berry, Charles via General discussions about Org-mode.]

Jeremie,

> On Sep 27, 2021, at 3:56 PM, Berry, Charles <ccberry@health.ucsd.edu> wrote:
> 
> There is something in my init that doesn't play nice with this.  



(setq ess-inject-source nil)


seems to be the culprit.


Also note, even with ess-inject-source set to t, there is an indentation issue:

#+begin_src R :session *R*  :results output :async yes
Sys.sleep(2)
1:5

10:20
#+end_src

#+RESULTS:
: [1] 1 2 3 4 5
:  [1] 10 11 12 13 14 15 16 17 18 19 20



HTH,

Chuck

Re: [PATCH] async process in R

[Jack Kamm]

Hi Jeremie,

> For the parameter  :async without any values assigned to it. I'm
> coordinating with ob-python.el and the orginal package
> https://github.com/jackkamm/ob-session-async.
>
> Jack do you see the need to change it and expect :async to have the
> value yes or no?

I wrote the :async header to use this behavior, following conventions
from other (external) org packages with the :async header.

In particular, below are the packages I know of with ":async" -- I
believe most of them allow using ":async" instead of ":async yes", but
it's been awhile since I've checked, so I may be misremembering:

- ob-async
- ob-ipython
- ob-jupyter
- ob-ein

I can see why requiring an explicit ":async yes" might be preferable
though, and wouldn't oppose the change if there's a consensus for it.

Jack

Re: [PATCH] async process in R

[Jeremie Juste]

[-- Attachment #1: Type: text/plain, Size: 946 bytes --]

Hello Chuck,

On Monday, 27 Sep 2021 at 23:40, Berry, Charles wrote:
> Jeremie,
>
>> On Sep 27, 2021, at 3:56 PM, Berry, Charles <ccberry@health.ucsd.edu> wrote:
>> 
>> There is something in my init that doesn't play nice with this.  
>
> (setq ess-inject-source nil)

Thanks for the feedback. With the following patch, I made sure that
ess-inject-source is set to default before evaluating the buffer.

So even if I set
(setq ess-inject-source 'function-and-buffer), I get the following
output. Note that I get the same output in the IESS console buffer when
I execute the command following command.

#+begin_src R :session *R*  :results output :async yes
  Sys.sleep(2)
  1:5

  10:20
  1:2
#+end_src

#+RESULTS:
: [1] 1 2 3 4 5
:  [1] 10 11 12 13 14 15 16 17 18 19 20
: [1] 1 2

It might be good to fix this on the ESS side. I'll see what can be
done, but I'd appreciate any input you might have on this. Thanks again.

Best regards,
Jeremie


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: patch3 --]
[-- Type: text/x-diff, Size: 1258 bytes --]

From db2ad631247a5c52d9d6f6779948f6d0cf34c698 Mon Sep 17 00:00:00 2001
From: Jeremie Juste <djj@debian-BULLSEYE-live-builder-AMD64>
Date: Tue, 28 Sep 2021 09:04:25 +0200
Subject: [PATCH] ob-R.el: Patch async evaluation when :results output

* lisp/ob-R.el (ob-session-async-org-babel-R-evaluate-session): Make
sure that `ess-inject-source' is set to the default
'function-and-buffer before running (ess-eval-buffer). Return
`ess-inject-source' to its user-specified state afterwards.
---
 lisp/ob-R.el | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/lisp/ob-R.el b/lisp/ob-R.el
index 188b9ac8f..7e050c094 100644
--- a/lisp/ob-R.el
+++ b/lisp/ob-R.el
@@ -528,9 +528,13 @@ by `org-babel-comint-async-filter'."
          (insert (format ob-session-async-R-indicator
 			 "end" uuid))
          (setq tmp ess-eval-visibly)
+         (setq user-inject-src-param ess-inject-source)
          (setq ess-eval-visibly nil)
+         (setq  ess-inject-source 'function-and-buffer)
          (ess-eval-buffer nil))
-         (setq ess-eval-visibly tmp)
+       (setq ess-eval-visibly tmp)
+       (setq ess-inject-source user-inject-src-param)
+       
        uuid))))
 
 (defun ob-session-async-R-value-callback (params tmp-file)
-- 
2.30.2

Re: [PATCH] async process in R

[Berry, Charles via General discussions about Org-mode.]

Jeremie,

> On Sep 28, 2021, at 12:34 AM, Jeremie Juste <jeremiejuste@gmail.com> wrote:
> 
> Thanks for the feedback. With the following patch, I made sure that
> ess-inject-source is set to default before evaluating the buffer.
> 
> So even if I set
> (setq ess-inject-source 'function-and-buffer), I get the following
> output. Note that I get the same output in the IESS console buffer when
> I execute the command following command.
> 
> #+begin_src R :session *R*  :results output :async yes
>  Sys.sleep(2)
>  1:5
> 
>  10:20
>  1:2
> #+end_src


OK. The patch works when applied on top of the previous 2 (but the second one has the same name, so there is that to watch out for).

However, I think we are not quite home free. With `:async no', this works as expected:

#+begin_src R :session *R*  :results output :async no
  Sys.sleep(2)
  1:5
  10:
    a
  1:2
#+end_src

#+RESULTS:
: 
: [1] 1 2 3 4 5
: 
: Error: object 'a' not found
: 
: [1] 1 2


But changing to `:async yes', the error aborts in a way that omits the output.

HTH,
Chuck

Re: [PATCH] async process in R

[Jeremie Juste]

Hello Chuck,

> OK. The patch works when applied on top of the previous 2 (but the second one has the same name, so there is that to watch out for).
Thanks for the feedback, I'll make sure to provide unique names for
patches in the future.
>
> However, I think we are not quite home free. With `:async no', this works as expected:
>
> #+begin_src R :session *R*  :results output :async no
>   Sys.sleep(2)
>   1:5
>   10:
>     a
>   1:2
> #+end_src
>
> #+RESULTS:
> : 
> : [1] 1 2 3 4 5
> : 
> : Error: object 'a' not found
> : 
> : [1] 1 2
>
> But changing to `:async yes', the error aborts in a way that omits the output.

Interesting, I haven't thought about errors cases enough. Async process
will be on the 9.5 release and this issue will be next on the todo list.
Many thanks again for the feedback.


It does help,

Jeremie

Re: [PATCH] async process in R

[Jack Kamm]

Hi Jeremie,

Many thanks for bringing this over the finish line! I'm very glad it
made it into Org 9.5.

All the tests passed on my end, and and I successfully ran a few async
R blocks without any issues.

I do have some suggestions for code style below. They apply to both the
original patch, as well as followup fixes that have been committed
since.

> +(defconst ob-session-async-R-indicator "'ob_comint_async_R_%s_%s'")
> +
> +(defun ob-session-async-org-babel-R-evaluate-session

For consistency with the rest of ob-R.el, as well as with the async
functions in ob-python, I suggest using a prefix of
"org-babel-R-async-". So
"ob-session-async-org-babel-R-evaluate-session" would become
"org-babel-R-async-evaluate-session", etc.

> +(ert-deftest ob-session-async-R-simple-session-async-value ()

Again, for consistency I suggest renaming the test functions so they are
prefixed with "test-ob-R/". For example,
"ob-session-async-R-simple-session-async-value" could become
"test-ob-R/async-session-simple-value". This would also allow easily
running all the R tests using "BTEST_RE='.*ob-R.*' make test".

> +         (setq user-inject-src-param ess-inject-source)
>           (setq ess-eval-visibly nil)
> +         (setq  ess-inject-source 'function-and-buffer)
>           (ess-eval-buffer nil))
> -         (setq ess-eval-visibly tmp)
> +       (setq ess-eval-visibly tmp)
> +       (setq ess-inject-source user-inject-src-param)

Rather than using setq, it would be better to let bind these like so:

(let ((ess-eval-visibly nil)
      (ess-inject-source 'function-and-buffer))
   ...code in here...
)

This temporarily sets the variables within the let-block, then resets
them to their original values afterwards. Then you can also remove the
temporary variables "user-inject-src-param" and "ess-eval-visibly-tmp"
as well as their defvars at the top of the file.

Actually, since the code is already in a let-block, you can simply add
these variables to the existing let-statement.

> +           (list (format org-babel-R-write-object-command
> +                         (if row-names-p "TRUE" "FALSE")
> +                         (if column-names-p
> +                             (if row-names-p "NA" "TRUE")
> +                           "FALSE")
> +                         ".Last.value"
> +                         (org-babel-process-file-name tmp-file 'noquote))

Some parts of ob-session-async-org-babel-R-evaluate-session, such as
the above, are duplicated from org-babel-R-evaluate-session; it would
be good to reduce duplication by abstracting these out to separate
functions.

Thanks again for porting this, and for taking care of ob-R.el in
general.

All the best,
Jack

Re: [PATCH] async process in R

[Jack Kamm]

Hi Jeremie and Chuck,

>> But changing to `:async yes', the error aborts in a way that omits the output.
>
> Interesting, I haven't thought about errors cases enough. Async process
> will be on the 9.5 release and this issue will be next on the todo list.
> Many thanks again for the feedback.

I agree async eval should handle error outputs better. Errors are also
not handled well in ob-python's async eval at the moment. Certainly
something to improve in the future.

Best,
Jack

Re: [PATCH] async process in R

[Jack Kamm]

I just noticed one more thing, regarding ess-eval-visibly etc:

>> +         (setq user-inject-src-param ess-inject-source)
>>           (setq ess-eval-visibly nil)
>> +         (setq  ess-inject-source 'function-and-buffer)
>>           (ess-eval-buffer nil))
>> -         (setq ess-eval-visibly tmp)
>> +       (setq ess-eval-visibly tmp)
>> +       (setq ess-inject-source user-inject-src-param)
>
> Rather than using setq, it would be better to let bind these like so:
>
> (let ((ess-eval-visibly nil)
>       (ess-inject-source 'function-and-buffer))
>    ...code in here...
> )

I noticed these variables are only reset to defaults when ":results
output". It may also be necessary to set them as well for the case of
":results value". In my original implementation [1], I set
"ess-eval-visibly" within the wrapping advice function, so it applied to
all cases.

[1] https://github.com/jackkamm/ob-session-async/blob/master/lisp/ob-session-async-R.el