ticoneva / pystata-kernel

Jupyter kernel for Stata based on pystata
GNU General Public License v3.0
13 stars 6 forks source link

Should we have a 'silent' cell magic? #16

Open ideabucket opened 2 years ago

ideabucket commented 2 years ago

I was thinking about #4 again in light of PR #15 and trying to work out when the echo-all behaviour is most annoying. For me, it's usually when I want to avoid the screen filling up with a bunch of echoed commands that have little or no output. For example, if I'm putting together an etable it looks something like this:

quietly regress…
estimates store…

quietly regress…
estimates store…

…

etable…

Or in a setup block—since pystata doesn't pick up profile.do, every notebook I make starts with:

version 17
set varabbrev off
global data_root…
…

and other throat-clearing.

So I was wondering: what if we just added a cell magic, e.g. *%silent, that tells the kernel not to print the output of that particular cell unless an exception is thrown? I'm happy to take a crack at implementing this but I thought I'd put it up for discussion first.

@hugetim, would this be any good for your use case?

ticoneva commented 2 years ago

That's a good idea. This can be done by passing quietly=True to pystata.stata.run().

For consistency with Stata, maybe we should name this magic *%quietly?

There is one question I have to ask though: why would you not use Stata's native quietly? Like

qui {
...
}
ideabucket commented 2 years ago

Primarily, because with a native quietly { … } block you still get the first line echoed back, and the empty dot prompt after it:

quietly {
    dosomething
    dosomethingelse
}
. quietly {

. 

I don't know why pystata behaves this way - if you type or paste a multiline quietly { … } block into Stata itself, you get the whole block in the results window, which makes sense: quietly suppresses the output of the command(s). (And this is consistent with what happens if you just run quietly dosomething.)

The theory behind this magic, on the other hand, is to suppress everything: the command echo and the output. That's also why I suggested a different name than 'quietly'; it's for a different purpose that doesn't really make sense outside the notebook environment. (But I'm not wedded to that.)

The less important reason for a magic over a quietly { … } block is that it doesn't require wrapping the cell in braces or indenting everything, which means less hassle and, if the notebook gets exported to a do-file, tidier code.

hugetim commented 2 years ago

That would help for my use case, yes. And, to me, the *%quietly name makes some sense because, in a notebook, you see the commands in the block, akin to how the commands would show up in the log using a quietly block in Stata normally.

I think I'd still like to also have a *%no_echo magic that displays only the output of the commands as in #15 (with documented caveats about program definitions not working, limited allowance for #delimit, and any other issues that arise over time).

ticoneva commented 2 years ago

Given the interest, I have added the logic necessary to implement such magics over the weekend. There are two new magics and a new configuration option:

Magics

Configuration

The logic should be able to handle multi-line comments and change of delimiter correctly. Please check and see if I miss anything.

hugetim commented 2 years ago

I'm not sure I'm going to be able to take the time soon to understand how you're handling the delimiters. (One thing I was worried about with the quietly-noisily approach was handling the /// approach to multi-line commands--or weirder things like delimiter changes via local macros.) But I should be able to test it out a bit tomorrow.

ticoneva commented 2 years ago

