* [PATCH] Async sessions: Fix prompt removal regression in ob-R
@ 2024-09-22 21:45 Jack Kamm
2024-10-02 17:05 ` Ihor Radchenko
0 siblings, 1 reply; 3+ messages in thread
From: Jack Kamm @ 2024-09-22 21:45 UTC (permalink / raw)
To: emacs-orgmode; +Cc: yantar92, matt, jeremiejuste
[-- Attachment #1: Type: text/plain, Size: 1520 bytes --]
Consider the following R block, which prints the occurrences of each
element in a list, including NAs:
#+begin_src R :session :results output :async
table(c("ab","ab","c",NA,NA), useNA='always')
#+end_src
#+RESULTS:
: ab c <NA>
: 2 1 2
Since Org 9.7, it instead prints:
#+RESULTS:
: ab c <
: 2 1 2
The regression happens in commit:
e9c288dfaccc2960e5b6889e6aabea700ad4e05a
which made the prompt filtering more consistent between
`org-babel-comint-with-output' and `org-babel-comint-async-filter'.
However, it causes ob-R async session blocks to be over-aggressive in
removing the prompt.
Note that non-async ob-R blocks don't suffer from this problem,
because ob-R let-binds `comint-prompt-regexp' around
`org-babel-comint-with-output' (specifically, it adds a
beginning-of-line at the front of the regexp). However, I don't see a
good way to let-bind this around `org-babel-comint-async-filter'.
The best solution I could think of was to define a new buffer-local
variable, `org-babel-comint-prompt-regexp-override', which could be used
to override `comint-prompt-regexp' for the purpose of filtering. I
attach a patch with this solution.
Additionally, the regression causes causes misalignment of the output
due to removal of indentation. The fix for this is simpler, and
involves replacing a call of `org-trim' with `org-babel-chomp'.
I'm not sure if my patch is the best solution. But whatever solution
we arrive at, I would like to request that it be applied to bugfix
branch.
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-ob-R-Fix-over-aggressive-async-prompt-removal.patch --]
[-- Type: text/x-patch, Size: 6494 bytes --]
From 11177e57f8a0c77b6c6541b852c5d105d70afec0 Mon Sep 17 00:00:00 2001
From: Jack Kamm <jackkamm@gmail.com>
Date: Sun, 22 Sep 2024 13:48:45 -0700
Subject: [PATCH] ob-R: Fix over-aggressive async prompt removal
* lisp/ob-comint.el (org-babel-comint-prompt-regexp-override): New
variable to override `comint-prompt-regexp' in
`org-babel-comint--prompt-filter'.
(org-babel-comint-async-filter): Replace `org-trim' with
`org-babel-chomp' to avoid removing leading indentation.
* lisp/ob-R.el (org-babel-R-evaluate): Set
`org-babel-comint-regexp-override' in session evaluation.
(org-babel-R-evaluate-session): Remove let binding of
`comint-prompt-regexp', since `org-babel-comint-regexp-override' is
now set.
* testing/lisp/test-ob-R.el (test-ob-R/async-prompt-filter): Test for
over-aggressive prompt removal.
---
lisp/ob-R.el | 25 ++++++++++++++-----------
lisp/ob-comint.el | 18 +++++++++++++++---
testing/lisp/test-ob-R.el | 28 ++++++++++++++++++++++++++++
3 files changed, 57 insertions(+), 14 deletions(-)
diff --git a/lisp/ob-R.el b/lisp/ob-R.el
index de2d27a9a..a9a58d0e4 100644
--- a/lisp/ob-R.el
+++ b/lisp/ob-R.el
@@ -375,11 +375,15 @@ (defun org-babel-R-evaluate
(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 column-names-p row-names-p)
- (org-babel-R-evaluate-session
- session body result-type result-params column-names-p row-names-p))
+ (progn
+ (with-current-buffer session
+ (setq org-babel-comint-prompt-regexp-override
+ (concat "^" comint-prompt-regexp)))
+ (if async
+ (ob-session-async-org-babel-R-evaluate-session
+ session body result-type column-names-p row-names-p)
+ (org-babel-R-evaluate-session
+ 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)))
@@ -456,12 +460,11 @@ (defun org-babel-R-evaluate-session
(substring line (match-end 1))
line))
(with-current-buffer session
- (let ((comint-prompt-regexp (concat "^" comint-prompt-regexp)))
- (org-babel-comint-with-output (session org-babel-R-eoe-output)
- (insert (mapconcat 'org-babel-chomp
- (list body org-babel-R-eoe-indicator)
- "\n"))
- (inferior-ess-send-input)))))))) "\n"))))
+ (org-babel-comint-with-output (session org-babel-R-eoe-output)
+ (insert (mapconcat 'org-babel-chomp
+ (list body org-babel-R-eoe-indicator)
+ "\n"))
+ (inferior-ess-send-input))))))) "\n"))))
(defun org-babel-R-process-value-result (result column-names-p)
"R-specific processing of return value.
diff --git a/lisp/ob-comint.el b/lisp/ob-comint.el
index 764927af7..7f1686035 100644
--- a/lisp/ob-comint.el
+++ b/lisp/ob-comint.el
@@ -75,11 +75,17 @@ (defun org-babel-comint--set-fallback-prompt ()
(setq comint-prompt-regexp org-babel-comint-prompt-regexp-old
org-babel-comint-prompt-regexp-old tmp))))
+(defvar-local org-babel-comint-prompt-regexp-override nil
+ "Overrides `comint-prompt-regexp' in `org-babel-comint--prompt-filter.'")
+
(defun org-babel-comint--prompt-filter (string &optional prompt-regexp)
"Remove PROMPT-REGEXP from STRING.
-PROMPT-REGEXP defaults to `comint-prompt-regexp'."
- (let* ((prompt-regexp (or prompt-regexp comint-prompt-regexp))
+PROMPT-REGEXP defaults to `comint-prompt-regexp', which can be
+overridden with `org-babel-comint-prompt-regexp-override'."
+ (let* ((prompt-regexp (or prompt-regexp
+ org-babel-comint-prompt-regexp-override
+ comint-prompt-regexp))
;; We need newline in case if we do progressive replacement
;; of agglomerated comint prompts with `comint-prompt-regexp'
;; containing ^.
@@ -327,7 +333,13 @@ (defun org-babel-comint-async-filter (string)
(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"))
+ (res-promptless
+ (org-trim (string-join
+ (mapcar #'org-babel-chomp
+ (org-babel-comint--prompt-filter
+ res-str-raw))
+ "\n")
+ t))
;; Apply user callback
(res-str (funcall org-babel-comint-async-chunk-callback res-promptless)))
;; Search for uuid in associated org-buffers to insert results
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.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] Async sessions: Fix prompt removal regression in ob-R
2024-09-22 21:45 [PATCH] Async sessions: Fix prompt removal regression in ob-R Jack Kamm
@ 2024-10-02 17:05 ` Ihor Radchenko
2024-10-15 7:03 ` Jack Kamm
0 siblings, 1 reply; 3+ messages in thread
From: Ihor Radchenko @ 2024-10-02 17:05 UTC (permalink / raw)
To: Jack Kamm; +Cc: emacs-orgmode, matt, jeremiejuste
Jack Kamm <jackkamm@gmail.com> writes:
> Consider the following R block, which prints the occurrences of each
> element in a list, including NAs:
>
> #+begin_src R :session :results output :async
> table(c("ab","ab","c",NA,NA), useNA='always')
> #+end_src
>
> #+RESULTS:
> : ab c <NA>
> : 2 1 2
>
> Since Org 9.7, it instead prints:
>
> #+RESULTS:
> : ab c <
> : 2 1 2
> ...
> The best solution I could think of was to define a new buffer-local
> variable, `org-babel-comint-prompt-regexp-override', which could be used
> to override `comint-prompt-regexp' for the purpose of filtering. I
> attach a patch with this solution.
Maybe we can simply override `comint-prompt-regexp' as we do in
ob-shell? The default regexp seems to be too permissive.
--
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] Async sessions: Fix prompt removal regression in ob-R
2024-10-02 17:05 ` Ihor Radchenko
@ 2024-10-15 7:03 ` Jack Kamm
0 siblings, 0 replies; 3+ messages in thread
From: Jack Kamm @ 2024-10-15 7:03 UTC (permalink / raw)
To: Ihor Radchenko; +Cc: emacs-orgmode, matt, jeremiejuste
[-- 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
^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2024-10-15 7:04 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-09-22 21:45 [PATCH] Async sessions: Fix prompt removal regression in ob-R Jack Kamm
2024-10-02 17:05 ` Ihor Radchenko
2024-10-15 7:03 ` Jack Kamm
Code repositories for project(s) associated with this public 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).