Manim-Notebook / manim-notebook

Simple commands to replicate the manim dev workflow in VSCode
MIT License
5 stars 0 forks source link

[bug] Quits if "Preview Manim Cell" before it finishes #16

Closed VladimirFokow closed 5 days ago

VladimirFokow commented 1 week ago

Is this happening on your side as well?

Example code I'm trying:

from manimlib import *

class Test(Scene):
    def construct(self):
        ##
        circle = Circle()
        self.play(ShowCreation(circle), run_time=3)

Steps:

manimgl

# Press "Preview Manim Cell"

# While it's still executing: "Preview Manim Cell" again -> it quits
Splines commented 1 week ago

This is wanted behavior as described here.

The currently running command is canceled and a new one is issued. I like this behavior as I don't want to be forced to wait for an animation to finish entirely before previewing it again (e.g. with adjusted parameters).

VladimirFokow commented 1 week ago

hmm.. After reading that text - I thought it would re-run the cell.. not quit.

For me it just quits (the window closes)

Splines commented 1 week ago

For me it works just fine (running in WSL Ubuntu):

https://github.com/user-attachments/assets/1328d465-fd7f-4832-b5ca-4bfb5239f7f5

VladimirFokow commented 1 week ago

ok, then maybe OS differences (I'm on MacOS) - will have to look into that..

bhoov commented 1 week ago

I can replicate @VladimirFokow's experience. Pressing the "preview" button before the animation is finished rendering exits the iPython terminal.

I am also on MacOS

VladimirFokow commented 1 week ago

looks like on macos, if you press ctrl+c while checkpoint_paste() is still executing, it quits the whole ipython

bhoov commented 1 week ago

yes, this happens to me as well. I get the following error when pressing ctrl+c during checkpoint_paste() execution, which is the same error that we get when pressing preview cell before the previous preview is finished

.../lib/python3.10/multiprocessing/resource_tracker.py:224: UserWarning: resource_tracker: There appear to be 1 leaked semaphore objects to clean up at shutdown
  warnings.warn('resource_tracker: There appear to be %d '

I like the functionality of interrupting the current render and starting a new one when preview cell is clicked -- this is especially helpful for long running animations.

Splines commented 1 week ago

Just to be sure, could you try with Python 3.12 as well, that's the version I'm using.

bhoov commented 1 week ago

I can confirm this still happens with python=3.12

.../lib/python3.12/multiprocessing/resource_tracker.py:254: UserWarning: resource_tracker: There appear to be 1 leaked semaphore objects to clean up at shutdown
  warnings.warn('resource_tracker: There appear to be %d '
Splines commented 1 week ago

Mmh ok, that's unfortunate. So maybe that's something for a new Manim issue with your specific system specs? Feel free to link or use the video from here to show how it's supposed to work.

bhoov commented 1 week ago

Just started debugging this tonight, haven't figured out exactly how to fix.

It seems like the issue of Ctrl-C killing the ipython instance is because of the way the ipython shell is launched. Specifically, if the shell is initialized through manim (see this line), ctrl+c will kill the terminal.

However, normal ipython does not behave this way. Instead, ctrl+c sends a "keyboard interrupt" to the running ipython, which corresponds to the nice behavior we see in @Splines' video

What is different about the ipython shell created in these two manners such that it fails on MacOS but not on Windows? I'm baffled 🤷

bhoov commented 1 week ago

This is a very important usability issue to fix by v0.1.0 release. As a temporary fix, I propose a configuration variable e.g., manim-notebook.allowInterruptPreview: false (by default) that implements @VladimirFokow's original logic to wait until the previous command finishes to run again. If this variable is set to true, we can use @Splines' interrupt logic that seems to work on windows

Splines commented 1 week ago

However, note that the current logic with the isExecuting variable is probably not even working on MacOS, see the docstrings here:

Note that this is not capturing whether the checkpoint_paste() command is still running. Instead, it only captures whether reading/writing to clipboard is currently happening to prevent unpredictable behavior.

This is because while we await the sending of the command to the clipboard, we don't actually see when the output of Manim has finished and it's not that easy to do even with the shell integration since Manim is running inside IPython. That means for example that there is no exit code associated to the end of a preview since IPython is still running at that point.


For this (and other use cases) to work, I have the idea of implementing a ManimShell class inside manimShell.ts as additional abstraction layer that provides methods like executeCommand(). It could then handle everything related to the states of the terminals like opening a new terminal if one is not already open, capturing the Manim output, executing callback functions when Manim throws an error, signaling that Manim has finished with preview etc.

The basis would be to read the output of the terminal, which I was able to do when a new command is sent recently and read() is called immediately after that as seen here and described here on SO. For terminals that already run a command I haven't yet been able to achieve this (yet). And even for the use case where I was able to capture the output, it only works for one command, so I'll have to experiment more with the API.

bhoov commented 1 week ago

I can confirm that, at least on MacOS, this is not a problem with the integrated terminal on VSCode -- the same behavior of Ctrl+C exiting the entire animation happens when starting the manimgl... command from iTerm2.

It looks like the problem is in the IPython's InteractiveShellEmbed and the lack of error handling around signal.SIGINT. Not sure why windows has no problem with this. I would guess that since Grant uses MacOS he is also unable to interrupt a currently-rendering animation with a new set of commands.

This is a tough bug to fix and is likely a problem at a more fundamental level than this extension. How should we move forward?

VladimirFokow commented 1 week ago
bhoov commented 1 week ago

Could you ask that question on their repo 🙏? Alternatively, I can ask that question late tonight when I am available again

Splines commented 1 week ago

on MacOS always use the terminal.sendText, and never the terminal.shellIntegration.executeCommand

I don't see how this would help. We could still use the shell integration on MacOS, right?

We "just" have to prevent that multiple commands overlap in time. This detection mechanism to lock until the whole command is executed (e.g. preview completely finished and not just command issued), can be more or less easily implemented after #30. That is, we disable this nice feature for MacOS while searching for a fix that gets down to the root of the trouble.

bhoov commented 1 week ago

I'm not near my computer, so I cannot validate my thought, but I think what @VladimirFokow meant was that the sendText command strictly sends the specified text to the terminal -- it does not "auto inject" a Ctrl+C like the executeCommand function does. Which means we MacOS users wouldn't have the strange erroring out behavior.

This said, if your PR works as claimed, this would be a huge step towards merging in the preferred functionality in the future (once we can narrow down the source of the bug). I'm in favor of the command detection feature

Splines commented 6 days ago

In #32 (in draft at the time of writing this), for MacOS we don't allow a new command to be executed when one is already executing. So you have to wait until that one is finished.

Should you somehow figure out any way to quit just the running IPython command, while not quitting the whole IPython shell itself, let me know. Then we can implement this behavior programmatically. In a future PR, I will send a keyboard interrupt manually anyways (reasons will be explained in this future PR, too cumbersome to explain without context).

Splines commented 5 days ago

What is different about the ipython shell created in these two manners such that it fails on MacOS but not on Windows? I'm baffled 🤷

Just to let you know that while I'm on Windows, I actually run everything related to this project inside WSL, which is Linux (Ubuntu). All the behavior I described therefore refers to Linux and not Windows.

Splines commented 4 days ago

Could you ask that question on their repo 🙏?

Did someone of you @bhoov @VladimirFokow already opened an issue for the MacOS behavior on the Manim Repo? At least, I couldn't find the issue there.

I think it'd be really nice to have this feature available on MacOS as well such that these users can profit from not having to view every animation until the bitter end, but instead start a new preview and let the old one cancel gracefully.

VladimirFokow commented 4 days ago

not yet

Splines commented 4 days ago

@VladimirFokow No worries.

Here is a video you might want to use to explain what you expect to see. While previewing an animation, I've pressed Ctrl + C. This sends a keyboard interrupt and then I stay in the IPython shell as expected.

https://github.com/user-attachments/assets/938c134d-160c-4a62-a4f3-40f82fbe70fd

VladimirFokow commented 4 days ago

Opened: https://github.com/3b1b/manim/discussions/2236

(I didn't include the video - because it contains our logic of cells, which is new for the community and would add to confusion)

Splines commented 4 days ago

thank you @VladimirFokow 🙌