Re: [PATCH] Async sessions: Fix prompt removal regression in ob-R

[Jack Kamm <jackkamm@gmail.com>]

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

Ihor Radchenko <yantar92@posteo.net> writes:

> Maybe we can simply override `comint-prompt-regexp' as we do in
> ob-shell? The default regexp seems to be too permissive.

I don't think this is a good idea, since this is a deliberate choice by
ESS, which contains explicit commentary that the regexp should not
contain BOL, because in some cases multiple prompts can end up on the
same line [1][2].

After some more thought, rather than overriding `comint-prompt-regexp',
I think it would be better to provide a mechanism to altogether prevent
the prompt removal in `org-babel-comint-async-filter'. There is no need
to remove prompts from Python and R async session evaluation, since I
wrote them in a way that avoids leaking any prompts in the output. And
in both cases, it is easy to come up with examples that work fine in Org
9.6 but get mangled in Org 9.7.

Therefore, I've attached an updated patch that provides such a mechanism
for ob-R and ob-python, reverting them to the Org 9.6 behavior. This is
done through a variable `org-babel-comint-async-remove-prompts-p', which
is set by an optional argument in `org-babel-comint-async-register'.

More generally, I think it is best to avoid doing the prompt removal
when possible, since it is difficult (impossible?) to do it perfectly,
and it can cause many problems. This is why ob-python avoids using
`org-babel-comint-with-output' -- it sources a tmp file rather than
inputting code directly to comint, so that prompts do not leak. I think
non-async R evaluation would benefit from a similar approach, and plan
to propose a patch to make ob-R non-async eval more similar to
ob-python.

[1] https://github.com/emacs-ess/ESS/blob/d60c13a6a347ea7a91ea3408bb464cff0ab4fef6/lisp/ess-r-mode.el#L2538
[2] https://github.com/emacs-ess/ESS/blob/d60c13a6a347ea7a91ea3408bb464cff0ab4fef6/lisp/ess-custom.el#L1829


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Disable-async-prompt-removal-in-ob-R-python.patch --]
[-- Type: text/x-patch, Size: 7059 bytes --]

From 5c9d6f28f14c51fc542c997ed6aa6792e59857c6 Mon Sep 17 00:00:00 2001
From: Jack Kamm <jackkamm@gmail.com>
Date: Sun, 22 Sep 2024 13:48:45 -0700
Subject: [PATCH] Disable async prompt removal in ob-R,python

* lisp/ob-comint.el (org-babel-comint-async-remove-prompts-p): New
variable to disable prompt removal in async output.
(org-babel-comint-async-filter): Check
`org-babel-comint-async-remove-prompts-p' before calling
`org-babel-comint--prompt-filter'.
(org-babel-comint-async-register): Added argument for whether prompts
should be removed from async output.
* lisp/ob-python.el (org-babel-python-async-evaluate-session): Set
option to inhibit prompt removal when registering async evaluators.
* lisp/ob-R.el (ob-session-async-org-babel-R-evaluate-session): Set
option to inhibit prompt removal when registering async evaluators.
* testing/lisp/test-ob-R.el (test-ob-R/async-prompt-filter): Test for
over-aggressive prompt removal.
---
 lisp/ob-R.el              |  3 ++-
 lisp/ob-comint.el         | 27 ++++++++++++++++++++-------
 lisp/ob-python.el         |  3 ++-
 testing/lisp/test-ob-R.el | 28 ++++++++++++++++++++++++++++
 4 files changed, 52 insertions(+), 9 deletions(-)

