From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Charles C. Berry" Subject: Re: R code block produces only partial output Date: Sat, 23 Aug 2014 17:10:16 -0700 Message-ID: References: <87iom8zd24.fsf@gmail.com> <877g2oz9gv.fsf@gmail.com> <87lhr27oap.fsf@gmail.com> <87r40uwavs.fsf@gmail.com> <8761i5kg8f.fsf@gmail.com> <87ppgcrg8n.fsf@gmail.com> <87lhr0qimr.fsf@gmail.com> <87wqa9owhv.fsf@gmail.com> <87oavkp2xa.fsf@gmail.com> Mime-Version: 1.0 Content-Type: MULTIPART/MIXED; BOUNDARY="0-1602133763-1408839016=:2651" Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:46610) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XLLO6-0004y7-Jc for emacs-orgmode@gnu.org; Sat, 23 Aug 2014 20:10:33 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XLLNz-00078P-Ea for emacs-orgmode@gnu.org; Sat, 23 Aug 2014 20:10:26 -0400 Received: from iport-acv2-out.ucsd.edu ([132.239.0.174]:24892) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XLLNy-00074b-V6 for emacs-orgmode@gnu.org; Sat, 23 Aug 2014 20:10:19 -0400 In-Reply-To: <87oavkp2xa.fsf@gmail.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.org@gnu.org Sender: emacs-orgmode-bounces+geo-emacs-orgmode=m.gmane.org@gnu.org To: Aaron Ecay Cc: emacs-orgmode@gnu.org This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. --0-1602133763-1408839016=:2651 Content-Type: TEXT/PLAIN; charset=utf-8; format=flowed Content-Transfer-Encoding: 8BIT Aaron, Some comments inline. A patch that modifies what you have done is attached. Apart from adding `org-babel-local-file-name' in two places, I think it is functionally that same as yours, but I find the R code easier to follow. On Sat, 16 Aug 2014, Aaron Ecay wrote: > Hi Chuck, > > Thanks for your feedback. > > 2014ko abuztuak 16an, "Charles C. Berry"-ek idatzi zuen: >> Aaron, >> >> I think doing something along these lines makes sense. evaluate() obviates >> the need for tryCatch() and capture.output(), and it makes customizing >> error/warning/message stuff clean. Its use might also lead to cleaner code >> for R graphics handling. >> >> But adding a dependency on the evaluate package is a significant step. >> This could be a nuisance for users whose code runs just fine right now. It >> would need to be installed anywhere R is executed, e.g. in remote >> sessions. The user would need to install it in her private directory if it >> is not on the system; some shops actually discourage this. > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > That’s ... special. Do you have experience with such environments? Not really. Just seen complaints... > >> >> So, making it optional might be necessary. > > I don’t like this idea; it means that we’d have to support the old, > hacky way of doing things indefinitely. I’d like to hear more from > people for whom local installation of R packages creates an issue. > The old hacky way works pretty well most of the time. I suspect that it will be hard to get folks to really test a new ob-R.el in advance of its moving to maint. ./lisp/test-ob-R.el only has one :session src block and a few other src blocks... I was thinking of having a drop in replacement for the each of the two functions that are changed and maybe selecting which version to use with defalias. If there is a slicker way to enable a user to revert to an earlier version without having good git skills, fine. But I think the possibility of breaking something that someone needs and not noticing till the changes go to maint is high. >> >> --- >> >> I looked at the patch briefly. Some comments: >> >> - You can ditch tryCatch and capture.output. You might browse >> knitr:::block_exec to see how it uses evaluate(). > > I’m not sure I see what you mean. The tryCatch exists to ensure that > the sentinel file is created, to signal emacs that the R code is done > running. It might not be strictly necessary, as long as the rest of > babel’s injected code is error-free (evaluate takes care of catching > errors in user code). But emacs will busy-wait indefinitely if the > creation of that file does not happen, so I have tried to play it safe. > Not sure I get why the sentinel file is needed, but the patch here uses on.exit(file.create(...)) to ensure that that file is created. One hiccup (not sure if it exists in master,too) is that starting a remote session and then trying to run src blocks from a buffer for a local file will hang (because a local temp file is used for sentinel). So there is still stuff to do. > As for capture.output, the knitr function you reference is doing a lot > of heavy lifting; I don’t understand it all. I just need to get the > result of evaluate() into a file somehow. There are other ways of doing > this than capture.output + replay, but it seems like they’d be just as > complicated. After looking more at evaluate and that knitr function, I am not keen to replace replay() to obviate sink() or capture.output(). I had thought that the output_handler arg offered more potential that it actually does. If you ever feel the need to revise how graphics handling is done, it might be worth working thru what Yihui does there. > >> >> - Wrap the code in local() to keep objects you create from persisting >> where they might not be wanted. I think using >> local({ res <- evaluate(input, envir=parent.frame(2),...); <...>}) >> will get assignments from `input' properly placed. > > I omitted to rm() .org.eval.result for debugging purposes, but in the > final patch I will do so. I much prefer an explicit variable creation / > rm() to environment-hacking. > Putting objects in userspace is considered poor practice. FWIW, CRAN disallows it. `environment-hacking' as you call it underlies much of R. If you must create and hold variables use the attach(NULL,name="org-vars") trick. >> >> - let stop_on_error, keep_warning, and keep_message args be customizable >> or depend on a header arg. (Then I can stop wrapping require() in >> suppressPackageStartupMessages() which I always misspell. Argh!) > > I don’t like this, since it would not carry over to the other 3/4 > cases (value results in a session; either type of result outside of a > session). I’d like to get this patch working first, and then focus on > the others. It may be that we want to move to using the evaluate > package in all cases, in which case this suggestion would be workable. Fair enough. Best, Chuck --0-1602133763-1408839016=:2651 Content-Type: TEXT/PLAIN; charset=US-ASCII; name=0002-merge-CCBs-version.patch Content-Transfer-Encoding: BASE64 Content-ID: Content-Description: modify AE's session-results-output patch Content-Disposition: attachment; filename=0002-merge-CCBs-version.patch RnJvbSBlNTFjMDkyY2M0NTVmOWRlZWU4ZDNkNWI0MTM0OTNhZmY4NGIzOGMz IE1vbiBTZXAgMTcgMDA6MDA6MDAgMjAwMQ0KRnJvbTogY2hhc2JlcnJ5IDxj Y2JlcnJ5QHVjc2QuZWR1Pg0KRGF0ZTogU2F0LCAyMyBBdWcgMjAxNCAxNTo1 NDo1NSAtMDcwMA0KU3ViamVjdDogW1BBVENIIDIvMl0gbWVyZ2UgQ0NCcyB2 ZXJzaW9uDQoNCi0tLQ0KIGxpc3Avb2ItUi5lbCB8IDMzICsrKysrKysrKysr KysrKy0tLS0tLS0tLS0tLS0tLS0tLQ0KIDEgZmlsZSBjaGFuZ2VkLCAxNSBp bnNlcnRpb25zKCspLCAxOCBkZWxldGlvbnMoLSkNCg0KZGlmZiAtLWdpdCBh L2xpc3Avb2ItUi5lbCBiL2xpc3Avb2ItUi5lbA0KaW5kZXggMDhlZjljMi4u ZjgyOGJmMiAxMDA2NDQNCi0tLSBhL2xpc3Avb2ItUi5lbA0KKysrIGIvbGlz cC9vYi1SLmVsDQpAQCAtMzgzLDYgKzM4MywxNyBAQCBsYXN0IHN0YXRlbWVu dCBpbiBCT0RZLCBhcyBlbGlzcC4iDQogDQogKGRlZnZhciBlc3MtZXZhbC12 aXNpYmx5LXApDQogDQorKGRlZnZhciBvcmctYmFiZWwtUi1zZXNzaW9uLWNt ZA0KKyAgICJsb2NhbCh7DQorICAgICAgb24uZXhpdChmaWxlLmNyZWF0ZSgl UykpDQorICAgICAgc2luayglUykNCisgICAgICByZXBsYXkoDQorICAgICAg ICAgIEZpbHRlcihOZWdhdGUoaXMuc291cmNlKSwNCisgICAgICAgICAgICAg ICAgIGV2YWx1YXRlKCVTLCBlbnZpcj1wYXJlbnQuZnJhbWUoMiksIG5ld19k ZXZpY2UgPSBGQUxTRSwgDQorICAgICAgICAgICAgICAgICAgICAgICAgICBz dG9wX29uX2Vycm9yID0gMEwpKSkNCisgICAgICBzaW5rKCl9KSINCisiZm9y bWF0IHN0cmluZyBmb3IgYW4gYFIgOnNlc3Npb24gOnJlc3VsdHMgb3V0cHV0 JyByZWdpbWUuIikNCisNCiAoZGVmdW4gb3JnLWJhYmVsLVItZXZhbHVhdGUt c2Vzc2lvbg0KICAgICAoc2Vzc2lvbiBib2R5IHJlc3VsdC10eXBlIHJlc3Vs dC1wYXJhbXMgY29sdW1uLW5hbWVzLXAgcm93LW5hbWVzLXApDQogICAiRXZh bHVhdGUgQk9EWSBpbiBTRVNTSU9OLg0KQEAgLTQxOSwyNCArNDMwLDEwIEBA IGxhc3Qgc3RhdGVtZW50IGluIEJPRFksIGFzIGVsaXNwLiINCiAgICAgICAg KG9yZy1iYWJlbC1jb21pbnQtZXZhbC1pbnZpc2libHktYW5kLXdhaXQtZm9y LWZpbGUNCiAJc2Vzc2lvbiBzZW50aW5lbC1maWxlDQogCShmb3JtYXQNCi0i DQotdHJ5Q2F0Y2goLm9yZy5ldmFsLnJlc3VsdCA8LSBldmFsdWF0ZSglUywg bmV3X2RldmljZSA9IEZBTFNFLCBzdG9wX29uX2Vycm9yID0gMEwpLA0KLSAg ICAgICAgIGZpbmFsbHkgPSB7DQotICAgICAgICAgICAgIGNhcHR1cmUub3V0 cHV0KA0KLSAgICAgICAgICAgICAgICAgd2l0aENhbGxpbmdIYW5kbGVycygN Ci0gICAgICAgICAgICAgICAgICAgICByZXBsYXkoRmlsdGVyKGZ1bmN0aW9u ICh4KSAhaW5oZXJpdHMoeCwgXCJzb3VyY2VcIiksDQotICAgICAgICAgICAg ICAgICAgICAgICAgICAgICAgICAgICAub3JnLmV2YWwucmVzdWx0KSksDQot ICAgICAgICAgICAgICAgICAgICAgbWVzc2FnZSA9IGZ1bmN0aW9uICh4KSB7 DQotICAgICAgICAgICAgICAgICAgICAgICAgIGNhdCh4JG1lc3NhZ2UpOw0K LSAgICAgICAgICAgICAgICAgICAgICAgICBpbnZva2VSZXN0YXJ0KFwibXVm ZmxlTWVzc2FnZVwiKQ0KLSAgICAgICAgICAgICAgICAgICAgIH0pLA0KLSAg ICAgICAgICAgICAgICAgZmlsZT0lUykNCi0gICAgICAgICAgICAgZmlsZS5j cmVhdGUoJVMpDQotICAgICAgICAgfSkNCi0iDQotCQkob3JnLWJhYmVsLWNo b21wIGJvZHkpDQotCQlvdXRwdXQtZmlsZQ0KLQkJc2VudGluZWwtZmlsZSkp DQorCSBvcmctYmFiZWwtUi1zZXNzaW9uLWNtZA0KKwkgKG9yZy1iYWJlbC1s b2NhbC1maWxlLW5hbWUgc2VudGluZWwtZmlsZSkJCQ0KKwkgKG9yZy1iYWJl bC1sb2NhbC1maWxlLW5hbWUgb3V0cHV0LWZpbGUpDQorCSAob3JnLWJhYmVs LWNob21wIGJvZHkpKSkNCiAgICAgICAgKHdpdGgtdGVtcC1idWZmZXINCiAJ IChpbnNlcnQtZmlsZS1jb250ZW50cyBvdXRwdXQtZmlsZSkNCiAJIChnb3Rv LWNoYXIgKHBvaW50LW1pbikpDQotLSANCjEuOC41LjIgKEFwcGxlIEdpdC00 OCkNCg0K --0-1602133763-1408839016=:2651--