spyder-ide / spyder-kernels

Jupyter Kernels for the Spyder console
MIT License
38 stars 40 forks source link

Can't repeat runcell from terminal #97

Closed impact27 closed 5 years ago

impact27 commented 5 years ago

When calling runfile, I can just repeat the call in the terminal by pressing up + enter. I use to be able to do that when running a cell as well. But since the introduction of the runcell function, I get:

--Run Cell Error--
Please use only through Spyder's Editor; shouldn't be called manually from the console

Maybe it would make sense to give the line number to runcell so it could be called from the terminal?

bcolsen commented 5 years ago

Relevant parts copied and edited from #98 to have the discussion continue here:

My use case would mainly be to be able to repeat the previous cell with up + enter. I just feel that it is strange to have a function executed in the console, and added to the history, with hidden arguments, which means that you can’t execute it again.

This is my one documented regression with the runcell function (see:spyder-ide/spyder#7113). Currently we can't have the qtconsole run commands in spyder without implementing the jupyter custom messaging api. This would be the best answer to this problem but it is quite difficult. To be clear though, up + enter didn't work that well with the previous version. It would only rerun the same code that was copied in last time.

Having runcell repeat the last command was one option that was discussed but it would be ultimately more confusing for the users since they would think it was running the current version of the cell from there open code and not a version from last tie they press ctrl + enter

ccordoba12 commented 5 years ago

@impact27, @bcolsen, what do you say if (for now) we skip runcell from history? I think that would be the simplest solution to fix this.

impact27 commented 5 years ago

@impact27, @bcolsen, what do you say if (for now) we skip runcell from history? I think that would be the simplest solution to fix this.

https://github.com/spyder-ide/spyder/pull/9014 should fix the history problem

Having runcell repeat the last command was one option that was discussed but it would be ultimately more confusing for the users since they would think it was running the current version of the cell from there open code and not a version from last tie they press ctrl + enter

I completely agree with that. Usually I want to rerun the cell after making a few modifications. Maybe the solution would be to give a reference to the editors to the kernels? Something like a callback function "cell_finder" that would get the referenced cell from the editor if the file is open.

The idea of a callback function would be to avoid adding any code specific to the editor in the spyder-kernels code base. I see something like:

text = None
if ipython_shell.cell_finder is not None:
    text = ipython_shell.cell_finder(args) # returns None if failed to find text
if text is None:
    text = cell_from_file(args)

Where args would be the file name and an unique identifier for the cell (The line number? I don't think we can expect the cell name to be unique.). cell_from_file would be similar to https://github.com/spyder-ide/spyder-kernels/pull/98.

Is there a reason why a closure might not work as expected?

impact27 commented 5 years ago

Is there a reason why a closure might not work as expected?

Is there a way to give a callback function with QtKernelManager or QtKernelClient?

impact27 commented 5 years ago

You can have a look at what I did in spyder/plugins/ipythonconsole/widgets/shell.py (https://github.com/spyder-ide/spyder/pull/9014). The only thing that is really missing is a way to identify a cell. I think that the cell name might not be unique. The line number is probably a better solution.

impact27 commented 5 years ago

To work around the jupyter custom messaging api, I am intercepting the messages before they go to jupyter.

impact27 commented 5 years ago

My solution is implemented in:

101 + https://github.com/spyder-ide/spyder/pull/9014

bcolsen commented 5 years ago

@impact27 @CAM-Gerlach I'm moving the conversation back to the issue to define the issue better so we can tell if it is fixed.

It seems the issue is runcell() looks like a function so we want to call it like a function and have it do something useful. This is exasperated by the fact that I used the function parameters to show the users what it is "doing", but in reality the runcell function is doing nothing with those parameters.

When I started the runcell execution I wanted the console to say "running cell ..." as but the function idea was cleaner and consistent with runfile and debugfile , but it's a bit of a hack since unlike the other functions the parameters are unused. Of course now that it looks like runfile people will think it acts like runfile

I should mention that runfile and debugfile don't do the same thing from IPython as F5 and ctrl+F5 do from the gui such save the changes before execution because there is no way have the ipython kernel interact with the rest of Spyder with out comms in qtconsole.

@impact27 has implemented a way around the comms issue by having qtconsole intercepts the call to runcell before it gets to the kernel and sets the code to the kernel variable. I like this! It's a hack that opens up things like saving files before running runfile of debugfile ( @impact27 As a suggestion this might be a more simpler first PR use for this type of interception to find possible problems). The one down side of this approach is the runcell function can only be used in the ipython console so it isn't general fuction to be used in scripts. It won't even currently allow running multiple cells in one execution (I would enjoy this).

Solution 1

The runcell() function takes no arguments. It always runs the cell at the current cursor position in the editor.

Pros: Always suggest the simplest first. We could make a runlastcell() function too.

Cons: No history of what's been executed, but I don't look at that much. I don't think Matlab puts cells in the history at all.

Solution 2

The runcell() function takes file, cell_name and line_number to find the cell to run.

Pros: Cells are well defined and have human readable locations. History of what was run. Currently implemented in https://github.com/spyder-ide/spyder/pull/9014

Cons: Line numbers can easily change. The name is only there as a history element. No one is going to type that into the console, so It really only good to rerun cells

Solution 3

The runcell() function takes cell_name and an optional file name (defaults to the current file in the editor or the file for the current dedicated console) and finds the cell to run. cell_name will be used as a key in a dictionary of previously run cell's data block positions. Unnamed or non-unique cell will be given a unique key.

Pros: Cells are well defined and have robust locations.

Cons: Can only run previously run cells. History isn't great for non-unique names.

Solution 4

Same as 3 but we pre-populate the cell dictionary when the file is loaded and actively update the cell dictionary as changes are made to lines with cell comments including new cells.

Pros: Cells are well defined and have robust locations.

Cons: History isn't great for non-unique names (if you care but some names in there..it's great documentation too). Hardest to implement

impact27 commented 5 years ago

The problem I see with solution 3 is: how to give a unique key to a cell that would be stable? What if I have: (1 | 2 3) and add a cell between 1 and 2? Should I have (1 4 2 3)? If I reopen the file, what will be the new order?

Maybe solution 2 can be improved by adding a reference number for the block? As you say,

No one is going to type that into the console, so It really only good to rerun cells

But at least finding the line would be more stable

bcolsen commented 5 years ago

The problem I see with solution 3 is: how to give a unique key to a cell that would be stable? What if I have: (1 | 2 3) and add a cell between 1 and 2? Should I have (1 4 2 3)? If I reopen the file, what will be the new order?

Maybe solution 2 can be improved by adding a reference number for the block? As you say,

No one is going to type that into the console, so It really only good to rerun cells

But at least finding the line would be more stable

To make code cells stable during editing it's best to use block_id and when reopening we only have line_numbers. We would have to save the code cell unique id and line_number to a config dictionary per file. When the file is loaded we make a working dictionary of unique id to block_id. This is similar to he way break points are handled. This was also recently implemented for cursor bookmarks in https://github.com/spyder-ide/spyder/pull/848. This way the unique_id and filename can be used in runcell to define the cell. The unique id would need to be edited when the code cell name changes.

The cell unique id can be the name of the cell if it's unique or a non-unique name + id number where the ID number assigned to new cells will be the highest ID number assigned in the current dictionary plus one. There is a corner case where someone names their cell with a number at the end# %% My Cell 1000000 the next unlabeled cell made will get Unnamed cell 1000001, but at least it's still unique and stable

impact27 commented 5 years ago

In my opinion, the way forward is to first fix https://github.com/spyder-ide/spyder/issues/7976.

The idea would be to find a stable identifier for a block, and save it in block.userData(). This could replace the line numbers everywhere in the code to solve similar problems:

For block_id, I first thought of id(block), but this is not constant for some reason. id(block.userData()) seems to be a bit better. This would not be conserved if the file is reopened. Do you have a better idea for block_id? is saving an id for every single line of every single open file not a bit extreme?

Once we have a block identifier, whatever it is, can be use with the cell name to identify a cell. It could even be used without the cell name to allow for a cell rename.

bcolsen commented 5 years ago

In my opinion, the way forward is to first fix spyder-ide/spyder#7976.

I agree. The outline explorer will be even more prominent in future Spyder versions. It could be handy to run code cells from the outliner as well.

I'll discuss the rest of this on spyder-ide/spyder#7976.