ob-python newline & indentation behavior

ob-python newline & indentation behavior

[Jack Kamm]

ob-python newline & indentation behavior in :session is very ugly and 
possibly broken. For example, consider the following code block:

#+BEGIN_SRC python :session :results output
   foo = 0
   for _ in range(10):
       foo += 1

       foo += 1

   print(foo)
#+END_SRC

Ideally this would print "20", but instead it prints "11",
because the second "foo+=1" is executed after the for loop.
OTOH "python-shell-send-region" (from python.el) gives the correct 
answer of "20".

The inconsistent behavior of "python-shell-send-region" and 
"org-babel-eval" often causes me problems, because I want to use both 
(the former for testing and async eval; the latter for inserting into 
the document).

There is a 2 year old patch that fixes this behavior but has not yet 
been incorporated:
https://lists.gnu.org/archive/html/emacs-orgmode/2015-03/msg00505.html

The patch follows the python.el behavior of using a temporary file and 
executing that from the shell.

Could this patch, or something similar, be incorporated into org-mode, 
to fix this behavior?

Thanks,
Jack Kamm

Re: ob-python newline & indentation behavior

[Kyle Meyer]

Hello,

Jack Kamm <jackkamm@gmail.com> writes:

> ob-python newline & indentation behavior in :session is very ugly and 
> possibly broken. For example, consider the following code block:

[...]

> There is a 2 year old patch that fixes this behavior but has not yet 
> been incorporated:
> https://lists.gnu.org/archive/html/emacs-orgmode/2015-03/msg00505.html

[...]

> The patch follows the python.el behavior of using a temporary file and 
> executing that from the shell.
>
> Could this patch, or something similar, be incorporated into org-mode, 
> to fix this behavior?

Here's what I said in a recent post about ob-python sessions:

    https://lists.gnu.org/archive/html/emacs-orgmode/2017-10/msg00198.html

    I dropped my attempt to fix it because 1) I was still having trouble
    getting a complete understanding of what the issue was and 2) I didn't
    have the motivation to spend time digging deeper because I don't use
    ob-python (and in general am not a heavy Org-Babel user).  Perhaps you
    or some other ob-python user could help make ob-python sessions more
    robust?

Perhaps you are the "you"?
 
-- 
Kyle

Re: ob-python newline & indentation behavior

[Jack Kamm]

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

Thanks Kyle, and sorry for missing that recent related thread.

I adapted your old patch to the current master branch. I also extended it
to work for ":results value" (the original patch only worked for ":results
output"). I did this by not writing the last line of the code block to the
tmpfile, unless it is indented.

I've never contributed before so would appreciate any comments on this
patch, and how to get it accepted. Cheers, Jack.

From a5d553ece9f6ee35cd1e273e554a21a19e80ec3c Mon Sep 17 00:00:00 2001

From: Jack Kamm <jackkamm@gmail.com>
Date: Sat, 18 Nov 2017 21:47:09 +0000
Subject: [PATCH] fix newline/indentation issues in ob-python :session

---
 lisp/ob-python.el | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/lisp/ob-python.el b/lisp/ob-python.el
index 60ec5fa47..6623ef5fc 100644
--- a/lisp/ob-python.el
+++ b/lisp/ob-python.el
@@ -303,6 +303,10 @@ last statement in BODY, as elisp."
                       (mapc (lambda (line) (insert line) (funcall
send-wait))
                             (split-string body "[\r\n]"))
                       (funcall send-wait)))
+        (indented-p (org-babel-python--indented-p body))
+        (body (if indented-p
+                  (org-babel-python--replace-body-tmpfile body)
+                body))
          (results
           (pcase result-type
             (`output
@@ -340,6 +344,35 @@ last statement in BODY, as elisp."
       (substring string 1 -1)
     string))

+
+(defun org-babel-python--indented-p (input)
+     "Non-nil if any line in INPUT is indented."
+     (string-match-p "^[ \t]" input))
+
+(defun org-babel-python--replace-body-tmpfile (body)
+  "Place body in tmpfile, and return string to exec the tmpfile.
+If last line of body is not indented, place it at end of exec string
+instead of tmpfile, so shell can see the result"
+  (let* ((tmp-file (org-babel-temp-file "python-"))
+        (lines (split-string body "\n" t))
+        (lastline (car (last lines)))
+        (newbody (concat
+                  (format "__pyfilename = '%s'; " tmp-file)
+                  "__pyfile = open(__pyfilename); "
+                  "exec(compile("
+                  "__pyfile.read(), __pyfilename, 'exec'"
+                  ")); "
+                  "__pyfile.close()")))
+    (if (string-match-p "^[ \t]" lastline)
+       (progn
+         (with-temp-file tmp-file (insert body))
+         newbody)
+      (with-temp-file tmp-file
+       (insert (mapconcat 'identity
+                          (butlast lines) "\n")))
+      (concat newbody "\n" lastline)))
+  )
+
 (provide 'ob-python)


--
2.15.0


On Sat, Nov 18, 2017 at 3:05 PM, Kyle Meyer <kyle@kyleam.com> wrote:

> Hello,
>
> Jack Kamm <jackkamm@gmail.com> writes:
>
> > ob-python newline & indentation behavior in :session is very ugly and
> > possibly broken. For example, consider the following code block:
>
> [...]
>
> > There is a 2 year old patch that fixes this behavior but has not yet
> > been incorporated:
> > https://lists.gnu.org/archive/html/emacs-orgmode/2015-03/msg00505.html
>
> [...]
>
> > The patch follows the python.el behavior of using a temporary file and
> > executing that from the shell.
> >
> > Could this patch, or something similar, be incorporated into org-mode,
> > to fix this behavior?
>
> Here's what I said in a recent post about ob-python sessions:
>
>     https://lists.gnu.org/archive/html/emacs-orgmode/2017-10/msg00198.html
>
>     I dropped my attempt to fix it because 1) I was still having trouble
>     getting a complete understanding of what the issue was and 2) I didn't
>     have the motivation to spend time digging deeper because I don't use
>     ob-python (and in general am not a heavy Org-Babel user).  Perhaps you
>     or some other ob-python user could help make ob-python sessions more
>     robust?
>
> Perhaps you are the "you"?
>
> --
> Kyle
>

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

Re: ob-python newline & indentation behavior

[Martin Alsinet]

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

Hello Jack:

What versions of emacs and org-mode are you using?

I tried your example on Emacs 25.3.1 and org-mode 9.1.2-22 and I got "20"
as result
It has happened to me in the past that some bug I was seeing goes away just
by updating org-mode.

Regards,


Martin

On Sat, Nov 18, 2017 at 5:16 PM Jack Kamm <jackkamm@gmail.com> wrote:

> Thanks Kyle, and sorry for missing that recent related thread.
>
> I adapted your old patch to the current master branch. I also extended it
> to work for ":results value" (the original patch only worked for ":results
> output"). I did this by not writing the last line of the code block to the
> tmpfile, unless it is indented.
>
> I've never contributed before so would appreciate any comments on this
> patch, and how to get it accepted. Cheers, Jack.
>
> From a5d553ece9f6ee35cd1e273e554a21a19e80ec3c Mon Sep 17 00:00:00 2001
>
> From: Jack Kamm <jackkamm@gmail.com>
> Date: Sat, 18 Nov 2017 21:47:09 +0000
> Subject: [PATCH] fix newline/indentation issues in ob-python :session
>
> ---
>  lisp/ob-python.el | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
>
> diff --git a/lisp/ob-python.el b/lisp/ob-python.el
> index 60ec5fa47..6623ef5fc 100644
> --- a/lisp/ob-python.el
> +++ b/lisp/ob-python.el
> @@ -303,6 +303,10 @@ last statement in BODY, as elisp."
>                        (mapc (lambda (line) (insert line) (funcall
> send-wait))
>                              (split-string body "[\r\n]"))
>                        (funcall send-wait)))
> +        (indented-p (org-babel-python--indented-p body))
> +        (body (if indented-p
> +                  (org-babel-python--replace-body-tmpfile body)
> +                body))
>           (results
>            (pcase result-type
>              (`output
> @@ -340,6 +344,35 @@ last statement in BODY, as elisp."
>        (substring string 1 -1)
>      string))
>
> +
> +(defun org-babel-python--indented-p (input)
> +     "Non-nil if any line in INPUT is indented."
> +     (string-match-p "^[ \t]" input))
> +
> +(defun org-babel-python--replace-body-tmpfile (body)
> +  "Place body in tmpfile, and return string to exec the tmpfile.
> +If last line of body is not indented, place it at end of exec string
> +instead of tmpfile, so shell can see the result"
> +  (let* ((tmp-file (org-babel-temp-file "python-"))
> +        (lines (split-string body "\n" t))
> +        (lastline (car (last lines)))
> +        (newbody (concat
> +                  (format "__pyfilename = '%s'; " tmp-file)
> +                  "__pyfile = open(__pyfilename); "
> +                  "exec(compile("
> +                  "__pyfile.read(), __pyfilename, 'exec'"
> +                  ")); "
> +                  "__pyfile.close()")))
> +    (if (string-match-p "^[ \t]" lastline)
> +       (progn
> +         (with-temp-file tmp-file (insert body))
> +         newbody)
> +      (with-temp-file tmp-file
> +       (insert (mapconcat 'identity
> +                          (butlast lines) "\n")))
> +      (concat newbody "\n" lastline)))
> +  )
> +
>  (provide 'ob-python)
>
>
> --
> 2.15.0
>
>
> On Sat, Nov 18, 2017 at 3:05 PM, Kyle Meyer <kyle@kyleam.com> wrote:
>
>> Hello,
>>
>> Jack Kamm <jackkamm@gmail.com> writes:
>>
>> > ob-python newline & indentation behavior in :session is very ugly and
>> > possibly broken. For example, consider the following code block:
>>
>> [...]
>>
>> > There is a 2 year old patch that fixes this behavior but has not yet
>> > been incorporated:
>> > https://lists.gnu.org/archive/html/emacs-orgmode/2015-03/msg00505.html
>>
>> [...]
>>
>> > The patch follows the python.el behavior of using a temporary file and
>> > executing that from the shell.
>> >
>> > Could this patch, or something similar, be incorporated into org-mode,
>> > to fix this behavior?
>>
>> Here's what I said in a recent post about ob-python sessions:
>>
>>
>> https://lists.gnu.org/archive/html/emacs-orgmode/2017-10/msg00198.html
>>
>>     I dropped my attempt to fix it because 1) I was still having trouble
>>     getting a complete understanding of what the issue was and 2) I didn't
>>     have the motivation to spend time digging deeper because I don't use
>>     ob-python (and in general am not a heavy Org-Babel user).  Perhaps you
>>     or some other ob-python user could help make ob-python sessions more
>>     robust?
>>
>> Perhaps you are the "you"?
>>
>> --
>> Kyle
>>
>
>

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

