From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jack Kamm Subject: Re: [PATCH] Fix several issues with python session value blocks Date: Mon, 03 Feb 2020 18:27:37 -0800 Message-ID: <87eevbcciu.fsf@gmail.com> References: <87k159fq7r.fsf@kyleam.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" Return-path: Received: from eggs.gnu.org ([2001:470:142:3::10]:49301) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1iynwI-00065R-0a for emacs-orgmode@gnu.org; Mon, 03 Feb 2020 21:27:47 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1iynwF-00075O-Vz for emacs-orgmode@gnu.org; Mon, 03 Feb 2020 21:27:45 -0500 Received: from mail-pf1-x443.google.com ([2607:f8b0:4864:20::443]:39924) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1iynwF-0006pL-Lc for emacs-orgmode@gnu.org; Mon, 03 Feb 2020 21:27:43 -0500 Received: by mail-pf1-x443.google.com with SMTP id 84so8633358pfy.6 for ; Mon, 03 Feb 2020 18:27:43 -0800 (PST) In-Reply-To: <87k159fq7r.fsf@kyleam.com> List-Id: "General discussions about Org-mode." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-orgmode-bounces+geo-emacs-orgmode=m.gmane-mx.org@gnu.org Sender: "Emacs-orgmode" To: Kyle Meyer , emacs-orgmode@gnu.org --=-=-= Content-Type: text/plain Hi Kyle, Thanks for the thorough review as always. An updated patch incorporating your feedback is at the end of this email. I'd like to do the honors of making my first push to the repo :) So please let me know if you're fine with me to proceed. Or, if you have more comments, they are always appreciated. Kyle Meyer writes: > Tangent: You can get better context for your diffs if you define a > custom xfuncname. Thanks for the tip! >> +with open('%s') as f: > > Hmm, I'm nervous about breakage here if org-babel-temp-file returns > something with a single quote. However, that's already a problem with > org-babel-python--exec-tmpfile used for ":results output", as well as > with a couple of other spots, so I think it'd be okay to punt on that > for now. Thanks for pointing this out, I've noted it and will try to fix it in future. > Hmm, we set the result to the exception on error, so the exception will > now show up under "#+RESULTS:". As a not-really-babel user, my guess is > that that'd be a good thing, but I do wonder how other languages handle > exceptions. I'm most familiar with R, Julia, and shell so I checked how those languages deal with errors when ":session :results value". In general, they print an empty "#+RESULTS:" element, with the exception of Julia which hangs Emacs. So, printing an empty results block would be the more consistent behavior here. This can be achieved by setting __org_babel_python_final to the empty string in the exception clause. On the other hand, I do think that printing the traceback is useful behavior. For now, I've opted to keep the traceback output, because we've already written it, and it's easy to remove later if we want. But I don't have a strong opinion here. > I've included a patch at the end that sits on top of yours and does > those two things. If it looks reasonable to you, please squash it into > your patch. Looks good, I've squashed it in, however I did add back a call to "raise" in the exception block, so that the error would appear in the REPL (which was the general behavior for shell, R, and Julia errors). --=-=-= Content-Type: text/x-patch Content-Disposition: inline; filename=0001-ob-python-Fix-several-issues-with-session-results-va.patch >From 572ca9fd8c89720acd8d7d2ace8bb3c0be3d288e Mon Sep 17 00:00:00 2001 From: Jack Kamm Date: Mon, 20 Jan 2020 17:40:22 -0800 Subject: [PATCH] ob-python: Fix several issues with :session :results value * lisp/ob-python.el (org-babel-python-evaluate-session): Fix a few related issues with :session :results value blocks, including broken if-else statements, indented blocks with blank lines, and returning the wrong value when underscore has been used. (org-babel-python--eval-ast): New constant variable, a string consisting of Python code to execute a source block using ast. Previously, python blocks with parameters ":session :results value" were entered line-by-line into the Python session, which could cause issues around indentation and new lines. Now, such python blocks are written to temp files, then the built-in ast python module is used to parse and execute them, and to extract the last line separately to return as a result. Introduces a change in behavior, requiring that the last line must be a top-level expression statement if its result is to be saved (otherwise, the result is None). --- etc/ORG-NEWS | 9 +++++ lisp/ob-python.el | 68 ++++++++++++++++++++-------------- testing/lisp/test-ob-python.el | 35 +++++++++++++++++ 3 files changed, 85 insertions(+), 27 deletions(-) diff --git a/etc/ORG-NEWS b/etc/ORG-NEWS index 6518c318d..2068b3aab 100644 --- a/etc/ORG-NEWS +++ b/etc/ORG-NEWS @@ -11,6 +11,15 @@ See the end of the file for license conditions. Please send Org bug reports to mailto:emacs-orgmode@gnu.org. * Version 9.4 (not yet released) +** Incompatible changes +*** Python session return values must be top-level expression statements + +Python blocks with ~:session :results value~ header arguments now only +return a value if the last line is a top-level expression statement, +otherwise the result is None. Also, None will now show up under +"#+RESULTS:", as it already did with ~:results value~ for non-session +blocks. + ** New features *** Numeric priorities are now allowed (up to 65) diff --git a/lisp/ob-python.el b/lisp/ob-python.el index 823f6e63d..5f71577cb 100644 --- a/lisp/ob-python.el +++ b/lisp/ob-python.el @@ -247,6 +247,25 @@ open('%s', 'w').write( pprint.pformat(main()) )") ")); " "__org_babel_python_fh.close()")) +(defconst org-babel-python--eval-ast "\ +import ast +try: + with open('%s') as f: + __org_babel_python_ast = ast.parse(f.read()) + __org_babel_python_final = __org_babel_python_ast.body[-1] + if isinstance(__org_babel_python_final, ast.Expr): + __org_babel_python_ast.body = __org_babel_python_ast.body[:-1] + exec(compile(__org_babel_python_ast, '', 'exec')) + __org_babel_python_final = eval(compile(ast.Expression( + __org_babel_python_final.value), '', 'eval')) + else: + exec(compile(__org_babel_python_ast, '', 'exec')) + __org_babel_python_final = None +except Exception: + from traceback import format_exc + __org_babel_python_final = format_exc() + raise") + (defun org-babel-python-evaluate (session body &optional result-type result-params preamble) "Evaluate BODY as Python code." @@ -294,32 +313,9 @@ If RESULT-TYPE equals `output' then return standard output as a string. If RESULT-TYPE equals `value' then return the value of the last statement in BODY, as elisp." (let* ((send-wait (lambda () (comint-send-input nil t) (sleep-for 0 5))) - (dump-last-value - (lambda - (tmp-file pp) - (mapc - (lambda (statement) (insert statement) (funcall send-wait)) - (if pp - (list - "import pprint" - (format "open('%s', 'w').write(pprint.pformat(_))" - (org-babel-process-file-name tmp-file 'noquote))) - (list (format "open('%s', 'w').write(str(_))" - (org-babel-process-file-name tmp-file - 'noquote))))))) (last-indent 0) (input-body (lambda (body) (dolist (line (split-string body "[\r\n]")) - ;; Insert a blank line to end an indent - ;; block. - (let ((curr-indent (string-match "\\S-" line))) - (if curr-indent - (progn - (when (< curr-indent last-indent) - (insert "") - (funcall send-wait)) - (setq last-indent curr-indent)) - (setq last-indent 0))) (insert line) (funcall send-wait)) (funcall send-wait))) @@ -344,17 +340,35 @@ last statement in BODY, as elisp." (funcall send-wait)) 2) "\n"))) (`value - (let ((tmp-file (org-babel-temp-file "python-"))) + (let ((tmp-results-file (org-babel-temp-file "python-")) + (body (let ((tmp-src-file (org-babel-temp-file + "python-"))) + (with-temp-file tmp-src-file (insert body)) + (format org-babel-python--eval-ast + tmp-src-file)))) (org-babel-comint-with-output (session org-babel-python-eoe-indicator nil body) (let ((comint-process-echoes nil)) (funcall input-body body) - (funcall dump-last-value tmp-file - (member "pp" result-params)) + (dolist + (statement + (if (member "pp" result-params) + (list + "import pprint" + (format "open('%s', 'w').write(pprint.pformat(\ +__org_babel_python_final))" + (org-babel-process-file-name + tmp-results-file 'noquote))) + (list (format "open('%s', 'w').write(str(\ +__org_babel_python_final))" + (org-babel-process-file-name + tmp-results-file 'noquote))))) + (insert statement) + (funcall send-wait)) (funcall send-wait) (funcall send-wait) (insert org-babel-python-eoe-indicator) (funcall send-wait))) - (org-babel-eval-read-file tmp-file)))))) + (org-babel-eval-read-file tmp-results-file)))))) (unless (string= (substring org-babel-python-eoe-indicator 1 -1) results) (org-babel-result-cond result-params results diff --git a/testing/lisp/test-ob-python.el b/testing/lisp/test-ob-python.el index 48ca3d640..7e2826404 100644 --- a/testing/lisp/test-ob-python.el +++ b/testing/lisp/test-ob-python.el @@ -138,6 +138,41 @@ if True: (org-babel-execute-maybe) (org-babel-execute-src-block))))) +(ert-deftest test-ob-python/if-else-block () + (should + (equal "success" (org-test-with-temp-text "#+begin_src python :session :results value +value = 'failure' +if False: + pass +else: + value = 'success' +value +#+end_src" + (org-babel-execute-src-block))))) + +(ert-deftest test-ob-python/indent-block-with-blank-lines () + (should + (equal 20 + (org-test-with-temp-text "#+begin_src python :session :results value + foo = 0 + for i in range(10): + foo += 1 + + foo += 1 + + foo +#+end_src" + (org-babel-execute-src-block))))) + +(ert-deftest test-ob-python/assign-underscore () + (should + (equal "success" + (org-test-with-temp-text "#+begin_src python :session :results value +_ = 'failure' +'success' +#+end_src" + (org-babel-execute-src-block))))) + (provide 'test-ob-python) ;;; test-ob-python.el ends here -- 2.25.0 --=-=-=--