The way it works right now is, whenever a user requests echo supression, helpers.clean_code() is called to do two things:

  1. Recursively detect and apply the correct delimiter.
  2. Strip multi-line comments (/// and /* */).

The processed code is then passed to pystata.stata.run().

Following your comment, I have run extensive tests on #delimit's behavior.

ideabucket commented 2 years ago

The way it actually works is, #delimit x changes the delimiter to ';' as long as x is not 'cr'.

Just to note that the Stata help file for delimit is quite explicit that it only supports two values: ; and cr.

hugetim commented 2 years ago

Ok, that makes sense and sounds quite robust. Thank you!

What I actually had in mind with my macro-delimit comment, though, was something more along these lines:

local my_delimit "#delimit ;"
`my_delimit'

But I've never heard of anyone doing that, and I haven't tested it yet.

p.s. I see now from the delimit help file that it's not possible to set the delimiter in a previous notebook block and have it carry over to the next block, which was another thing I had been puzzled about.

hugetim commented 2 years ago

Hmm, when defining programs (with echo = None), it will add noisily to each line within the program, which seems undesirable. Maybe we should prevent that--treat lines between a program define and end differently?

update: Actually, program definition doesn't work in that setting (echo = None or with *%noecho). I get:

SystemError:   ...
  5. noisily end
  6. 
unexpected end of file
r(612);

update2: The same problem occurs for something like a for loop. It complains about the noisily } line. But maybe exceptions for lines that start with } or end are sufficient?

hugetim commented 2 years ago

With echo = None, the *%quietly magic isn't working as I expected. It seems the noecho behavior is preempting it.

ticoneva commented 2 years ago

The way it actually works is, #delimit x changes the delimiter to ';' as long as x is not 'cr'.

Just to note that the Stata help file for delimit is quite explicit that it only supports two values: ; and cr.

Yes, which is why I thought the only acceptable values after #delimit are ';' and 'cr'. Turns out that's not the case.

ticoneva commented 2 years ago

p.s. I see now from the delimit help file that it's not possible to set the delimiter in a previous notebook block and have it carry over to the next block, which was another thing I had been puzzled about.

In Stata the effect of #delimit ; does not extend beyond the current block of code being run—e.g. if you highlight a block of code with #delimit ; in it, run it, and then run another block of highlighted code, the latter won't honor the delimiter change.

Since pystata-kernel process the delimiter before passing the code to Stata, I can actually make the effect persistent across notebook cells. It's arguable whether doing so is more in line with native Stata behavior or not: is running a cell more akin or running a block of highlighted code in a do file, or should we treat the execution of consecutive cells as one single run? I am open to either interpretration. I could also make this into an option.

Maybe we should prevent that--treat lines between a program define and end differently?

update2: The same problem occurs for something like a for loop. It complains about the noisily } line. But maybe exceptions for lines that start with } or end are sufficient?

Thanks for the catch. I will post a commit on that soon.

ticoneva commented 2 years ago

Program definition and loops should now be properly excluded. The quietly magic should also work under the echo = None configuration.

hugetim commented 2 years ago

is running a cell more akin or running a block of highlighted code in a do file, or should we treat the execution of consecutive cells as one single run?

As I think about it, a cell feels more akin to running a block of highlighted code. So I'd vote for keeping the default as is (and I never use delimit myself, anyhow). But an option would make sense if there's demand for it.

hugetim commented 2 years ago

The quietly magic is working for me, but I'm running into various other issues as I test out the noecho mode. For example, the commands within a loop would be echoed. I started #18 to address that and a few other niggles.

hugetim commented 2 years ago

I'm also hitting a corner-case delimit error when #delimit cr is on the last line of code in a cell. (I now understand that there's never a need for such a #delimit at the end of a block, but it is valid Stata code.) For instance:

#delimit ;
disp "hello";
#delimit cr

triggers:

command cr is unrecognized
r(199);

I haven't been able to find a fix so far, myself.

hugetim commented 2 years ago

As I think about it, a cell feels more akin to running a block of highlighted code. So I'd vote for keeping the default as is (and I never use delimit myself, anyhow). But an option would make sense if there's demand for it.

Hmmm, resetting delimit to cr at the top of every cell prevents the exported do file from working. That makes me think the default should be to make the effect persistent across notebook cells (unless we can insert any needed #delimit crs into the exported script).

hugetim commented 1 year ago

Hmmm, resetting delimit to cr at the top of every cell prevents the exported do file from working. That makes me think the default should be to make the effect persistent across notebook cells

To follow up, I've now made #delimit; persist across cells (and also addressed the other issues raised in the above discussion) in the latest release of https://github.com/hugetim/nbstata