Re: [PATCH] Fix several issues with python session value blocks

[Jack Kamm <jackkamm@gmail.com>]

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

Hi Kyle,

Thanks for the thorough review as always. An updated patch incorporating
your feedback is at the end of this email.

I'd like to do the honors of making my first push to the repo :) So
please let me know if you're fine with me to proceed. Or, if you have
more comments, they are always appreciated.

Kyle Meyer <kyle@kyleam.com> writes:

> Tangent: You can get better context for your diffs if you define a
> custom xfuncname.

Thanks for the tip!

>> +with open('%s') as f:
>
> Hmm, I'm nervous about breakage here if org-babel-temp-file returns
> something with a single quote.  However, that's already a problem with
> org-babel-python--exec-tmpfile used for ":results output", as well as
> with a couple of other spots, so I think it'd be okay to punt on that
> for now.

Thanks for pointing this out, I've noted it and will try to fix it in
future.

> Hmm, we set the result to the exception on error, so the exception will
> now show up under "#+RESULTS:".  As a not-really-babel user, my guess is
> that that'd be a good thing, but I do wonder how other languages handle
> exceptions.

I'm most familiar with R, Julia, and shell so I checked how those
languages deal with errors when ":session :results value". In general,
they print an empty "#+RESULTS:" element, with the exception of Julia
which hangs Emacs.

So, printing an empty results block would be the more consistent
behavior here. This can be achieved by setting __org_babel_python_final
to the empty string in the exception clause.

On the other hand, I do think that printing the traceback is useful
behavior. For now, I've opted to keep the traceback output, because
we've already written it, and it's easy to remove later if we want. But
I don't have a strong opinion here.

> I've included a patch at the end that sits on top of yours and does
> those two things.  If it looks reasonable to you, please squash it into
> your patch.

Looks good, I've squashed it in, however I did add back a call to
"raise" in the exception block, so that the error would appear in the
REPL (which was the general behavior for shell, R, and Julia errors).


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-ob-python-Fix-several-issues-with-session-results-va.patch --]
[-- Type: text/x-patch, Size: 7323 bytes --]

From 572ca9fd8c89720acd8d7d2ace8bb3c0be3d288e Mon Sep 17 00:00:00 2001
From: Jack Kamm <jackkamm@gmail.com>
Date: Mon, 20 Jan 2020 17:40:22 -0800
Subject: [PATCH] ob-python: Fix several issues with :session :results value

* lisp/ob-python.el (org-babel-python-evaluate-session): Fix a few
related issues with :session :results value blocks, including broken
if-else statements, indented blocks with blank lines, and returning
the wrong value when underscore has been used.
(org-babel-python--eval-ast): New constant variable, a string
consisting of Python code to execute a source block using ast.

Previously, python blocks with parameters ":session :results value"
were entered line-by-line into the Python session, which could cause
issues around indentation and new lines.  Now, such python blocks are
written to temp files, then the built-in ast python module is used to
parse and execute them, and to extract the last line separately to
return as a result.  Introduces a change in behavior, requiring that
the last line must be a top-level expression statement if its result
is to be saved (otherwise, the result is None).
---
 etc/ORG-NEWS                   |  9 +++++
 lisp/ob-python.el              | 68 ++++++++++++++++++++--------------
 testing/lisp/test-ob-python.el | 35 +++++++++++++++++
 3 files changed, 85 insertions(+), 27 deletions(-)

diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS
index 6518c318d..2068b3aab 100644
--- a/etc/ORG-NEWS
+++ b/etc/ORG-NEWS
@@ -11,6 +11,15 @@ See the end of the file for license conditions.
 Please send Org bug reports to mailto:emacs-orgmode@gnu.org.
 
 * Version 9.4 (not yet released)
+** Incompatible changes
+*** Python session return values must be top-level expression statements
+
+Python blocks with ~:session :results value~ header arguments now only
+return a value if the last line is a top-level expression statement,
+otherwise the result is None. Also, None will now show up under
+"#+RESULTS:", as it already did with ~:results value~ for non-session
+blocks.
+
 ** New features
 
 *** Numeric priorities are now allowed (up to 65)
diff --git a/lisp/ob-python.el b/lisp/ob-python.el
index 823f6e63d..5f71577cb 100644
--- a/lisp/ob-python.el
+++ b/lisp/ob-python.el
@@ -247,6 +247,25 @@ open('%s', 'w').write( pprint.pformat(main()) )")
    ")); "
    "__org_babel_python_fh.close()"))
 
