Re: [O] ob-python newline & indentation behavior

[Kyle Meyer]

Kyle Meyer  writes:

> Jack Kamm  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: [O] ob-python newline & indentation behavior

[Kyle Meyer]

Jack Kamm  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: [O] 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 
Date: Sat, 2 Dec 2017 09:03:00 +
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  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: [O] ob-python newline & indentation behavior

[Kyle Meyer]

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



> 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: [O] ob-python newline & indentation behavior

[Jack Kamm]

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  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  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"  wrote:
> >>
> >> Jack Kamm  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: [O] ob-python newline & indentation behavior

[Jack Kamm]

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  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"  wrote:
>
>> Jack Kamm  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
>>
>
From e46458004b83b034cb1388e707e866e84809b420 Mon Sep 17 00:00:00 2001
From: Jack Kamm 
Date: Sun, 26 Nov 2017 08:00:29 +
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'; "
+ 

Re: [O] 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  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"  wrote:
>>
>> Jack Kamm  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: [O] ob-python newline & indentation behavior

[Jack Kamm]

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"  wrote:

> Jack Kamm  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: [O] ob-python newline & indentation behavior

[Kyle Meyer]

Jack Kamm  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: [O] ob-python newline & indentation behavior

[Jack Kamm]

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

Re: [O] ob-python newline & indentation behavior

[Kyle Meyer]

Jack Kamm  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 
> Date: Sun, 19 Nov 2017 07:13:56 +
> Subject: [PATCH] Squashed commit of the following:
>
> commit d1fe88a9f61a8e7082f08b7c190a29737bb655d5
> Author: Jack Kamm 
> Date:   Sun Nov 19 07:08:31 2017 +
>
> fix block ending in blank lines; send multiline blocks to tmpfile
>
> commit fcc5a7795e882716775c9d925b0cd5b657da041b
> Author: Jack Kamm 
> Date:   Sat Nov 18 22:40:31 2017 +
>
> fix newlines and blanklines when sending codeblock to tmpfile
>
> commit a5d553ece9f6ee35cd1e273e554a21a19e80ec3c
> Author: Jack Kamm 
> Date:   Sat Nov 18 21:47:09 2017 +
>
> fix newline/indentation issues in ob-python :session

Please see 
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
 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
,
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 

Re: [O] ob-python newline & indentation behavior

[Kyle Meyer]

Jack Kamm  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

Re: [O] ob-python newline & indentation behavior

[Jack Kamm]

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 
Date: Sun, 19 Nov 2017 07:13:56 +
Subject: [PATCH] Squashed commit of the following:

commit d1fe88a9f61a8e7082f08b7c190a29737bb655d5
Author: Jack Kamm 
Date:   Sun Nov 19 07:08:31 2017 +

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

commit fcc5a7795e882716775c9d925b0cd5b657da041b
Author: Jack Kamm 
Date:   Sat Nov 18 22:40:31 2017 +

fix newlines and blanklines when sending codeblock to tmpfile

commit a5d553ece9f6ee35cd1e273e554a21a19e80ec3c
Author: Jack Kamm 
Date:   Sat Nov 18 21:47:09 2017 +

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 
wrote:

> Sorry Jack, I overlooked the :session bit.
> Disregard my email please
>
>
> On Sat, Nov 18, 2017 at 10:27 PM Martin Alsinet 
> 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  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 
>>> Date: Sat, 18 Nov 2017 21:47:09 +
>>> 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 

Re: [O] ob-python newline & indentation behavior

[Martin Alsinet]

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


On Sat, Nov 18, 2017 at 10:27 PM Martin Alsinet 
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  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 
>> Date: Sat, 18 Nov 2017 21:47:09 +
>> 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  wrote:
>>
>>> Hello,
>>>
>>> Jack Kamm  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: [O] ob-python newline & indentation behavior

[Martin Alsinet]

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  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 
> Date: Sat, 18 Nov 2017 21:47:09 +
> 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  wrote:
>
>> Hello,
>>
>> Jack Kamm  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: [O] ob-python newline & indentation behavior

[Jack Kamm]

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 
Date: Sat, 18 Nov 2017 21:47:09 +
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  wrote:

> Hello,
>
> Jack Kamm  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: [O] ob-python newline & indentation behavior

[Kyle Meyer]

Hello,

Jack Kamm  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

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