Re: ob-python newline & indentation behavior

[Martin Alsinet]

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

Sorry Jack, I overlooked the :session bit.
Disregard my email please


On Sat, Nov 18, 2017 at 10:27 PM Martin Alsinet <martin@alsinet.com.ar>
wrote:

> Hello Jack:
>
> What versions of emacs and org-mode are you using?
>
> I tried your example on Emacs 25.3.1 and org-mode 9.1.2-22 and I got "20"
> as result
> It has happened to me in the past that some bug I was seeing goes away
> just by updating org-mode.
>
> Regards,
>
>
> Martin
>
> On Sat, Nov 18, 2017 at 5:16 PM Jack Kamm <jackkamm@gmail.com> wrote:
>
>> Thanks Kyle, and sorry for missing that recent related thread.
>>
>> I adapted your old patch to the current master branch. I also extended it
>> to work for ":results value" (the original patch only worked for ":results
>> output"). I did this by not writing the last line of the code block to the
>> tmpfile, unless it is indented.
>>
>> I've never contributed before so would appreciate any comments on this
>> patch, and how to get it accepted. Cheers, Jack.
>>
>> From a5d553ece9f6ee35cd1e273e554a21a19e80ec3c Mon Sep 17 00:00:00 2001
>>
>> From: Jack Kamm <jackkamm@gmail.com>
>> Date: Sat, 18 Nov 2017 21:47:09 +0000
>> Subject: [PATCH] fix newline/indentation issues in ob-python :session
>>
>> ---
>>  lisp/ob-python.el | 33 +++++++++++++++++++++++++++++++++
>>  1 file changed, 33 insertions(+)
>>
>> diff --git a/lisp/ob-python.el b/lisp/ob-python.el
>> index 60ec5fa47..6623ef5fc 100644
>> --- a/lisp/ob-python.el
>> +++ b/lisp/ob-python.el
>> @@ -303,6 +303,10 @@ last statement in BODY, as elisp."
>>                        (mapc (lambda (line) (insert line) (funcall
>> send-wait))
>>                              (split-string body "[\r\n]"))
>>                        (funcall send-wait)))
>> +        (indented-p (org-babel-python--indented-p body))
>> +        (body (if indented-p
>> +                  (org-babel-python--replace-body-tmpfile body)
>> +                body))
>>           (results
>>            (pcase result-type
>>              (`output
>> @@ -340,6 +344,35 @@ last statement in BODY, as elisp."
>>        (substring string 1 -1)
>>      string))
>>
>> +
>> +(defun org-babel-python--indented-p (input)
>> +     "Non-nil if any line in INPUT is indented."
>> +     (string-match-p "^[ \t]" input))
>> +
>> +(defun org-babel-python--replace-body-tmpfile (body)
>> +  "Place body in tmpfile, and return string to exec the tmpfile.
>> +If last line of body is not indented, place it at end of exec string
>> +instead of tmpfile, so shell can see the result"
>> +  (let* ((tmp-file (org-babel-temp-file "python-"))
>> +        (lines (split-string body "\n" t))
>> +        (lastline (car (last lines)))
>> +        (newbody (concat
>> +                  (format "__pyfilename = '%s'; " tmp-file)
>> +                  "__pyfile = open(__pyfilename); "
>> +                  "exec(compile("
>> +                  "__pyfile.read(), __pyfilename, 'exec'"
>> +                  ")); "
>> +                  "__pyfile.close()")))
>> +    (if (string-match-p "^[ \t]" lastline)
>> +       (progn
>> +         (with-temp-file tmp-file (insert body))
>> +         newbody)
>> +      (with-temp-file tmp-file
>> +       (insert (mapconcat 'identity
>> +                          (butlast lines) "\n")))
>> +      (concat newbody "\n" lastline)))
>> +  )
>> +
>>  (provide 'ob-python)
>>
>>
>> --
>> 2.15.0
>>
>>
>> On Sat, Nov 18, 2017 at 3:05 PM, Kyle Meyer <kyle@kyleam.com> wrote:
>>
>>> Hello,
>>>
>>> Jack Kamm <jackkamm@gmail.com> writes:
>>>
>>> > ob-python newline & indentation behavior in :session is very ugly and
>>> > possibly broken. For example, consider the following code block:
>>>
>>> [...]
>>>
>>> > There is a 2 year old patch that fixes this behavior but has not yet
>>> > been incorporated:
>>> > https://lists.gnu.org/archive/html/emacs-orgmode/2015-03/msg00505.html
>>>
>>> [...]
>>>
>>> > The patch follows the python.el behavior of using a temporary file and
>>> > executing that from the shell.
>>> >
>>> > Could this patch, or something similar, be incorporated into org-mode,
>>> > to fix this behavior?
>>>
>>> Here's what I said in a recent post about ob-python sessions:
>>>
>>>
>>> https://lists.gnu.org/archive/html/emacs-orgmode/2017-10/msg00198.html
>>>
>>>     I dropped my attempt to fix it because 1) I was still having trouble
>>>     getting a complete understanding of what the issue was and 2) I
>>> didn't
>>>     have the motivation to spend time digging deeper because I don't use
>>>     ob-python (and in general am not a heavy Org-Babel user).  Perhaps
>>> you
>>>     or some other ob-python user could help make ob-python sessions more
>>>     robust?
>>>
>>> Perhaps you are the "you"?
>>>
>>> --
>>> Kyle
>>>
>>
>>

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

Re: ob-python newline & indentation behavior

[Jack Kamm]

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

Sorry, I should have mentioned my version info anyways. I have tested on
emacs 25.3.1 and emacs 26.0.90, and org-mode versions 9.1.2 and 9.1.3
(current master).

The same error occurs on all emacs and org-mode versions. However the error
slightly differs between Python and IPython interpreters. IPython prints
"11" (instead of the expected "20"), whereas Python raises an
IndentationError then prints "10".

I've cleaned up my patch to deal with a few edge cases. In particular:
1) Fixed a bug where I was removing blank lines (these may be needed if
they are part of a multiline string object)
2) Send to tmpfile any multiline block even if not indented. This follows
the python.el behavior and gives more consistent behavior for ":results
output"
3) Allow ":results value" to work when the block ends in several newlines,
using string-trim-right

Here is the new version of the patch:

From f009da37d3b7e2730abb8cbb10f4d07b3d456dd8 Mon Sep 17 00:00:00 2001

From: Jack Kamm <jackkamm@gmail.com>
Date: Sun, 19 Nov 2017 07:13:56 +0000
Subject: [PATCH] Squashed commit of the following:

commit d1fe88a9f61a8e7082f08b7c190a29737bb655d5
Author: Jack Kamm <jackkamm@gmail.com>
Date:   Sun Nov 19 07:08:31 2017 +0000

    fix block ending in blank lines; send multiline blocks to tmpfile

commit fcc5a7795e882716775c9d925b0cd5b657da041b
Author: Jack Kamm <jackkamm@gmail.com>
Date:   Sat Nov 18 22:40:31 2017 +0000

    fix newlines and blanklines when sending codeblock to tmpfile

commit a5d553ece9f6ee35cd1e273e554a21a19e80ec3c
Author: Jack Kamm <jackkamm@gmail.com>
Date:   Sat Nov 18 21:47:09 2017 +0000

    fix newline/indentation issues in ob-python :session
---
 lisp/ob-python.el | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

diff --git a/lisp/ob-python.el b/lisp/ob-python.el
index 60ec5fa47..c3dba1565 100644
--- a/lisp/ob-python.el
+++ b/lisp/ob-python.el
@@ -303,6 +303,9 @@ last statement in BODY, as elisp."
                       (mapc (lambda (line) (insert line) (funcall
send-wait))
                             (split-string body "[\r\n]"))
                       (funcall send-wait)))
+        (body (if (string-match-p ".\n+." body) ;; Multiline
+                  (org-babel-python--replace-body-tmpfile body)
+                body))
          (results
           (pcase result-type
             (`output
@@ -340,6 +343,32 @@ last statement in BODY, as elisp."
       (substring string 1 -1)
     string))

+
+(defun org-babel-python--replace-body-tmpfile (body)
+  "Place body in tmpfile, and return string to exec the tmpfile.
+If last line of body is not indented, place it at end of exec string
+instead of tmpfile, so shell can see the result"
+  (let* ((body (string-trim-right body))
+        (tmp-file (org-babel-temp-file "python-"))
+        (lines (split-string body "[\r\n]"))
+        (lastline (car (last lines)))
+        (newbody (concat
+                  (format "__pyfilename = '%s'; " tmp-file)
+                  "__pyfile = open(__pyfilename); "
+                  "exec(compile("
+                  "__pyfile.read(), __pyfilename, 'exec'"
+                  ")); "
+                  "__pyfile.close()")))
+    (if (string-match-p "^[ \t]" lastline)
+       (progn
+         (with-temp-file tmp-file (insert body))
+         newbody)
+      (with-temp-file tmp-file
+       (insert (mapconcat 'identity
+                          (butlast lines) "\n")))
+      (concat newbody "\n" lastline)))
+  )
+
 (provide 'ob-python)


-- 
2.15.0




On Sun, Nov 19, 2017 at 3:34 AM, Martin Alsinet <martin@alsinet.com.ar>
wrote:

> Sorry Jack, I overlooked the :session bit.
> Disregard my email please
>
>
> On Sat, Nov 18, 2017 at 10:27 PM Martin Alsinet <martin@alsinet.com.ar>
> wrote:
>
>> Hello Jack:
>>
>> What versions of emacs and org-mode are you using?
>>
>> I tried your example on Emacs 25.3.1 and org-mode 9.1.2-22 and I got "20"
>> as result
>> It has happened to me in the past that some bug I was seeing goes away
>> just by updating org-mode.
>>
>> Regards,
>>
>>
>> Martin
>>
>> On Sat, Nov 18, 2017 at 5:16 PM Jack Kamm <jackkamm@gmail.com> wrote:
>>
>>> Thanks Kyle, and sorry for missing that recent related thread.
>>>
>>> I adapted your old patch to the current master branch. I also extended
>>> it to work for ":results value" (the original patch only worked for
>>> ":results output"). I did this by not writing the last line of the code
>>> block to the tmpfile, unless it is indented.
>>>
>>> I've never contributed before so would appreciate any comments on this
>>> patch, and how to get it accepted. Cheers, Jack.
>>>
>>> From a5d553ece9f6ee35cd1e273e554a21a19e80ec3c Mon Sep 17 00:00:00 2001
>>>
>>> From: Jack Kamm <jackkamm@gmail.com>
>>> Date: Sat, 18 Nov 2017 21:47:09 +0000
>>> Subject: [PATCH] fix newline/indentation issues in ob-python :session
>>>
>>> ---
>>>  lisp/ob-python.el | 33 +++++++++++++++++++++++++++++++++
>>>  1 file changed, 33 insertions(+)
>>>
>>> diff --git a/lisp/ob-python.el b/lisp/ob-python.el
>>> index 60ec5fa47..6623ef5fc 100644
>>> --- a/lisp/ob-python.el
>>> +++ b/lisp/ob-python.el
>>> @@ -303,6 +303,10 @@ last statement in BODY, as elisp."
>>>                        (mapc (lambda (line) (insert line) (funcall
>>> send-wait))
>>>                              (split-string body "[\r\n]"))
>>>                        (funcall send-wait)))
>>> +        (indented-p (org-babel-python--indented-p body))
>>> +        (body (if indented-p
>>> +                  (org-babel-python--replace-body-tmpfile body)
>>> +                body))
>>>           (results
>>>            (pcase result-type
>>>              (`output
>>> @@ -340,6 +344,35 @@ last statement in BODY, as elisp."
>>>        (substring string 1 -1)
>>>      string))
>>>
>>> +
>>> +(defun org-babel-python--indented-p (input)
>>> +     "Non-nil if any line in INPUT is indented."
>>> +     (string-match-p "^[ \t]" input))
>>> +
>>> +(defun org-babel-python--replace-body-tmpfile (body)
>>> +  "Place body in tmpfile, and return string to exec the tmpfile.
>>> +If last line of body is not indented, place it at end of exec string
>>> +instead of tmpfile, so shell can see the result"
>>> +  (let* ((tmp-file (org-babel-temp-file "python-"))
>>> +        (lines (split-string body "\n" t))
>>> +        (lastline (car (last lines)))
>>> +        (newbody (concat
>>> +                  (format "__pyfilename = '%s'; " tmp-file)
>>> +                  "__pyfile = open(__pyfilename); "
>>> +                  "exec(compile("
>>> +                  "__pyfile.read(), __pyfilename, 'exec'"
>>> +                  ")); "
>>> +                  "__pyfile.close()")))
>>> +    (if (string-match-p "^[ \t]" lastline)
>>> +       (progn
>>> +         (with-temp-file tmp-file (insert body))
>>> +         newbody)
>>> +      (with-temp-file tmp-file
>>> +       (insert (mapconcat 'identity
>>> +                          (butlast lines) "\n")))
>>> +      (concat newbody "\n" lastline)))
>>> +  )
>>> +
>>>  (provide 'ob-python)
>>>
>>>
>>> --
>>> 2.15.0
>>>
>>>
>>> On Sat, Nov 18, 2017 at 3:05 PM, Kyle Meyer <kyle@kyleam.com> wrote:
>>>
>>>> Hello,
>>>>
>>>> Jack Kamm <jackkamm@gmail.com> writes:
>>>>
>>>> > ob-python newline & indentation behavior in :session is very ugly and
>>>> > possibly broken. For example, consider the following code block:
>>>>
>>>> [...]
>>>>
>>>> > There is a 2 year old patch that fixes this behavior but has not yet
>>>> > been incorporated:
>>>> > https://lists.gnu.org/archive/html/emacs-orgmode/2015-03/
>>>> msg00505.html
>>>>
>>>> [...]
>>>>
>>>> > The patch follows the python.el behavior of using a temporary file and
>>>> > executing that from the shell.
>>>> >
>>>> > Could this patch, or something similar, be incorporated into org-mode,
>>>> > to fix this behavior?
>>>>
>>>> Here's what I said in a recent post about ob-python sessions:
>>>>
>>>>     https://lists.gnu.org/archive/html/emacs-orgmode/2017-10/
>>>> msg00198.html
>>>>
>>>>     I dropped my attempt to fix it because 1) I was still having trouble
>>>>     getting a complete understanding of what the issue was and 2) I
>>>> didn't
>>>>     have the motivation to spend time digging deeper because I don't use
>>>>     ob-python (and in general am not a heavy Org-Babel user).  Perhaps
>>>> you
>>>>     or some other ob-python user could help make ob-python sessions more
>>>>     robust?
>>>>
>>>> Perhaps you are the "you"?
>>>>
>>>> --
>>>> Kyle
>>>>
>>>
>>>

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

Re: ob-python newline & indentation behavior

[Kyle Meyer]

Jack Kamm <jackkamm@gmail.com> writes:

> Here is the new version of the patch:

I haven't had any luck applying this patch to master.  Perhaps your
email client is altering the inline patch; you can instead attach the
output file of 'git format-patch'.

> From f009da37d3b7e2730abb8cbb10f4d07b3d456dd8 Mon Sep 17 00:00:00 2001
>
> From: Jack Kamm <jackkamm@gmail.com>
> Date: Sun, 19 Nov 2017 07:13:56 +0000
> Subject: [PATCH] Squashed commit of the following:
>
> commit d1fe88a9f61a8e7082f08b7c190a29737bb655d5
> Author: Jack Kamm <jackkamm@gmail.com>
> Date:   Sun Nov 19 07:08:31 2017 +0000
>
>     fix block ending in blank lines; send multiline blocks to tmpfile
>
> commit fcc5a7795e882716775c9d925b0cd5b657da041b
> Author: Jack Kamm <jackkamm@gmail.com>
> Date:   Sat Nov 18 22:40:31 2017 +0000
>
>     fix newlines and blanklines when sending codeblock to tmpfile
>
> commit a5d553ece9f6ee35cd1e273e554a21a19e80ec3c
> Author: Jack Kamm <jackkamm@gmail.com>
> Date:   Sat Nov 18 21:47:09 2017 +0000
>
>     fix newline/indentation issues in ob-python :session

Please see <http://orgmode.org/worg/org-contribute.html#commit-messages>
for information on Org's convention for commit messages.  In addition to
following this format, ideally the message would contain a brief
description of the problem the patch is fixing.

> ---
>  lisp/ob-python.el | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)

This patch probably passes the TINYCHANGE threshold, so please see
<http://orgmode.org/worg/org-contribute.html#copyright-issues> for
information about assigning copyright.

> diff --git a/lisp/ob-python.el b/lisp/ob-python.el
> index 60ec5fa47..c3dba1565 100644
> --- a/lisp/ob-python.el
> +++ b/lisp/ob-python.el
> @@ -303,6 +303,9 @@ last statement in BODY, as elisp."
>                        (mapc (lambda (line) (insert line) (funcall
> send-wait))
>                              (split-string body "[\r\n]"))
>                        (funcall send-wait)))
> +        (body (if (string-match-p ".\n+." body) ;; Multiline

nitpick: When a comments is on the same line as code, the convention is
to use a single ";".

I see your point that sending all multiline code through a temporary
file is consistent with python.el's behavior.  When you say that it
"gives more consistent behavior for ":results output", I believe you're
talking about problems with ">>>" and "..."  markers leaking into the
output.  I agree that this change should be an improvement since I think
most of the prompt issues are racy problems related to sending multiple
lines.  (I think there still might be a unrelated prompt issue regarding
python.el's startup message leaking into the output of the first
executed block.)

Looking back at my summary of ob-python problems in
<https://lists.gnu.org/archive/html/emacs-orgmode/2015-03/msg00540.html>,
this seems like a good way to take the solution to problem #3
(indentation, multiline syntax errors) and apply it to problem #1
(markers leaking into the output).  Nice, hadn't occurred to me.

Assuming we do go this direction, I wonder if there's any ob-python code
that can be cleaned up or simplified since the multiline processing
logic is no longer needed.

> +                  (org-babel-python--replace-body-tmpfile body)
> +                body))
>           (results
>            (pcase result-type
>              (`output
> @@ -340,6 +343,32 @@ last statement in BODY, as elisp."
>        (substring string 1 -1)
>      string))
>
> +
> +(defun org-babel-python--replace-body-tmpfile (body)
> +  "Place body in tmpfile, and return string to exec the tmpfile.

nitpick: s/body/BODY/ here and below

> +If last line of body is not indented, place it at end of exec string
> +instead of tmpfile, so shell can see the result"

nitpick: missing trailing period

This is the part of the patch that I'm unsure about.  I don't like that
it

  * can fail if the last line is a non-indented, continued string

  * doesn't allow you to get a return value for commons things like
    conditionals, try-except blocks, context statements.

I can't think of a good solution, though.  Stepping back a bit, I think
it's unfortunate that python blocks handle ":results value" differently
depending on whether the block is hooked up to a session or not.  For
non-sessions, you have to use return.  Using the same approach
(org-babel-python-wrapper-method) for ":session :results value", we
could then get the return value reliably, but the problem with this
approach is that any variables defined in a ":results value" block
wouldn't be defined in the session after executing the block because the
code is wrapped in a function.

So at this point I think the choice is between

  * restricting the return value to unindented last lines and not
    supporting continued strings in the last line

  * wrapping the return value with org-babel-python-wrapper-method and
    not supporting persistent session variables in that block

  * not supporting ":session :results value"

> +  (let* ((body (string-trim-right body))
> +        (tmp-file (org-babel-temp-file "python-"))
> +        (lines (split-string body "[\r\n]"))
> +        (lastline (car (last lines)))
> +        (newbody (concat
> +                  (format "__pyfilename = '%s'; " tmp-file)

Since you're already using concat, I prefer to avoid the format here.
Or you could add a defconst that just defines that format string and
then only have a format call here.

> +                  "__pyfile = open(__pyfilename); "

Right, the leading underscores are good to reduce the risk of clobbering
a variable that the user may have defined in the shell.  Since this is
generated code, I wonder if you should be even more specific and use
ugly names like "__org_babel_python_fname" and "__org_babel_python_fh".

> +                  "exec(compile("
> +                  "__pyfile.read(), __pyfilename, 'exec'"
> +                  ")); "
> +                  "__pyfile.close()")))
> +    (if (string-match-p "^[ \t]" lastline)
> +       (progn
> +         (with-temp-file tmp-file (insert body))
> +         newbody)
> +      (with-temp-file tmp-file
> +       (insert (mapconcat 'identity
> +                          (butlast lines) "\n")))
> +      (concat newbody "\n" lastline)))
> +  )

nitpick: Move this trailing paren to the previous line.

Also, it'd be nice to add a few tests to testing/lisp/test-ob-python.el.

Thanks for working on this.

-- 
Kyle

Re: ob-python newline & indentation behavior

[Jack Kamm]

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

Hi Kyle,

In response to this:

I can't think of a good solution, though.  Stepping back a bit, I think
> it's unfortunate that python blocks handle ":results value" differently
> depending on whether the block is hooked up to a session or not.  For
> non-sessions, you have to use return.  Using the same approach
> (org-babel-python-wrapper-method) for ":session :results value", we
> could then get the return value reliably, but the problem with this
> approach is that any variables defined in a ":results value" block
> wouldn't be defined in the session after executing the block because the
> code is wrapped in a function.
>

How about if we used the "globals()" and "locals()" functions in Python?

Something like this at the end of the wrapper block, before return:

for k, v in locals().items():
    globals()[k] = v


I think this would work a lot better than the current approach.

Another bug with the current approach is that it breaks if common idioms
like "for _ in range(10)" are used. ("_" is used to inspect the last output
of the shell, an obscure feature I hadn't known about until now).

Thanks for reviewing my suggested changes. Might be a few days until I can
submit a new patch but I will incorporate your other suggestions. Waiting
for the FSF forms anyways.

Jack

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

Re: ob-python newline & indentation behavior

[Kyle Meyer]

Jack Kamm <jackkamm@gmail.com> writes:

> In response to this:
>
>> I can't think of a good solution, though.  Stepping back a bit, I think
>> it's unfortunate that python blocks handle ":results value" differently
>> depending on whether the block is hooked up to a session or not.  For
>> non-sessions, you have to use return.  Using the same approach
>> (org-babel-python-wrapper-method) for ":session :results value", we
>> could then get the return value reliably, but the problem with this
>> approach is that any variables defined in a ":results value" block
>> wouldn't be defined in the session after executing the block because the
>> code is wrapped in a function.
>
> How about if we used the "globals()" and "locals()" functions in Python?
>
> Something like this at the end of the wrapper block, before return:
>
> for k, v in locals().items():
>     globals()[k] = v

Hmm, placing that code "before return" is a problem.  Like with
non-session ":results value" blocks, the user would be responsible for
inserting the return (or even multiple return's), so we can't know where
to insert the above code without parsing the block :/

> Another bug with the current approach is that it breaks if common idioms
> like "for _ in range(10)" are used. ("_" is used to inspect the last output
> of the shell, an obscure feature I hadn't known about until now).

Right.  Also, IIRC the built-in interactive python and ipython treat
multiline blocks differently.  With

    if True:
        "ipython ignores my existence"

the built-in shell binds "_" to the string's value, but ipython doesn't.

-- 
Kyle

Re: ob-python newline & indentation behavior

[Jack Kamm]

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

Yes, I'm starting to see now how difficult it is to properly support
":session :results value". I would vote to remove it from ob-python...

I think the patch still improves ":session :results output" so I will
simplify it and restrict to that case, leaving ":session :results value"
unchanged for now.

Sorry for sending this twice Kyle, forgot to reply all.

On 21 Nov 2017 4:04 am, "Kyle Meyer" <kyle@kyleam.com> wrote:

> Jack Kamm <jackkamm@gmail.com> writes:
>
> > In response to this:
> >
> >> I can't think of a good solution, though.  Stepping back a bit, I think
> >> it's unfortunate that python blocks handle ":results value" differently
> >> depending on whether the block is hooked up to a session or not.  For
> >> non-sessions, you have to use return.  Using the same approach
> >> (org-babel-python-wrapper-method) for ":session :results value", we
> >> could then get the return value reliably, but the problem with this
> >> approach is that any variables defined in a ":results value" block
> >> wouldn't be defined in the session after executing the block because the
> >> code is wrapped in a function.
> >
> > How about if we used the "globals()" and "locals()" functions in Python?
> >
> > Something like this at the end of the wrapper block, before return:
> >
> > for k, v in locals().items():
> >     globals()[k] = v
>
> Hmm, placing that code "before return" is a problem.  Like with
> non-session ":results value" blocks, the user would be responsible for
> inserting the return (or even multiple return's), so we can't know where
> to insert the above code without parsing the block :/
>
> > Another bug with the current approach is that it breaks if common idioms
> > like "for _ in range(10)" are used. ("_" is used to inspect the last
> output
> > of the shell, an obscure feature I hadn't known about until now).
>
> Right.  Also, IIRC the built-in interactive python and ipython treat
> multiline blocks differently.  With
>
>     if True:
>         "ipython ignores my existence"
>
> the built-in shell binds "_" to the string's value, but ipython doesn't.
>
> --
> Kyle
>

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

Re: ob-python newline & indentation behavior

[Ista Zahn]

ob-ipython[1] provides a working alternative:

#+BEGIN_SRC jupyter-python :session :results output
  foo = 0
  for _ in range(10):
      foo += 1

      foo += 1

  print(foo)
#+END_SRC

#+RESULTS:
: 20

I've long wished that more org people would show ob-ipython some love.
Letting jupyter handle things on the backend seems like it should
simplifly things considerably.

[1] https://github.com/gregsexton/ob-ipython

Best,
Ista

On Tue, Nov 21, 2017 at 3:28 AM, Jack Kamm <jackkamm@gmail.com> wrote:
> Yes, I'm starting to see now how difficult it is to properly support
> ":session :results value". I would vote to remove it from ob-python...
>
> I think the patch still improves ":session :results output" so I will
> simplify it and restrict to that case, leaving ":session :results value"
> unchanged for now.
>
> Sorry for sending this twice Kyle, forgot to reply all.
>
> On 21 Nov 2017 4:04 am, "Kyle Meyer" <kyle@kyleam.com> wrote:
>>
>> Jack Kamm <jackkamm@gmail.com> writes:
>>
>> > In response to this:
>> >
>> >> I can't think of a good solution, though.  Stepping back a bit, I think
>> >> it's unfortunate that python blocks handle ":results value" differently
>> >> depending on whether the block is hooked up to a session or not.  For
>> >> non-sessions, you have to use return.  Using the same approach
>> >> (org-babel-python-wrapper-method) for ":session :results value", we
>> >> could then get the return value reliably, but the problem with this
>> >> approach is that any variables defined in a ":results value" block
>> >> wouldn't be defined in the session after executing the block because
>> >> the
>> >> code is wrapped in a function.
>> >
>> > How about if we used the "globals()" and "locals()" functions in Python?
>> >
>> > Something like this at the end of the wrapper block, before return:
>> >
>> > for k, v in locals().items():
>> >     globals()[k] = v
>>
>> Hmm, placing that code "before return" is a problem.  Like with
>> non-session ":results value" blocks, the user would be responsible for
>> inserting the return (or even multiple return's), so we can't know where
>> to insert the above code without parsing the block :/
>>
>> > Another bug with the current approach is that it breaks if common idioms
>> > like "for _ in range(10)" are used. ("_" is used to inspect the last
>> > output
>> > of the shell, an obscure feature I hadn't known about until now).
>>
>> Right.  Also, IIRC the built-in interactive python and ipython treat
>> multiline blocks differently.  With
>>
>>     if True:
>>         "ipython ignores my existence"
>>
>> the built-in shell binds "_" to the string's value, but ipython doesn't.
>>
>> --
>> Kyle

Re: ob-python newline & indentation behavior

[Jack Kamm]

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

ob-ipython has a bug which renders it basically unusable to me -- it
crashes when trying to interrupt the kernel with C-c C-c, which is
something I often find myself needing to do (see
https://github.com/gregsexton/ob-ipython/issues/115).

However I agree that ob-ipython is very promising and could lead to
org-babel becoming a viable jupyter notebook alternative in the future.
Having :async execution is especially cool, if somewhat buggy right now. I
hope ob-ipython continues to improve.

Nevertheless, I would like ob-python to work properly, regardless of
ob-ipython, as I like being able to use an python session in emacs without
jupyter.

On Sun, Nov 26, 2017 at 2:45 AM, Ista Zahn <istazahn@gmail.com> wrote:

> ob-ipython[1] provides a working alternative:
>
> #+BEGIN_SRC jupyter-python :session :results output
>   foo = 0
>   for _ in range(10):
>       foo += 1
>
>       foo += 1
>
>   print(foo)
> #+END_SRC
>
> #+RESULTS:
> : 20
>
> I've long wished that more org people would show ob-ipython some love.
> Letting jupyter handle things on the backend seems like it should
> simplifly things considerably.
>
> [1] https://github.com/gregsexton/ob-ipython
>
> Best,
> Ista
>
> On Tue, Nov 21, 2017 at 3:28 AM, Jack Kamm <jackkamm@gmail.com> wrote:
> > Yes, I'm starting to see now how difficult it is to properly support
> > ":session :results value". I would vote to remove it from ob-python...
> >
> > I think the patch still improves ":session :results output" so I will
> > simplify it and restrict to that case, leaving ":session :results value"
> > unchanged for now.
> >
> > Sorry for sending this twice Kyle, forgot to reply all.
> >
> > On 21 Nov 2017 4:04 am, "Kyle Meyer" <kyle@kyleam.com> wrote:
> >>
> >> Jack Kamm <jackkamm@gmail.com> writes:
> >>
> >> > In response to this:
> >> >
> >> >> I can't think of a good solution, though.  Stepping back a bit, I
> think
> >> >> it's unfortunate that python blocks handle ":results value"
> differently
> >> >> depending on whether the block is hooked up to a session or not.  For
> >> >> non-sessions, you have to use return.  Using the same approach
> >> >> (org-babel-python-wrapper-method) for ":session :results value", we
> >> >> could then get the return value reliably, but the problem with this
> >> >> approach is that any variables defined in a ":results value" block
> >> >> wouldn't be defined in the session after executing the block because
> >> >> the
> >> >> code is wrapped in a function.
> >> >
> >> > How about if we used the "globals()" and "locals()" functions in
> Python?
> >> >
> >> > Something like this at the end of the wrapper block, before return:
> >> >
> >> > for k, v in locals().items():
> >> >     globals()[k] = v
> >>
> >> Hmm, placing that code "before return" is a problem.  Like with
> >> non-session ":results value" blocks, the user would be responsible for
> >> inserting the return (or even multiple return's), so we can't know where
> >> to insert the above code without parsing the block :/
> >>
> >> > Another bug with the current approach is that it breaks if common
> idioms
> >> > like "for _ in range(10)" are used. ("_" is used to inspect the last
> >> > output
> >> > of the shell, an obscure feature I hadn't known about until now).
> >>
> >> Right.  Also, IIRC the built-in interactive python and ipython treat
> >> multiline blocks differently.  With
> >>
> >>     if True:
> >>         "ipython ignores my existence"
> >>
> >> the built-in shell binds "_" to the string's value, but ipython doesn't.
> >>
> >> --
> >> Kyle
>

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

Re: ob-python newline & indentation behavior

[Jack Kamm]

[-- Attachment #1.1: Type: text/plain, Size: 2849 bytes --]

Hello,

Attached please find my revised patch based on Kyle's feedback. I
simplified the behavior to send the whole block to the tmpfile (including
the lastline), but restricted this change to ":results output" only
(":results value" retains its old, broken behavior). I also added a test
and fixed minor style issues.

I haven't heard back from the FSF yet, any idea how long it takes to hear
from them?

Given how broken ":session :results value" is, and the difficulty in
implementing it correctly, I agree with Kyle's suggestion to remove it.
Should we start another thread to discuss this?

Jack

On Tue, Nov 21, 2017 at 8:28 AM, Jack Kamm <jackkamm@gmail.com> wrote:

> Yes, I'm starting to see now how difficult it is to properly support
> ":session :results value". I would vote to remove it from ob-python...
>
> I think the patch still improves ":session :results output" so I will
> simplify it and restrict to that case, leaving ":session :results value"
> unchanged for now.
>
> Sorry for sending this twice Kyle, forgot to reply all.
>
> On 21 Nov 2017 4:04 am, "Kyle Meyer" <kyle@kyleam.com> wrote:
>
>> Jack Kamm <jackkamm@gmail.com> writes:
>>
>> > In response to this:
>> >
>> >> I can't think of a good solution, though.  Stepping back a bit, I think
>> >> it's unfortunate that python blocks handle ":results value" differently
>> >> depending on whether the block is hooked up to a session or not.  For
>> >> non-sessions, you have to use return.  Using the same approach
>> >> (org-babel-python-wrapper-method) for ":session :results value", we
>> >> could then get the return value reliably, but the problem with this
>> >> approach is that any variables defined in a ":results value" block
>> >> wouldn't be defined in the session after executing the block because
>> the
>> >> code is wrapped in a function.
>> >
>> > How about if we used the "globals()" and "locals()" functions in Python?
>> >
>> > Something like this at the end of the wrapper block, before return:
>> >
>> > for k, v in locals().items():
>> >     globals()[k] = v
>>
>> Hmm, placing that code "before return" is a problem.  Like with
>> non-session ":results value" blocks, the user would be responsible for
>> inserting the return (or even multiple return's), so we can't know where
>> to insert the above code without parsing the block :/
>>
>> > Another bug with the current approach is that it breaks if common idioms
>> > like "for _ in range(10)" are used. ("_" is used to inspect the last
>> output
>> > of the shell, an obscure feature I hadn't known about until now).
>>
>> Right.  Also, IIRC the built-in interactive python and ipython treat
>> multiline blocks differently.  With
>>
>>     if True:
>>         "ipython ignores my existence"
>>
>> the built-in shell binds "_" to the string's value, but ipython doesn't.
>>
>> --
>> Kyle
>>
>

[-- Attachment #1.2: Type: text/html, Size: 4426 bytes --]

[-- Attachment #2: 0001-ob-python.el-fix-session-results-output-multiline-be.patch --]
[-- Type: text/x-patch, Size: 3376 bytes --]

From e46458004b83b034cb1388e707e866e84809b420 Mon Sep 17 00:00:00 2001
From: Jack Kamm <jackkamm@gmail.com>
Date: Sun, 26 Nov 2017 08:00:29 +0000
Subject: [PATCH] ob-python.el: fix :session :results output multiline behavior

* ob-python.el (orb-babel-python-evaluate-session
org-babel-python--exec-tmpfile): When :session :results output, send
multiline code blocks to tmpfile and execute in Python with exec()
* test-ob-python.el (test-ob-python/session-multiline): test for
:session with multiple lines and indentation
---
 lisp/ob-python.el              | 38 ++++++++++++++++++++++++++++----------
 testing/lisp/test-ob-python.el | 16 ++++++++++++++++
 2 files changed, 44 insertions(+), 10 deletions(-)

diff --git a/lisp/ob-python.el b/lisp/ob-python.el
index 60ec5fa47..3af5fb6bb 100644
--- a/lisp/ob-python.el
+++ b/lisp/ob-python.el
@@ -306,16 +306,19 @@ last statement in BODY, as elisp."
          (results
           (pcase result-type
             (`output
-             (mapconcat
-              #'org-trim
-              (butlast
-               (org-babel-comint-with-output
-                   (session org-babel-python-eoe-indicator t body)
-                 (funcall input-body body)
-                 (funcall send-wait) (funcall send-wait)
-                 (insert org-babel-python-eoe-indicator)
-                 (funcall send-wait))
-               2) "\n"))
+	     (let ((body (if (string-match-p ".\n+." body) ; Multiline
+			     (org-babel-python--replace-body-tmpfile body)
+			   body)))
+	       (mapconcat
+		#'org-trim
+		(butlast
+		 (org-babel-comint-with-output
+		     (session org-babel-python-eoe-indicator t body)
+		   (funcall input-body body)
+		   (funcall send-wait) (funcall send-wait)
+		   (insert org-babel-python-eoe-indicator)
+		   (funcall send-wait))
+		 2) "\n")))
             (`value
              (let ((tmp-file (org-babel-temp-file "python-")))
                (org-babel-comint-with-output
@@ -340,6 +343,21 @@ last statement in BODY, as elisp."
       (substring string 1 -1)
     string))
 
+(defconst org-babel-python--exec-tmpfile
+  (concat
+   "__org_babel_python_fname = '%s'; "
+   "__org_babel_python_fh = open(__org_babel_python_fname); "
+   "exec(compile("
+   "__org_babel_python_fh.read(), __org_babel_python_fname, 'exec'"
+   ")); "
+   "__org_babel_python_fh.close()"))
+
+(defun org-babel-python--replace-body-tmpfile (body)
+  "Place BODY in tmpfile, and return string to exec the tmpfile."
+  (let ((tmp-file (org-babel-temp-file "python-")))
+    (with-temp-file tmp-file (insert body))
+    (format org-babel-python--exec-tmpfile tmp-file)))
+
 (provide 'ob-python)
 
 
diff --git a/testing/lisp/test-ob-python.el b/testing/lisp/test-ob-python.el
index d9fca220f..e77f9f36c 100644
--- a/testing/lisp/test-ob-python.el
+++ b/testing/lisp/test-ob-python.el
@@ -101,6 +101,22 @@ return x
     (should (equal '(("col") ("a") ("b"))
 		   (org-babel-execute-src-block)))))
 
+(ert-deftest test-ob-python/session-multiline ()
+  (run-python)
+  (sleep-for 0 10)
+  (org-test-with-temp-text "
+#+begin_src python :session :results output
+  foo = 0
+  for _ in range(10):
+      foo += 1
+
+      foo += 1
+
+  print(foo)
+#+end_src"
+   (org-babel-next-src-block)
+   (should (equal "20" (org-babel-execute-src-block)))))
+
 (provide 'test-ob-python)
 
 ;;; test-ob-python.el ends here
-- 
2.15.0

Re: ob-python newline & indentation behavior

[Kyle Meyer]

Jack Kamm <jackkamm@gmail.com> writes:

> I haven't heard back from the FSF yet, any idea how long it takes to hear
> from them?

Hmm, according to the site below, a response should come within 5
business days.  That fits with my personal experience, but I'm not sure
what's typical.  Also, the delay might be Thanksgiving-related.

    <https://www.gnu.org/prep/maintain/html_node/Copyright-Papers.html>

> Given how broken ":session :results value" is, and the difficulty in
> implementing it correctly, I agree with Kyle's suggestion to remove it.
> Should we start another thread to discuss this?

I think that'd be good since 1) someone with an opinion on that topic
might not notice it within this thread and 2) it's now orthogonal to
this patch.

> Subject: [PATCH] ob-python.el: fix :session :results output multiline behavior

nit: To follow the convention used in the Org repo, "fix" would be
capitalized.

Also, just a preference, but the ".el" could be dropped.

So, combining those two, that'd be

    ob-python: Fix :session :results output multiline behavior

> * ob-python.el (orb-babel-python-evaluate-session

s|ob-python.el|lisp/&|

> org-babel-python--exec-tmpfile): When :session :results output, send
> multiline code blocks to tmpfile and execute in Python with exec()

nit: missing period

Because org-babel-python--exec-tmpfile is a new function, I think it
should be listed separately as

    (org-babel-python--exec-tmpfile): New function.

> * test-ob-python.el (test-ob-python/session-multiline): test for
> :session with multiple lines and indentation

s|test-ob-python.el|testing/lisp/&|

nits: s/test/Test/, missing period

> +(defconst org-babel-python--exec-tmpfile
> +  (concat
> +   "__org_babel_python_fname = '%s'; "
> +   "__org_babel_python_fh = open(__org_babel_python_fname); "
> +   "exec(compile("
> +   "__org_babel_python_fh.read(), __org_babel_python_fname, 'exec'"
> +   ")); "
> +   "__org_babel_python_fh.close()"))
> +

Looks good.  Not a big deal either way, but I'd prefer
org-babel-python--exec-tmpfile to be located directly below the other
defconst templates (org-babel-python-{,pp-}wrapper-method).

> +(defun org-babel-python--replace-body-tmpfile (body)
> +  "Place BODY in tmpfile, and return string to exec the tmpfile."
> +  (let ((tmp-file (org-babel-temp-file "python-")))
> +    (with-temp-file tmp-file (insert body))
> +    (format org-babel-python--exec-tmpfile tmp-file)))

If you do the defconst move above, this function should go along with
it.

Purely a style preference, but for a short function like this that's
only called in one place, I'd be in favor of dropping the defun and
putting the code directly in org-babel-python-evaluate-session.

> +(ert-deftest test-ob-python/session-multiline ()

Thanks.  Passes on my end (and of course fails if I keep the test but
revert the ob-python.el changes).

> +  (run-python)
> +  (sleep-for 0 10)

I suspect you added this to avoid the issue of the starting prompt
leaking into the output.  I think that guess is right because, when I
comment the two above lines out, it fails with an output string that
contains the Python shell's startup text.

I'm OK with this workaround, since the issue is unrelated to this patch,
but should a FIXME comment be added above the run-python call to explain
why it's there?

Thanks.

-- 
Kyle

Re: ob-python newline & indentation behavior

[John Kamm]

Kyle Meyer writes:

> Hmm, according to the site below, a response should come within 5
> business days.  That fits with my personal experience, but I'm not sure
> what's typical.  Also, the delay might be Thanksgiving-related.

FSF sent me the papers which I signed and returned about a week ago, am
just waiting for the final confirmation now.

>> Given how broken ":session :results value" is, and the difficulty in
>> implementing it correctly, I agree with Kyle's suggestion to remove it.
>> Should we start another thread to discuss this?
>
> I think that'd be good since 1) someone with an opinion on that topic
> might not notice it within this thread and 2) it's now orthogonal to
> this patch.

Great, I'll start another thread in a few days, summarizing the various
options you laid out before. Another idea I had was to have some special
"result" variable that could be assigned to. Anyways, we can discuss
more in the new thread.

> ... (suggestions to improve patch)

I've incorporated all your suggestions/improvements into the patch,
please see below. Thanks again for spending time to review it.


From 40e961e349fdb347fbf9d59b3aca58163749f804 Mon Sep 17 00:00:00 2001
From: Jack Kamm <jackkamm@gmail.com>
Date: Sat, 2 Dec 2017 09:03:00 +0000
Subject: [PATCH] ob-python: Fix :session :results output multiline behavior

* lisp/ob-python.el (orb-babel-python-evaluate-session): When :session
:results output, send multiline code blocks to tmpfile and execute in
Python with exec().
(org-babel-python--exec-tmpfile): New function.
* testing/lisp/test-ob-python.el (test-ob-python/session-multiline):
Test for :session with multiple lines and indentation.
---
 lisp/ob-python.el              | 36 ++++++++++++++++++++++++++----------
 testing/lisp/test-ob-python.el | 17 +++++++++++++++++
 2 files changed, 43 insertions(+), 10 deletions(-)

diff --git a/lisp/ob-python.el b/lisp/ob-python.el
index 60ec5fa47..b3f50cfe5 100644
--- a/lisp/ob-python.el
+++ b/lisp/ob-python.el
@@ -239,6 +239,15 @@ def main():
 
 open('%s', 'w').write( pprint.pformat(main()) )")
 
+(defconst org-babel-python--exec-tmpfile
+  (concat
+   "__org_babel_python_fname = '%s'; "
+   "__org_babel_python_fh = open(__org_babel_python_fname); "
+   "exec(compile("
+   "__org_babel_python_fh.read(), __org_babel_python_fname, 'exec'"
+   ")); "
+   "__org_babel_python_fh.close()"))
+
 (defun org-babel-python-evaluate
   (session body &optional result-type result-params preamble)
   "Evaluate BODY as Python code."
@@ -306,16 +315,23 @@ last statement in BODY, as elisp."
          (results
           (pcase result-type
             (`output
-             (mapconcat
-              #'org-trim
-              (butlast
-               (org-babel-comint-with-output
-                   (session org-babel-python-eoe-indicator t body)
-                 (funcall input-body body)
-                 (funcall send-wait) (funcall send-wait)
-                 (insert org-babel-python-eoe-indicator)
-                 (funcall send-wait))
-               2) "\n"))
+	     (let ((body (if (string-match-p ".\n+." body) ; Multiline
+			     (let ((tmp-src-file (org-babel-temp-file
+						  "python-")))
+			       (with-temp-file tmp-src-file (insert body))
+			       (format org-babel-python--exec-tmpfile
+				       tmp-src-file))
+			   body)))
+	       (mapconcat
+		#'org-trim
+		(butlast
+		 (org-babel-comint-with-output
+		     (session org-babel-python-eoe-indicator t body)
+		   (funcall input-body body)
+		   (funcall send-wait) (funcall send-wait)
+		   (insert org-babel-python-eoe-indicator)
+		   (funcall send-wait))
+		 2) "\n")))
             (`value
              (let ((tmp-file (org-babel-temp-file "python-")))
                (org-babel-comint-with-output
diff --git a/testing/lisp/test-ob-python.el b/testing/lisp/test-ob-python.el
index d9fca220f..915b1bc77 100644
--- a/testing/lisp/test-ob-python.el
+++ b/testing/lisp/test-ob-python.el
@@ -101,6 +101,23 @@ return x
     (should (equal '(("col") ("a") ("b"))
 		   (org-babel-execute-src-block)))))
 
+(ert-deftest test-ob-python/session-multiline ()
+  ;; FIXME workaround to prevent starting prompt leaking into output
+  (run-python)
+  (sleep-for 0 10)
+  (org-test-with-temp-text "
+#+begin_src python :session :results output
+  foo = 0
+  for _ in range(10):
+      foo += 1
+
+      foo += 1
+
+  print(foo)
+#+end_src"
+   (org-babel-next-src-block)
+   (should (equal "20" (org-babel-execute-src-block)))))
+
 (provide 'test-ob-python)
 
 ;;; test-ob-python.el ends here
-- 
2.15.1

Re: ob-python newline & indentation behavior

[Jack Kamm]

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


FSF sent me the signed papers :) I've attached them.

Could Kyle or someone else merge in the patch, which I am reattaching?

Thanks,
Jack


[-- Attachment #2: Kamm.1253922.EMACS.pdf --]
[-- Type: application/pdf, Size: 1434020 bytes --]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0001-ob-python-Fix-session-results-output-multiline-behav.patch --]
[-- Type: text/x-patch, Size: 3441 bytes --]

From bab63ad0941f45333208a8b690a38c0f51b568be Mon Sep 17 00:00:00 2001
From: Jack Kamm <jackkamm@gmail.com>
Date: Sat, 2 Dec 2017 09:03:00 +0000
Subject: [PATCH] ob-python: Fix :session :results output multiline behavior

* lisp/ob-python.el (orb-babel-python-evaluate-session): When :session
:results output, send multiline code blocks to tmpfile and execute in
Python with exec().
(org-babel-python--exec-tmpfile): New function.
* testing/lisp/test-ob-python.el (test-ob-python/session-multiline):
Test for :session with multiple lines and indentation.
---
 lisp/ob-python.el              | 36 ++++++++++++++++++++++++++----------
 testing/lisp/test-ob-python.el | 17 +++++++++++++++++
 2 files changed, 43 insertions(+), 10 deletions(-)

diff --git a/lisp/ob-python.el b/lisp/ob-python.el
index 60ec5fa47..b3f50cfe5 100644
--- a/lisp/ob-python.el
+++ b/lisp/ob-python.el
@@ -239,6 +239,15 @@ def main():
 
 open('%s', 'w').write( pprint.pformat(main()) )")
 
+(defconst org-babel-python--exec-tmpfile
+  (concat
+   "__org_babel_python_fname = '%s'; "
+   "__org_babel_python_fh = open(__org_babel_python_fname); "
+   "exec(compile("
+   "__org_babel_python_fh.read(), __org_babel_python_fname, 'exec'"
+   ")); "
+   "__org_babel_python_fh.close()"))
+
 (defun org-babel-python-evaluate
   (session body &optional result-type result-params preamble)
   "Evaluate BODY as Python code."
@@ -306,16 +315,23 @@ last statement in BODY, as elisp."
          (results
           (pcase result-type
             (`output
-             (mapconcat
-              #'org-trim
-              (butlast
-               (org-babel-comint-with-output
-                   (session org-babel-python-eoe-indicator t body)
-                 (funcall input-body body)
-                 (funcall send-wait) (funcall send-wait)
-                 (insert org-babel-python-eoe-indicator)
-                 (funcall send-wait))
-               2) "\n"))
+	     (let ((body (if (string-match-p ".\n+." body) ; Multiline
+			     (let ((tmp-src-file (org-babel-temp-file
+						  "python-")))
+			       (with-temp-file tmp-src-file (insert body))
+			       (format org-babel-python--exec-tmpfile
+				       tmp-src-file))
+			   body)))
+	       (mapconcat
+		#'org-trim
+		(butlast
+		 (org-babel-comint-with-output
+		     (session org-babel-python-eoe-indicator t body)
+		   (funcall input-body body)
+		   (funcall send-wait) (funcall send-wait)
+		   (insert org-babel-python-eoe-indicator)
+		   (funcall send-wait))
+		 2) "\n")))
             (`value
              (let ((tmp-file (org-babel-temp-file "python-")))
                (org-babel-comint-with-output
diff --git a/testing/lisp/test-ob-python.el b/testing/lisp/test-ob-python.el
index d9fca220f..915b1bc77 100644
--- a/testing/lisp/test-ob-python.el
+++ b/testing/lisp/test-ob-python.el
@@ -101,6 +101,23 @@ return x
     (should (equal '(("col") ("a") ("b"))
 		   (org-babel-execute-src-block)))))
 
+(ert-deftest test-ob-python/session-multiline ()
+  ;; FIXME workaround to prevent starting prompt leaking into output
+  (run-python)
+  (sleep-for 0 10)
+  (org-test-with-temp-text "
+#+begin_src python :session :results output
+  foo = 0
+  for _ in range(10):
+      foo += 1
+
+      foo += 1
+
+  print(foo)
+#+end_src"
+   (org-babel-next-src-block)
+   (should (equal "20" (org-babel-execute-src-block)))))
+
 (provide 'test-ob-python)
 
 ;;; test-ob-python.el ends here
-- 
2.15.1

Re: ob-python newline & indentation behavior

[Kyle Meyer]

Jack Kamm <jackkamm@gmail.com> writes:

> FSF sent me the signed papers :) I've attached them.

great :)

> Could Kyle or someone else merge in the patch, which I am reattaching?

I'll take a final look tonight (est).

Re: ob-python newline & indentation behavior

[Kyle Meyer]

Kyle Meyer <kyle@kyleam.com> writes:

> Jack Kamm <jackkamm@gmail.com> writes:
>
>> FSF sent me the signed papers :) I've attached them.
>
> great :)

I've added your name here:
http://orgmode.org/worg/org-contribute.html#contributors_with_fsf_papers

>> Could Kyle or someone else merge in the patch, which I am reattaching?
>
> I'll take a final look tonight (est).

Applied.  Thanks for your work!

-- 
Kyle

Re: ob-python newline & indentation behavior

[Kyle Meyer]

Jack Kamm <jackkamm@gmail.com> writes:

> I adapted your old patch to the current master branch. I also extended it
> to work for ":results value" (the original patch only worked for ":results
> output"). I did this by not writing the last line of the code block to the
> tmpfile, unless it is indented.
>
> I've never contributed before so would appreciate any comments on this
> patch, and how to get it accepted. Cheers, Jack.

Thanks for picking this up.  I'll find time in the next couple of days
to review your patch.  For general information about contributing, take
a look at http://orgmode.org/worg/org-contribute.html

-- 
Kyle