+(defconst org-babel-python--eval-ast "\
+import ast
+try:
+    with open('%s') as f:
+        __org_babel_python_ast = ast.parse(f.read())
+    __org_babel_python_final = __org_babel_python_ast.body[-1]
+    if isinstance(__org_babel_python_final, ast.Expr):
+        __org_babel_python_ast.body = __org_babel_python_ast.body[:-1]
+        exec(compile(__org_babel_python_ast, '<string>', 'exec'))
+        __org_babel_python_final = eval(compile(ast.Expression(
+            __org_babel_python_final.value), '<string>', 'eval'))
+    else:
+        exec(compile(__org_babel_python_ast, '<string>', 'exec'))
+        __org_babel_python_final = None
+except Exception:
+    from traceback import format_exc
+    __org_babel_python_final = format_exc()
+    raise")
+
 (defun org-babel-python-evaluate
   (session body &optional result-type result-params preamble)
   "Evaluate BODY as Python code."
@@ -294,32 +313,9 @@ If RESULT-TYPE equals `output' then return standard output as a
 string.  If RESULT-TYPE equals `value' then return the value of the
 last statement in BODY, as elisp."
   (let* ((send-wait (lambda () (comint-send-input nil t) (sleep-for 0 5)))
-	 (dump-last-value
-	  (lambda
-	    (tmp-file pp)
-	    (mapc
-	     (lambda (statement) (insert statement) (funcall send-wait))
-	     (if pp
-		 (list
-		  "import pprint"
-		  (format "open('%s', 'w').write(pprint.pformat(_))"
-			  (org-babel-process-file-name tmp-file 'noquote)))
-	       (list (format "open('%s', 'w').write(str(_))"
-			     (org-babel-process-file-name tmp-file
-                                                          'noquote)))))))
 	 (last-indent 0)
 	 (input-body (lambda (body)
 		       (dolist (line (split-string body "[\r\n]"))
-			 ;; Insert a blank line to end an indent
-			 ;; block.
-			 (let ((curr-indent (string-match "\\S-" line)))
-			   (if curr-indent
-			       (progn
-				 (when (< curr-indent last-indent)
-				   (insert "")
-				   (funcall send-wait))
-				 (setq last-indent curr-indent))
-			     (setq last-indent 0)))
 			 (insert line)
 			 (funcall send-wait))
 		       (funcall send-wait)))
@@ -344,17 +340,35 @@ last statement in BODY, as elisp."
 		   (funcall send-wait))
 		 2) "\n")))
             (`value
-             (let ((tmp-file (org-babel-temp-file "python-")))
+             (let ((tmp-results-file (org-babel-temp-file "python-"))
+		   (body (let ((tmp-src-file (org-babel-temp-file
+					      "python-")))
+			   (with-temp-file tmp-src-file (insert body))
+			   (format org-babel-python--eval-ast
+				   tmp-src-file))))
                (org-babel-comint-with-output
                    (session org-babel-python-eoe-indicator nil body)
                  (let ((comint-process-echoes nil))
                    (funcall input-body body)
-                   (funcall dump-last-value tmp-file
-                            (member "pp" result-params))
+		   (dolist
+		       (statement
+			(if (member "pp" result-params)
+			    (list
+			     "import pprint"
+			     (format "open('%s', 'w').write(pprint.pformat(\
+__org_babel_python_final))"
+				     (org-babel-process-file-name
+				      tmp-results-file 'noquote)))
+			  (list (format "open('%s', 'w').write(str(\
+__org_babel_python_final))"
+					(org-babel-process-file-name
+					 tmp-results-file 'noquote)))))
+		     (insert statement)
+		     (funcall send-wait))
                    (funcall send-wait) (funcall send-wait)
                    (insert org-babel-python-eoe-indicator)
                    (funcall send-wait)))
-               (org-babel-eval-read-file tmp-file))))))
+               (org-babel-eval-read-file tmp-results-file))))))
     (unless (string= (substring org-babel-python-eoe-indicator 1 -1) results)
       (org-babel-result-cond result-params
 	results
diff --git a/testing/lisp/test-ob-python.el b/testing/lisp/test-ob-python.el
index 48ca3d640..7e2826404 100644
--- a/testing/lisp/test-ob-python.el
+++ b/testing/lisp/test-ob-python.el
@@ -138,6 +138,41 @@ if True:
 	      (org-babel-execute-maybe)
 	      (org-babel-execute-src-block)))))
 
+(ert-deftest test-ob-python/if-else-block ()
+  (should
+   (equal "success" (org-test-with-temp-text "#+begin_src python :session :results value
+value = 'failure'
+if False:
+    pass
+else:
+    value = 'success'
+value
+#+end_src"
+	      (org-babel-execute-src-block)))))
+
+(ert-deftest test-ob-python/indent-block-with-blank-lines ()
+  (should
+   (equal 20
+	  (org-test-with-temp-text "#+begin_src python :session :results value
+  foo = 0
+  for i in range(10):
+      foo += 1
+
+      foo += 1
+
+  foo
+#+end_src"
+	    (org-babel-execute-src-block)))))
+
+(ert-deftest test-ob-python/assign-underscore ()
+  (should
+   (equal "success"
+	  (org-test-with-temp-text "#+begin_src python :session :results value
+_ = 'failure'
+'success'
+#+end_src"
+	    (org-babel-execute-src-block)))))
+
 (provide 'test-ob-python)
 
 ;;; test-ob-python.el ends here
-- 
2.25.0