[PATCH] Fix several issues with python session value blocks

[PATCH] Fix several issues with python session value blocks

[Jack Kamm]

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

This patch fixes several related issues with python blocks with
parameters ":session :results value", including:

- Broken if-else and try-except statements.
- Correctly parsing blank lines in indented blocks.
- Returning the correct value when the underscore "_" variable
  has been assigned.

It works by using the built-in ast python module to parse the source
block, then executes it, evaluating the last line separately to store
the result.

Note this patch also creates a slight change in behavior: the final
result must be a top-level expression, otherwise we return "None" as the
result.

There is some useful background and discussion of the issues here:

https://lists.gnu.org/archive/html/emacs-orgmode/2017-11/msg00274.html

In that thread from 2017, we solved similar problems for the ":results
output" case. With this patch, I'm now circling back to try and fix the
":results value" case.

I've checked this patch works for both Python2 and Python3, as well as
for IPython.

Here are some examples that this patch fixes. I've also added them as
unit tests:

This block should return 20, but previously it raised an
IndentationError and then returned 10:

#+begin_src python :session :results value
  foo = 0
  for i in range(10):
      foo += 1

      foo += 1

  foo
#+end_src

This block should return "success", but previously it raised a
SyntaxError and then returned "failure":

#+begin_src python :session :results value
  value = "failure"
  if False:
      pass
  else:
      value = "success"
  value
#+end_src

This block should return "success", but previously it returned "failure":

#+begin_src python :session :results value
  _ = "failure"
  "success"
#+end_src


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

From 0d8f35d8a34c6297ade50f321f79b07a3cf8d5aa 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.  Uses the built-in ast
python module to parse a source block, execute it, and evaluate the
last line separately to return as a result.  Introduces a slight
change in behavior, requiring that the last line must be a top-level
statement if it's result is to be saved (otherwise, the result is
None).
---
 lisp/ob-python.el              | 64 ++++++++++++++++++----------------
 testing/lisp/test-ob-python.el | 35 +++++++++++++++++++
 2 files changed, 69 insertions(+), 30 deletions(-)

diff --git a/lisp/ob-python.el b/lisp/ob-python.el
index 823f6e63d..a610c471f 100644
--- a/lisp/ob-python.el
+++ b/lisp/ob-python.el
@@ -247,6 +247,20 @@ open('%s', 'w').write( pprint.pformat(main()) )")
    ")); "
    "__org_babel_python_fh.close()"))
 
+(defconst org-babel-python--eval-tmpfile "import ast
+with open('%s') as f:
+    __org_babel_python_ast = ast.parse(f.read())
+
+__org_babel_python_final = __org_babel_python_ast.body[-1]
+if type(__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")
+
 (defun org-babel-python-evaluate
   (session body &optional result-type result-params preamble)
   "Evaluate BODY as Python code."
@@ -294,34 +308,10 @@ 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))
+		       (mapc (lambda (line) (insert line) (funcall send-wait))
+			     (split-string body "[\r\n]"))
 		       (funcall send-wait)))
          (results
           (pcase result-type
@@ -344,17 +334,31 @@ 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-tmpfile
+				   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))
+		   (mapc
+		    (lambda (statement) (insert statement) (funcall send-wait))
+		    (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)))))
                    (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

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

[Kyle Meyer]

Jack Kamm <jackkamm@gmail.com> writes:

> This patch fixes several related issues with python blocks with
> parameters ":session :results value", including:
>
> - Broken if-else and try-except statements.
> - Correctly parsing blank lines in indented blocks.
> - Returning the correct value when the underscore "_" variable
>   has been assigned.
> [...]
> There is some useful background and discussion of the issues here:
>
> https://lists.gnu.org/archive/html/emacs-orgmode/2017-11/msg00274.html
>
> In that thread from 2017, we solved similar problems for the ":results
> output" case. With this patch, I'm now circling back to try and fix the
> ":results value" case.

Thanks!  I'll plan to take a deeper look within the next week.  However,
given that I'm not an ob-python user (or a much of a babel user in
general), I hope those who are will try out your patch and give
feedback.

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

[Jack Kamm]

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

Thanks Kyle! I've slightly updated the patch, because I had neglected to
handle Python exceptions. If you haven't started yet, please consider
the updated patch instead (it only differs by a few lines within
org-babel-python--eval-tmpfile).


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

From c2fff65f36141b78d91af9d6264d0f936ee5a3a1 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.  Uses the built-in ast
python module to parse a source block, execute it, and evaluate the
last line separately to return as a result.  Introduces a slight
change in behavior, requiring that the last line must be a top-level
statement if it's result is to be saved (otherwise, the result is
None).
---
 lisp/ob-python.el              | 68 +++++++++++++++++++---------------
 testing/lisp/test-ob-python.el | 35 +++++++++++++++++
 2 files changed, 73 insertions(+), 30 deletions(-)

