[PATCH] (Tiny) Tweak Python session null return value

[PATCH] (Tiny) Tweak Python session null return value

[Jack Kamm]

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

Hi,

Below is a very small patch to Python session blocks, to make them
return a blank result (empty string) instead of None when there is no
return value.

Normally I would push this myself, but since we are so close to 9.4, I
thought it prudent to mail a patch and let the maintainers handle it. It
would be nice to include in 9.4, but not a big deal if I've missed the
window.

Now for an explanation of the patch: 9.4 changes Python session blocks
to fix several bugs and improve robustness overall [0]. However, there
is a cost to these fixes, which is that the session blocks can only
return a result when it is a top-level expression on the last line of
the block. If the last line is not a top-level expression, the block
would previously print "None". However, after some testing, I think this
is a little counter-intuitive, and it would be better if it returned a
blank (empty) result instead of "None".

[0] https://lists.gnu.org/archive/html/emacs-orgmode/2020-01/msg00190.html

Best,
Jack



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-ob-python-Session-returns-empty-string-if-no-return-.patch --]
[-- Type: text/x-patch, Size: 2040 bytes --]

From 0b44c3f1c7454e7948cd34eb02995924046b6976 Mon Sep 17 00:00:00 2001
From: Jack Kamm <jackkamm@gmail.com>
Date: Mon, 17 Feb 2020 08:11:49 -0800
Subject: [PATCH] ob-python: Session returns empty string if no return value

* lisp/ob-python.el (org-babel-python--eval-ast): Change sessions to
return an empty string, instead of None, if there is no return value.
---
 etc/ORG-NEWS      | 12 +++++++-----
 lisp/ob-python.el |  2 +-
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS
index b6ee443e4..1b5429870 100644
--- a/etc/ORG-NEWS
+++ b/etc/ORG-NEWS
@@ -12,13 +12,15 @@ 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
+*** Change how Python sessions handle return values
 
 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.
+return a value if the last line is a top-level expression
+statement. If the last line is not a top-level expression statement,
+the block will return a blank (empty) result.
+
+Also, a return value of ~None~ will now show up under "#+RESULTS:", as
+it already did for non-session blocks.
 
 *** In HTML export, change on how outline-container-* is set
 
