emacs-orgmode@gnu.org archives
 help / color / mirror / code / Atom feed
* [PATCH] async process in R
@ 2021-09-26 17:13 Jeremie Juste
  2021-09-26 18:33 ` Greg Minshall
                   ` (3 more replies)
  0 siblings, 4 replies; 19+ messages in thread
From: Jeremie Juste @ 2021-09-26 17:13 UTC (permalink / raw)
  To: Org Mode; +Cc: Jack Kamm, Berry, Charles

[-- 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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] async process in R
  2021-09-26 17:13 [PATCH] async process in R Jeremie Juste
@ 2021-09-26 18:33 ` Greg Minshall
  2021-09-26 19:52   ` Jeremie Juste
  2021-09-27  8:18 ` Bastien
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 19+ messages in thread
From: Greg Minshall @ 2021-09-26 18:33 UTC (permalink / raw)
  To: Jeremie Juste; +Cc: Jack Kamm, Org Mode, Berry, Charles

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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] async process in R
  2021-09-26 18:33 ` Greg Minshall
@ 2021-09-26 19:52   ` Jeremie Juste
  2021-09-27  4:04     ` Greg Minshall
  0 siblings, 1 reply; 19+ messages in thread
From: Jeremie Juste @ 2021-09-26 19:52 UTC (permalink / raw)
  To: Greg Minshall; +Cc: Jack Kamm, Org Mode, Berry, Charles

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





^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] async process in R
  2021-09-26 19:52   ` Jeremie Juste
@ 2021-09-27  4:04     ` Greg Minshall
  2021-09-27  6:48       ` Bastien
  0 siblings, 1 reply; 19+ messages in thread
From: Greg Minshall @ 2021-09-27  4:04 UTC (permalink / raw)
  To: Jeremie Juste; +Cc: Jack Kamm, Org Mode, Berry, Charles

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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] async process in R
  2021-09-27  4:04     ` Greg Minshall
@ 2021-09-27  6:48       ` Bastien
  2021-09-27 19:21         ` Jeremie Juste
  0 siblings, 1 reply; 19+ messages in thread
From: Bastien @ 2021-09-27  6:48 UTC (permalink / raw)
  To: Greg Minshall; +Cc: Jack Kamm, Org Mode, Berry, Charles

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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] async process in R
  2021-09-26 17:13 [PATCH] async process in R Jeremie Juste
  2021-09-26 18:33 ` Greg Minshall
@ 2021-09-27  8:18 ` Bastien
  2021-09-27 18:28 ` Berry, Charles via General discussions about Org-mode.
  2021-10-02 22:54 ` Jack Kamm
  3 siblings, 0 replies; 19+ messages in thread
From: Bastien @ 2021-09-27  8:18 UTC (permalink / raw)
  To: Jeremie Juste; +Cc: Jack Kamm, Org Mode, Berry, Charles

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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] async process in R
  2021-09-26 17:13 [PATCH] async process in R Jeremie Juste
  2021-09-26 18:33 ` Greg Minshall
  2021-09-27  8:18 ` Bastien
@ 2021-09-27 18:28 ` Berry, Charles via General discussions about Org-mode.
  2021-09-27 19:25   ` Jeremie Juste
  2021-09-27 20:28   ` Jeremie Juste
  2021-10-02 22:54 ` Jack Kamm
  3 siblings, 2 replies; 19+ messages in thread
From: Berry, Charles via General discussions about Org-mode. @ 2021-09-27 18:28 UTC (permalink / raw)
  To: Jeremie Juste; +Cc: Jack Kamm, 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




^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] async process in R
  2021-09-27  6:48       ` Bastien
@ 2021-09-27 19:21         ` Jeremie Juste
  2021-09-28  3:02           ` Jack Kamm
  0 siblings, 1 reply; 19+ messages in thread
From: Jeremie Juste @ 2021-09-27 19:21 UTC (permalink / raw)
  To: Bastien; +Cc: Greg Minshall, Jack Kamm, Org Mode, Berry, Charles

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










^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] async process in R
  2021-09-27 18:28 ` Berry, Charles via General discussions about Org-mode.
@ 2021-09-27 19:25   ` Jeremie Juste
  2021-09-27 20:28   ` Jeremie Juste
  1 sibling, 0 replies; 19+ messages in thread
From: Jeremie Juste @ 2021-09-27 19:25 UTC (permalink / raw)
  To: Berry, Charles; +Cc: Jack Kamm, Org Mode

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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] async process in R
  2021-09-27 18:28 ` Berry, Charles via General discussions about Org-mode.
  2021-09-27 19:25   ` Jeremie Juste
@ 2021-09-27 20:28   ` Jeremie Juste
  2021-09-27 22:56     ` Berry, Charles via General discussions about Org-mode.
  1 sibling, 1 reply; 19+ messages in thread
From: Jeremie Juste @ 2021-09-27 20:28 UTC (permalink / raw)
  To: Berry, Charles; +Cc: Jack Kamm, Org Mode