diff --git a/lisp/ob-python.el b/lisp/ob-python.el
index 823f6e63d..0b1073df5 100644
--- a/lisp/ob-python.el
+++ b/lisp/ob-python.el
@@ -247,6 +247,24 @@ open('%s', 'w').write( pprint.pformat(main()) )")
    ")); "
    "__org_babel_python_fh.close()"))
 
+(defconst org-babel-python--eval-tmpfile "import ast
+with open('%s') as f:
+    __org_babel_python_ast = ast.parse(f.read())
+
+__org_babel_python_final = __org_babel_python_ast.body[-1]
+try:
+    if type(__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 as e:
+        __org_babel_python_final = e
+        raise e")
+
 (defun org-babel-python-evaluate
   (session body &optional result-type result-params preamble)
   "Evaluate BODY as Python code."
@@ -294,34 +312,10 @@ 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))
+		       (mapc (lambda (line) (insert line) (funcall send-wait))
+			     (split-string body "[\r\n]"))
 		       (funcall send-wait)))
          (results
           (pcase result-type
@@ -344,17 +338,31 @@ 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-tmpfile
+				   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))
+		   (mapc
+		    (lambda (statement) (insert statement) (funcall send-wait))
+		    (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)))))
                    (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

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

[Bastien]

Hi Jack,

thanks for your patch - Kyle is on it, but IIRC we do not have a
maintainer for the ob-python.el file.

Would you like to take this in charge?  Or someone else?

Thanks,

-- 
 Bastien

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

[Jack Kamm]

> thanks for your patch - Kyle is on it, but IIRC we do not have a
> maintainer for the ob-python.el file.
>
> Would you like to take this in charge?  Or someone else?

Sure, I would be interested in this, if it's helpful.

I need an account for code.orgmode.org, and whatever commit permissions
you think appropriate.

I should caveat that I have only submitted a handful of patches, and am
still learning the best practices here. But I am trying to learn, and to
become more involved.

Best,
Jack

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

[Bastien]

Hi Jack,

Jack Kamm <jackkamm@gmail.com> writes:

> Sure, I would be interested in this, if it's helpful.

It is for sure!  Thanks for accepting.

> I need an account for code.orgmode.org, and whatever commit permissions
> you think appropriate.

Please send me a private message with your prefered username and I
will create the account on code.orgmode.org and add you to org-mode
as a collaborator.

> I should caveat that I have only submitted a handful of patches, and am
> still learning the best practices here. But I am trying to learn, and to
> become more involved.

We're all learning, it's always nice to have new contributors.

Thanks again,

-- 
 Bastien

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

[황병희]

Jack Kamm <jackkamm@gmail.com> writes:

> [...]
> I've checked this patch works for both Python2 and Python3, as well as
> for IPython.

Currently, i'm learning both elisp and python. Thanks for good patchs^^^

Sincerely, Byung-Hee

-- 
^고맙습니다 _地平天成_ 감사합니다_^))//

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

[Kyle Meyer]

Hi Jack,

Thanks again for the patch.  Testing it out a bit, your ast-based
approach seems to work nicely.

Jack Kamm <jackkamm@gmail.com> writes:

> 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.  Uses the built-in ast

This is subjective, but I'd say up until "Uses [...]" would be good for
the changelog entry, and the remaining discussion could come as a
paragraph after.

The changelog should also include

    (org-babel-python--eval-tmpfile): New variable

or rather whatever the name ends up being, based on the discussion
below.

The content of your commit message is good as is, but I would be happy
to see a brief mention of what the approach is that's being replaced
before you describe the new approach with ast.

> python module to parse a source block, execute it, and evaluate the
> last line separately to return as a result.  Introduces a slight
> change in behavior, requiring that the last line must be a top-level
> statement if it's result is to be saved (otherwise, the result is
> None).

s/it's/its/

Wouldn't the more specific "top-level expression statement" be clearer
than "top-level statement"?

It's nice to see the change in behavior noted.  My uninformed guess is
that not many users were relying on getting the last value from
non-top-level expressions and that the issues you fix here very much
outweigh support for that, especially given that the change in behavior
is easy to accommodate.  However, this change in behavior probably
deserves a mention in ORG-NEWS.

Wrapped up in your statement about the top-level requirement, there is
also a change in behavior in that None will now show up under
"#+RESULTS:", as it already did with ":results value" for non-session
blocks.  I think the old behavior was likely just a limitation of
getting the old value from "_", and that the new behavior can be viewed
as a fix.

> diff --git a/lisp/ob-python.el b/lisp/ob-python.el
> index 823f6e63d..0b1073df5 100644
> --- a/lisp/ob-python.el
> +++ b/lisp/ob-python.el
> @@ -247,6 +247,24 @@ open('%s', 'w').write( pprint.pformat(main()) )")

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