diff --git a/lisp/ob-R.el b/lisp/ob-R.el
index de2d27a9a..08d8227f0 100644
--- a/lisp/ob-R.el
+++ b/lisp/ob-R.el
@@ -486,7 +486,8 @@ (defun ob-session-async-org-babel-R-evaluate-session
    session (current-buffer)
    "^\\(?:[>.+] \\)*\\[1\\] \"ob_comint_async_R_\\(start\\|end\\|file\\)_\\(.+\\)\"$"
    'org-babel-chomp
-   'ob-session-async-R-value-callback)
+   'ob-session-async-R-value-callback
+   t)
   (cl-case result-type
     (value
      (let ((tmp-file (org-babel-temp-file "R-")))
diff --git a/lisp/ob-comint.el b/lisp/ob-comint.el
index 764927af7..f37aa5264 100644
--- a/lisp/ob-comint.el
+++ b/lisp/ob-comint.el
@@ -239,6 +239,9 @@ (defvar-local org-babel-comint-async-chunk-callback nil
 comint process.  It should return a string that will be passed
 to `org-babel-insert-result'.")
 
+(defvar-local org-babel-comint-async-remove-prompts-p t
+  "Whether prompts should be detected and removed from async output.")
+
 (defvar-local org-babel-comint-async-dangling nil
   "Dangling piece of the last process output, as a string.
 Used when `org-babel-comint-async-indicator' is spread across multiple
@@ -326,10 +329,16 @@ (defun org-babel-comint-async-filter (string)
 		      until (and (equal (match-string 1) "start")
 				 (equal (match-string 2) uuid))
 		      finally return (+ 1 (match-end 0)))))
-                   ;; Remove prompt
-                   (res-promptless (org-trim (string-join (mapcar #'org-trim (org-babel-comint--prompt-filter res-str-raw)) "\n") "\n"))
 		   ;; Apply user callback
-		   (res-str (funcall org-babel-comint-async-chunk-callback res-promptless)))
+		   (res-str (funcall org-babel-comint-async-chunk-callback
+                                     (if org-babel-comint-async-remove-prompts-p
+                                         (org-trim (string-join
+                                                    (mapcar #'org-trim
+                                                            (org-babel-comint--prompt-filter
+                                                             res-str-raw))
+                                                    "\n")
+                                                   t)
+                                       res-str-raw))))
 	      ;; Search for uuid in associated org-buffers to insert results
 	      (cl-loop for buf in org-buffers
 		       until (with-current-buffer buf
@@ -350,18 +359,22 @@ (defun org-babel-comint-async-filter (string)
 
 (defun org-babel-comint-async-register
     (session-buffer org-buffer indicator-regexp
-		    chunk-callback file-callback)
+		    chunk-callback file-callback
+                    &optional inhibit-prompt-removal)
   "Set local org-babel-comint-async variables in SESSION-BUFFER.
 ORG-BUFFER is added to `org-babel-comint-async-buffers' if not
 present.  `org-babel-comint-async-indicator',
 `org-babel-comint-async-chunk-callback', and
 `org-babel-comint-async-file-callback' are set to
-INDICATOR-REGEXP, CHUNK-CALLBACK, and FILE-CALLBACK
-respectively."
+INDICATOR-REGEXP, CHUNK-CALLBACK, and FILE-CALLBACK respectively.
+If INHIBIT-PROMPT-REMOVAL,
+`org-babel-comint-async-remove-prompts-p' is set to `nil' to
+prevent prompt detection and removal from async output."
   (org-babel-comint-in-buffer session-buffer
     (setq org-babel-comint-async-indicator indicator-regexp
 	  org-babel-comint-async-chunk-callback chunk-callback
-	  org-babel-comint-async-file-callback file-callback)
+	  org-babel-comint-async-file-callback file-callback
+          org-babel-comint-async-remove-prompts-p (not inhibit-prompt-removal))
     (unless (memq org-buffer org-babel-comint-async-buffers)
       (setq org-babel-comint-async-buffers
 	    (cons org-buffer org-babel-comint-async-buffers)))
diff --git a/lisp/ob-python.el b/lisp/ob-python.el
index 8a3c24f70..38ebe9147 100644
--- a/lisp/ob-python.el
+++ b/lisp/ob-python.el
@@ -538,7 +538,8 @@ (defun org-babel-python-async-evaluate-session
   (org-babel-comint-async-register
    session (current-buffer)
    "ob_comint_async_python_\\(start\\|end\\|file\\)_\\(.+\\)"
-   'org-babel-chomp 'org-babel-python-async-value-callback)
+   'org-babel-chomp 'org-babel-python-async-value-callback
+   t)
   (pcase result-type
     (`output
      (let ((uuid (org-id-uuid)))
diff --git a/testing/lisp/test-ob-R.el b/testing/lisp/test-ob-R.el
index 9ffbf3afd..05b91afd6 100644
--- a/testing/lisp/test-ob-R.el
+++ b/testing/lisp/test-ob-R.el
@@ -316,6 +316,34 @@ (org-test-with-temp-text-in-file
             (string= (concat text result)
                      (buffer-string)))))))
 
+(ert-deftest test-ob-R/async-prompt-filter ()
+  "Test that async evaluation doesn't remove spurious prompts and leading indentation."
+  (let* (ess-ask-for-ess-directory
+         ess-history-file
+         org-confirm-babel-evaluate
+         (session-name "*R:test-ob-R/session-async-results*")
+         (kill-buffer-query-functions nil)
+         (start-time (current-time))
+         (wait-time (time-add start-time 3))
+         uuid-placeholder)
+    (org-test-with-temp-text
+     (concat "#+begin_src R :session " session-name " :async t :results output
+table(c('ab','ab','c',NA,NA), useNA='always')
+#+end_src")
+     (setq uuid-placeholder (org-trim (org-babel-execute-src-block)))
+     (catch 'too-long
+       (while (string-match uuid-placeholder (buffer-string))
+         (progn
+           (sleep-for 0.01)
+           (when (time-less-p wait-time (current-time))
+             (throw 'too-long (ert-fail "Took too long to get result from callback"))))))
+     (search-forward "#+results")
+     (beginning-of-line 2)
+     (when (should (re-search-forward "\
+:\\([ ]+ab\\)[ ]+c[ ]+<NA>[ ]*
+:\\([ ]+2\\)[ ]+1[ ]+2"))
+       (should (equal (length (match-string 1)) (length (match-string 2))))
+       (kill-buffer session-name)))))
 
 (provide 'test-ob-R)
 
-- 
2.47.0