[-- 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

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] async process in R
  2021-09-27 20:28   ` Jeremie Juste
@ 2021-09-27 22:56     ` Berry, Charles via General discussions about Org-mode.
  2021-09-27 23:40       ` Berry, Charles via General discussions about Org-mode.
  0 siblings, 1 reply; 19+ messages in thread
From: Berry, Charles via General discussions about Org-mode. @ 2021-09-27 22:56 UTC (permalink / raw)
  To: Jeremie Juste; +Cc: Jack Kamm, 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




^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] async process in R
  2021-09-27 22:56     ` Berry, Charles via General discussions about Org-mode.
@ 2021-09-27 23:40       ` Berry, Charles via General discussions about Org-mode.
  2021-09-28  7:34         ` Jeremie Juste
  0 siblings, 1 reply; 19+ messages in thread
From: Berry, Charles via General discussions about Org-mode. @ 2021-09-27 23:40 UTC (permalink / raw)
  To: Jeremie Juste; +Cc: Jack Kamm, 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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] async process in R
  2021-09-27 19:21         ` Jeremie Juste
@ 2021-09-28  3:02           ` Jack Kamm
  0 siblings, 0 replies; 19+ messages in thread
From: Jack Kamm @ 2021-09-28  3:02 UTC (permalink / raw)
  To: Jeremie Juste, Bastien; +Cc: Greg Minshall, Berry, Charles, Org Mode

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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] async process in R
  2021-09-27 23:40       ` Berry, Charles via General discussions about Org-mode.
@ 2021-09-28  7:34         ` Jeremie Juste
  2021-09-28 18:22           ` Berry, Charles via General discussions about Org-mode.
  0 siblings, 1 reply; 19+ messages in thread
From: Jeremie Juste @ 2021-09-28  7:34 UTC (permalink / raw)
  To: Berry, Charles; +Cc: Jack Kamm, Org Mode

[-- 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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] async process in R
  2021-09-28  7:34         ` Jeremie Juste
@ 2021-09-28 18:22           ` Berry, Charles via General discussions about Org-mode.
  2021-09-28 20:40             ` Jeremie Juste
  0 siblings, 1 reply; 19+ messages in thread
From: Berry, Charles via General discussions about Org-mode. @ 2021-09-28 18:22 UTC (permalink / raw)
  To: Jeremie Juste; +Cc: Jack Kamm, 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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] async process in R
  2021-09-28 18:22           ` Berry, Charles via General discussions about Org-mode.
@ 2021-09-28 20:40             ` Jeremie Juste
  2021-10-02 22:57               ` Jack Kamm
  0 siblings, 1 reply; 19+ messages in thread
From: Jeremie Juste @ 2021-09-28 20:40 UTC (permalink / raw)
  To: Berry, Charles; +Cc: Jack Kamm, Org Mode

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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] async process in R
  2021-09-26 17:13 [PATCH] async process in R Jeremie Juste
                   ` (2 preceding siblings ...)
  2021-09-27 18:28 ` Berry, Charles via General discussions about Org-mode.
@ 2021-10-02 22:54 ` Jack Kamm
  2021-10-02 23:14   ` Jack Kamm
  3 siblings, 1 reply; 19+ messages in thread
From: Jack Kamm @ 2021-10-02 22:54 UTC (permalink / raw)
  To: Jeremie Juste, Org Mode; +Cc: Berry, Charles

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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] async process in R
  2021-09-28 20:40             ` Jeremie Juste
@ 2021-10-02 22:57               ` Jack Kamm
  0 siblings, 0 replies; 19+ messages in thread
From: Jack Kamm @ 2021-10-02 22:57 UTC (permalink / raw)
  To: Jeremie Juste, Berry, Charles; +Cc: Org Mode

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


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH] async process in R
  2021-10-02 22:54 ` Jack Kamm
@ 2021-10-02 23:14   ` Jack Kamm
  0 siblings, 0 replies; 19+ messages in thread
From: Jack Kamm @ 2021-10-02 23:14 UTC (permalink / raw)
  To: Jeremie Juste, Org Mode; +Cc: Berry, Charles

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


^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2021-10-02 23:15 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-09-26 17:13 [PATCH] async process in R Jeremie Juste
2021-09-26 18:33 ` Greg Minshall
2021-09-26 19:52   ` Jeremie Juste
2021-09-27  4:04     ` Greg Minshall
2021-09-27  6:48       ` Bastien
2021-09-27 19:21         ` Jeremie Juste
2021-09-28  3:02           ` Jack Kamm
2021-09-27  8:18 ` Bastien
2021-09-27 18:28 ` Berry, Charles via General discussions about Org-mode.
2021-09-27 19:25   ` Jeremie Juste
2021-09-27 20:28   ` Jeremie Juste
2021-09-27 22:56     ` Berry, Charles via General discussions about Org-mode.
2021-09-27 23:40       ` Berry, Charles via General discussions about Org-mode.
2021-09-28  7:34         ` Jeremie Juste
2021-09-28 18:22           ` Berry, Charles via General discussions about Org-mode.
2021-09-28 20:40             ` Jeremie Juste
2021-10-02 22:57               ` Jack Kamm
2021-10-02 22:54 ` Jack Kamm
2021-10-02 23:14   ` Jack Kamm

Code repositories for project(s) associated with this inbox:

	https://git.savannah.gnu.org/cgit/emacs/org-mode.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).