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

[Jack Kamm <jackkamm@gmail.com>]

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

Ihor Radchenko <yantar92@posteo.net> writes:

>> +           (t (error (format "Unrecognized prompt handling behavior %s"
>> +                             (symbol-name prompt-handling))))))
>
> I think that `symbol-name' is unnecessary here.
> It will lead to cryptic error if PROMPT-HANDLING happens to be something
> that is not symbol (for example, a number)
>
>> +*** ~org-babel-comint-async-register~: Added argument to specify prompt handling
>
> Maybe "New optional argument controlling prompt handling"
>
>> +The new argument ~prompt-handling~ allows Babel languages to specify
>> +how prompts should be handled when passing output to
>> +~org-babel-comint-async-chunk-callback~.  If equal to
>> +~filter-prompts~, prompts are removed beforehand, similar to the
>
> similar -> same (or did I miss something?)

Thanks for the suggestions, all of which I agree with. I've updated the
patches accordingly, and also rebased them to most recent versions of
bugfix/main.


[-- 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: 7525 bytes --]

From 16dfc0d4500ab941333a9e705b2ddac85c30efdc 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         | 34 ++++++++++++++++++++++++++++------
 lisp/ob-python.el         |  3 ++-
 testing/lisp/test-ob-R.el | 28 ++++++++++++++++++++++++++++
 4 files changed, 60 insertions(+), 8 deletions(-)

diff --git a/lisp/ob-R.el b/lisp/ob-R.el
index 8074496f8..e9a1ce768 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
+   'disable-prompt-filtering)
   (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..be5b41331 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,31 @@ (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 prompt-handling)
   "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.
+PROMPT-HANDLING may be either of the symbols `filter-prompts', in
+which case prompts matching `comint-prompt-regexp' are filtered
+from output before it is passed to CHUNK-CALLBACK, or
+`disable-prompt-filtering', in which case this behavior is
+disabled.  For backward-compatibility, the default value of `nil'
+is equivalent to `filter-prompts'."
   (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)
+    (setq org-babel-comint-async-remove-prompts-p
+          (cond
+           ((eq prompt-handling 'disable-prompt-filtering) nil)
+           ((eq prompt-handling 'filter-prompts) t)
+           ((eq prompt-handling nil) t)
+           (t (error (format "Unrecognized prompt handling behavior %s"
+                             prompt-handling)))))
     (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 f881918c7..9975e83be 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
+   'disable-prompt-filtering)
   (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.46.2


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0001-NEWS-entry-for-the-new-argument-of-org-babel-comint-.patch --]
[-- Type: text/x-patch, Size: 1875 bytes --]

From a4e66d0f66e04983d75d93ac84acf96d6e41d870 Mon Sep 17 00:00:00 2001
From: Jack Kamm <jackkamm@gmail.com>
Date: Mon, 21 Oct 2024 20:22:00 -0700
Subject: [PATCH] NEWS entry for the new argument of
 org-babel-comint-async-register

The optional argument was added on bugfix branch, but we are adding
this extra NEWS entry on main.  See also:

https://list.orgmode.org/875xpnrubg.fsf@localhost/T/#m179d313e1db284ff28eb4098129bb49418824a25
---
 etc/ORG-NEWS | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS
index 5d421172f..de4f11b25 100644
--- a/etc/ORG-NEWS
+++ b/etc/ORG-NEWS
@@ -186,6 +186,24 @@ accept the INFO channel and return a string.  This makes it possible
 to dynamically generate the content of the resulting ~<head>~ tag in
 the resulting HTML document.
 
+*** ~org-babel-comint-async-register~: New optional argument controlling prompt handling
+
+The new argument ~prompt-handling~ allows Babel languages to specify
+how prompts should be handled when passing output to
+~org-babel-comint-async-chunk-callback~.  If equal to
+~filter-prompts~, prompts are removed beforehand, same as the
+default behavior of ~org-babel-comint-with-output~.  If equal to
+~disable-prompt-filtering~, then the prompt filtering is skipped.  If
+unset, then the default behavior is the same as ~filter-prompts~ for
+backwards compatibility.
+
+Prompt filtering is needed for some Babel languages, such as ob-shell,
+which leave extra prompts in the output as a side effect of
+evaluation.  However other Babel languages, like ob-python, don't
+leave extra prompts after evaluation, and skipping the prompt
+filtering can be more robust for such languages (as this avoids
+removing false positive prompts).
+
 ** Miscellaneous
 *** Org mode no longer prevents =flyspell= from spell-checking inside =LOGBOOK= drawers
 
-- 
2.46.2