,----[ $XDG_CONFIG_HOME/git/attributes ]
| *.el     diff=lisp
`----

,----[ $XDG_CONFIG_HOME/git/config or ~/.gitconfig ]
| [diff "lisp"]
| 	xfuncname = "^(\\(.*)$"
`----

>     ")); "
>     "__org_babel_python_fh.close()"))
>  

> +(defconst org-babel-python--eval-tmpfile "import ast

nitpick: I think it'd read a bit nicer if you put `import ast' on a new
line with the same indentation level.  You could still avoid the string
having a new line at the start by using "\".

I don't find this variable name to be very clear, particularly when
taken along with the preceding org-babel-python--exec-tmpfile.  This new
variable is using both exec() and eval(), though granted the eval() is
the crucial bit for getting the value.  Perhaps a clearer name would be
something like org-babel-python--eval-ast.

> +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.

> +    __org_babel_python_ast = ast.parse(f.read())
> +
> +__org_babel_python_final = __org_babel_python_ast.body[-1]
> +try:
> +    if type(__org_babel_python_final) == ast.Expr:

I'd prefer `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

Okay, so if the last top-level node is an expression statement, convert
it to an Expression() that can be passed to eval() so that we can get
the return value.  Makes sense.

> +except Exception as e:
> +        __org_babel_python_final = e
> +        raise e")

nitpick: Your indentation here is one level too deep.

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.

Assuming we want to stick with that behavior, I suggest two changes:

  * Absorb the ast.parse() call into the try suite.  That way syntax
    errors in the source block are reported.

  * Set the value to some sort of traceback information rather than the
    exception itself.  If you set it to the exception, something like
    ValueError("i look like a proper result") will just show up under
    "#+RESULTS:" as "i look like a proper result" when it's fed to
    str().

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.

>  	 (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))
> +		       (mapc (lambda (line) (insert line) (funcall send-wait))
> +			     (split-string body "[\r\n]"))

Let's stick with dolist rather than switching to mapc.  In addition to
leading to a more minimal/pleasant diff, it's in line with the
preference shown by changes like ef0178980 (org.el: Use `dolist' instead
of `mapc' + `lambda', 2016-01-10).

>              (`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-tmpfile
> +				   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))
> +		   (mapc

Same comment here about mapc vs dolist.

> +		    (lambda (statement) (insert statement) (funcall send-wait))
> +		    (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))"

Hmm, this str(), as well as the write(), could cause encoding errors on
Python 2 if __org_babel_python_final is a unicode() object.  I don't
think adding a compatibility kludge is worth the trouble given that
other spots in ob-python.el have similar issues and that Python 2 has
reached EOL.

-- >8 --
diff --git a/lisp/ob-python.el b/lisp/ob-python.el
index 0b1073df5..8cbcd54a3 100644
--- a/lisp/ob-python.el
+++ b/lisp/ob-python.el
@@ -248,11 +248,10 @@ (defconst org-babel-python--exec-tmpfile
    "__org_babel_python_fh.close()"))
 
 (defconst org-babel-python--eval-tmpfile "import ast
-with open('%s') as f:
-    __org_babel_python_ast = ast.parse(f.read())
-
-__org_babel_python_final = __org_babel_python_ast.body[-1]
 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 type(__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'))
@@ -261,9 +260,9 @@ (defconst org-babel-python--eval-tmpfile "import ast
     else:
         exec(compile(__org_babel_python_ast, '<string>', 'exec'))
         __org_babel_python_final = None
-except Exception as e:
-        __org_babel_python_final = e
-        raise e")
+except Exception:
+    from traceback import format_exc
+    __org_babel_python_final = format_exc()")
 
 (defun org-babel-python-evaluate
   (session body &optional result-type result-params preamble)

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

[Jack Kamm]

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

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

[Kyle Meyer]

Jack Kamm <jackkamm@gmail.com> writes:

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

Thanks for the updates.

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

All good from my end :>

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

[Jack Kamm]

Kyle Meyer <kyle@kyleam.com> writes:

>> 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.
> All good from my end :>

Thanks -- I've pushed to master :D

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

[Bastien]

Hi Jack,

Jack Kamm <jackkamm@gmail.com> writes:

> Kyle Meyer <kyle@kyleam.com> writes:
>
>>> 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.
>> All good from my end :>
>
> Thanks -- I've pushed to master :D

Thanks a lot!  Nitpicking, there is this new warning:

  In toplevel form:                                                
  ob-python.el:309:1: Warning: Unused lexical variable ‘last-indent’

Can you fix it?

Thanks again,

-- 
 Bastien

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

[Jack Kamm]

Hi Bastien,

Bastien <bzg@gnu.org> writes:

> Thanks a lot!  Nitpicking, there is this new warning:
>
>   In toplevel form:                                                
>   ob-python.el:309:1: Warning: Unused lexical variable ‘last-indent’
>
> Can you fix it?

Oops, sorry for letting that slip through, and thanks for noticing so
quickly. I've pushed a fix for this now.

Cheers,
Jack