diff --git a/lisp/ob-python.el b/lisp/ob-python.el
index de718b04b..b9e36dba0 100644
--- a/lisp/ob-python.el
+++ b/lisp/ob-python.el
@@ -260,7 +260,7 @@ (defconst org-babel-python--eval-ast "\
             __org_babel_python_final.value), '<string>', 'eval'))
     else:
         exec(compile(__org_babel_python_ast, '<string>', 'exec'))
-        __org_babel_python_final = None
+        __org_babel_python_final = ''
 except Exception:
     from traceback import format_exc
     __org_babel_python_final = format_exc()
-- 
2.25.0

Re: [PATCH] (Tiny) Tweak Python session null return value

[Bastien]

Hi Jack,

Jack Kamm <jackkamm@gmail.com> writes:

> Below is a very small patch to Python session blocks, to make them
> return a blank result (empty string) instead of None when there is no
> return value.
>
> Normally I would push this myself, but since we are so close to 9.4, I
> thought it prudent to mail a patch and let the maintainers handle it. It
> would be nice to include in 9.4, but not a big deal if I've missed the
> window.

Applied, thanks for the patch and the precaution not to commit it
directly.

> Now for an explanation of the patch: 9.4 changes Python session blocks
> to fix several bugs and improve robustness overall [0]. However, there
> is a cost to these fixes, which is that the session blocks can only
> return a result when it is a top-level expression on the last line of
> the block. If the last line is not a top-level expression, the block
> would previously print "None". However, after some testing, I think this
> is a little counter-intuitive, and it would be better if it returned a
> blank (empty) result instead of "None".

I've seen you update the NEWS entry, which is good: is there a way to
present the enhancements in the "* New features" section?  If you feel
like it, please advertize the enhancements there too.

Thanks,

-- 
 Bastien

Re: [PATCH] (Tiny) Tweak Python session null return value

[John Kitchin]

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

I think None is correct. If you don't specify a return value in Python,
then a function returns None. I would expect that to happen in a Python
block too.


John

-----------------------------------
Professor John Kitchin
Doherty Hall A207F
Department of Chemical Engineering
Carnegie Mellon University
Pittsburgh, PA 15213
412-268-7803
@johnkitchin
http://kitchingroup.cheme.cmu.edu



On Mon, Feb 17, 2020 at 11:25 AM Jack Kamm <jackkamm@gmail.com> wrote:

> Hi,
>
> Below is a very small patch to Python session blocks, to make them
> return a blank result (empty string) instead of None when there is no
> return value.
>
> Normally I would push this myself, but since we are so close to 9.4, I
> thought it prudent to mail a patch and let the maintainers handle it. It
> would be nice to include in 9.4, but not a big deal if I've missed the
> window.
>
> Now for an explanation of the patch: 9.4 changes Python session blocks
> to fix several bugs and improve robustness overall [0]. However, there
> is a cost to these fixes, which is that the session blocks can only
> return a result when it is a top-level expression on the last line of
> the block. If the last line is not a top-level expression, the block
> would previously print "None". However, after some testing, I think this
> is a little counter-intuitive, and it would be better if it returned a
> blank (empty) result instead of "None".
>
> [0] https://lists.gnu.org/archive/html/emacs-orgmode/2020-01/msg00190.html
>
> Best,
> Jack
>
>
>

[-- Attachment #2: Type: text/html, Size: 2332 bytes --]

Re: [PATCH] (Tiny) Tweak Python session null return value

[Jack Kamm]

Hi John,

John Kitchin <jkitchin@andrew.cmu.edu> writes:

> I think None is correct. If you don't specify a return value in Python,
> then a function returns None. I would expect that to happen in a Python
> block too.

Hmm, OK, thanks for your intuition, it's useful feedback.

Working this out loud, I was considering the following sort of block:

#+begin_src python :session :results value
  if some_condition:
      True
  else:
      False
#+end_src

#+RESULTS:
: None

Ideally, it would return True/False, but the current implementation
cannot grab that result unfortunately. (In its defense, it at least
doesn't crash like it did before).

I was thinking not printing anything at all under "#+RESULTS" would be
less surprising than printing "None". But both are admittedly surprising
at first.

I agree your preference of returning "None" is the more technically
consistent behavior though.

I don't want to rush a decision now, I think I need to gather more user
feedback after the 9.4 release. I'll ask Bastien to revert that commit
and put this off to 9.5, rather than writing an updated NEWS entry like
he asked.

Either way, we'll need to update Worg to clearly document what's going
on here.

Re: [PATCH] (Tiny) Tweak Python session null return value

[Jack Kamm]

Hi Bastien,

> I've seen you update the NEWS entry, which is good: is there a way to
> present the enhancements in the "* New features" section?  If you feel
> like it, please advertize the enhancements there too.

Given John's feedback, I now think it's better to put off this change to
9.5, if at all. So instead of updating NEWS, I'd prefer to revert this
commit for now. Sorry for the noise! Looking forward to 9.4.

Re: [PATCH] (Tiny) Tweak Python session null return value

[John Kitchin]

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

I can see why you would want to see True/False there, but to get the value,
you need to specifically return what you want because AFAIK the body is
wrapped in a function that is evaluated to get the value, it is not simply
the last thing that gets evaluated. Your example clarified to me at least
why it would be tricky to figure it out, you can't rely on the last line,
for example. I don't know if there is some special Python variable that
contains that.

It is also a little strange to me to put return specifically in like this:

#+begin_src python :session :results value
  if some_condition:
      a = True
  else:
      a = False
return a
#+end_src

or:

#+BEGIN_SRC python :results value
a = 1
return True if a else False
#+END_SRC

#+RESULTS:
: True

because it is not valid syntax in a script (try it with :results output),
but this is how it has been for ob-python all along when what you want is
the value returned.

John

-----------------------------------
Professor John Kitchin
Doherty Hall A207F
Department of Chemical Engineering
Carnegie Mellon University
Pittsburgh, PA 15213
412-268-7803
@johnkitchin
http://kitchingroup.cheme.cmu.edu



On Mon, Feb 17, 2020 at 2:06 PM Jack Kamm <jackkamm@gmail.com> wrote:

> Hi John,
>
> John Kitchin <jkitchin@andrew.cmu.edu> writes:
>
> > I think None is correct. If you don't specify a return value in Python,
> > then a function returns None. I would expect that to happen in a Python
> > block too.
>
> Hmm, OK, thanks for your intuition, it's useful feedback.
>
> Working this out loud, I was considering the following sort of block:
>
> #+begin_src python :session :results value
>   if some_condition:
>       True
>   else:
>       False
> #+end_src
>
> #+RESULTS:
> : None
>
> Ideally, it would return True/False, but the current implementation
> cannot grab that result unfortunately. (In its defense, it at least
> doesn't crash like it did before).
>
> I was thinking not printing anything at all under "#+RESULTS" would be
> less surprising than printing "None". But both are admittedly surprising
> at first.
>
> I agree your preference of returning "None" is the more technically
> consistent behavior though.
>
> I don't want to rush a decision now, I think I need to gather more user
> feedback after the 9.4 release. I'll ask Bastien to revert that commit
> and put this off to 9.5, rather than writing an updated NEWS entry like
> he asked.
>
> Either way, we'll need to update Worg to clearly document what's going
> on here.
>

[-- Attachment #2: Type: text/html, Size: 3531 bytes --]

Re: [PATCH] (Tiny) Tweak Python session null return value

[Jack Kamm]

Hi John,

John Kitchin <jkitchin@andrew.cmu.edu> writes:

> I can see why you would want to see True/False there, but to get the value,
> you need to specifically return what you want because AFAIK the body is
> wrapped in a function that is evaluated to get the value, it is not simply
> the last thing that gets evaluated.

This is true for non-session blocks, which require explicitly calling
"return". However, session blocks aren't wrapped in functions and don't
use "return" (even before the most recent patches). The problem is that
variables created in a function have local scope, so session blocks
can't be wrapped in functions.

> Your example clarified to me at least why it would be tricky to figure
> it out, you can't rely on the last line, for example.

Since the recent patches, we do extract the last line, using the Python
ast module, however this only works if the last line is a top-level
statement like "f()" or "1+1", not an assignment (like "x = 1+1") or an
indented block (like "if:...else:...").

>  I don't know if there is some special Python variable that contains
> that.

There actually is -- in most Python interpreters, the variable "_"
(underscore) refers to the last statement, unless it's been explicitly
assigned to. This is what was previously relied on. Unfortunately, using
"_" for a dummy variable is a common Python idiom (e.g. "for _ in
range(10)"), and if used would break all subsequent Python session
blocks. So we no longer rely on "_".

In the standard Python interpreter, we can also use "__builtins__._",
but this doesn't work in IPython. Furthermore, this only works for code
explicitly entered in the shell, it won't work for code executed in
"exec()" or "eval()", which we now rely on, because it handles
indentation much more robustly. In particular, ob-python sessions have
had longstanding issues with multiline indented blocks, which are now
solved in the recent patches.

Re: [PATCH] (Tiny) Tweak Python session null return value

[John Kitchin]

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

On Mon, Feb 17, 2020 at 3:46 PM Jack Kamm <jackkamm@gmail.com> wrote:

> Hi John,
>
> John Kitchin <jkitchin@andrew.cmu.edu> writes:
>
> > I can see why you would want to see True/False there, but to get the
> value,
> > you need to specifically return what you want because AFAIK the body is
> > wrapped in a function that is evaluated to get the value, it is not
> simply
> > the last thing that gets evaluated.
>
> This is true for non-session blocks, which require explicitly calling
> "return". However, session blocks aren't wrapped in functions and don't
> use "return" (even before the most recent patches). The problem is that
> variables created in a function have local scope, so session blocks
> can't be wrapped in functions.
>

Fair point, I am not a python session user (I have used the ob-ipython for
a long time, or stand-alone python blocks), and I had forgotten or not
known of this. Indeed in a REPL, you get something closer to what you
originally suggested.

>>> a = 1

>>> if a:

...     True

... else:

...     False

...

True

I guess I would expect something like that if I was using a Python session
in org-mode. It is like a REPL that is easier to edit.

My earlier concern is mostly related to consistency of what an org Python
block does compared to what you might do at a REPL or from a script. I also
note that I almost never use :results value, and almost always prefer
:results output. That reflects the kind of stuff we usually do here though,
and may not be representative of others.



> > Your example clarified to me at least why it would be tricky to figure
> > it out, you can't rely on the last line, for example.
>
> Since the recent patches, we do extract the last line, using the Python
> ast module, however this only works if the last line is a top-level
> statement like "f()" or "1+1", not an assignment (like "x = 1+1") or an
> indented block (like "if:...else:...").
>
> >  I don't know if there is some special Python variable that contains
> > that.
>
> There actually is -- in most Python interpreters, the variable "_"
> (underscore) refers to the last statement, unless it's been explicitly
> assigned to. This is what was previously relied on. Unfortunately, using
> "_" for a dummy variable is a common Python idiom (e.g. "for _ in
> range(10)"), and if used would break all subsequent Python session
> blocks. So we no longer rely on "_".


> In the standard Python interpreter, we can also use "__builtins__._",
> but this doesn't work in IPython. Furthermore, this only works for code

explicitly entered in the shell, it won't work for code executed in
> "exec()" or "eval()", which we now rely on, because it handles
> indentation much more robustly. In particular, ob-python sessions have
> had longstanding issues with multiline indented blocks, which are now
> solved in the recent patches.
>

[-- Attachment #2: Type: text/html, Size: 6289 bytes --]

Re: [PATCH] (Tiny) Tweak Python session null return value

[Bastien]

Hi Jack,

Jack Kamm <jackkamm@gmail.com> writes:

>> I've seen you update the NEWS entry, which is good: is there a way to
>> present the enhancements in the "* New features" section?  If you feel
>> like it, please advertize the enhancements there too.
>
> Given John's feedback, I now think it's better to put off this change to
> 9.5, if at all. So instead of updating NEWS, I'd prefer to revert this
> commit for now. Sorry for the noise! Looking forward to 9.4.

Sure, I just reverted the commit.

This discussion made me realize that the various Babel backends may
have different approaches wrt evaluating blocks, returning output and
results, interpreting empty output/result, etc.  The mere notion of a
"session" may differ from one language to another.

I wish we can push for more consistency here--perhaps the first stop
would be (1) to pick major backends and (2) document their behaviors.

I'm sure this could help enhancing Org babel code too.

Thanks,

-- 
 Bastien