Fix exit scene behavior (when exit is anticipated)

[Splines]

In this PR, we fix the following situations:

1. Start Scene somewhere. Then start scene at another line. Confirm Kill it. Before this change, this resulted in a new scene being created and after that being immediately destroyed. The Manim Shell was left in a weird state.
2. Preview a Manim Cell. While previewing issue "Start scene" somewhere else and confirm Kill it. Before this change, the progress notification would remain frozen in the last position indefinitely.
3. Preview a Manim Cell. While previewing, manually open a new terminal, start manimgl in there. The first session should automatically exit, while the second is booting up.

Known limitations

• We currently don't detect when the user forces the shell to exit by other means, e.g. by clicking on the trash bin. This may result in indefinite progress bars when force-quitting the shell during previewing. This can be tackled in a future PR.

[bhoov]

It seems like I am now unable to start a terminal at all on my device. Could this be because I am running inside a conda environment where manimgl is installed?

[Splines]

Cold you check again if it's working now? And make sure to have a clean state, sometimes during extension development VSCode has some caching issues during reload, so start with a fresh new instance. (at least this was sometimes a problem for me. Occassionally, I had to even kill the watch task to force a re-compilation since old code was used even though I reloaded)

Could this be because I am running inside a conda environment where manimgl is installed?

No, I don't think this would be an issue. We are only erroring in case we are trying to spawn a new shell but receive an TerminalShellExecutionEndEvent. We're not even checking if there's anything special about the user's environment. We just want to see the manim welcome string and then the first In [1]: statement and then we know we have successfully started.

So, when you receive this error message, the terminal should also open up automatically. What does it show?
Does this problem only occur with this PR, not already with #45? So on main, it's working for you, just not here?

[VladimirFokow]

@bhoov your error may be just an edge case of where you place a cursor #50

on the construct method
(or on blank lines immediately after it)

[VladimirFokow]

@Splines , you wrote 3 points above.
I checked each of them (with the current state of this branch).
Correspondingly:

• works 👍
• progress notification disappears 👍, but I still get: "❌ No active Manim session found to exit."
• it auto-exits 👍, but I still get the same as above: "❌ No active Manim session found to exit."

Other thoughts:

• I love (!) the integration right with the VSCode notifications!! It is definitely a challenge.

• question:
  why do we need to kill the old Manim window before starting a new one?
  Can't we have multiple Manims running at the same time - just in multiple terminals?
  And even if the user manually opens a new terminal and types manimgl? That doesn't sound very permissive to the user..
  (I suspect: the ManimShell class supports only 1 terminal, and it doesn't matter which terminal is actually focused?
  so this isn't a hard limitation, but just the current implementation?
  I haven't read the ManimShell class yet)

Status:

1. do you agree that these "❌ No active Manim session found to exit." should NOT appear?
2. the question above (just for info)

[Splines]

Warning

Edit: Nevermind, I think we have to fix #53 first before reviewing this one makes sense.

Thanks for testing this out @VladimirFokow! As a few things have changed in the meantime, would you mind checking it out again? And if it doesn't work, could you attach respective log files, where each of them tries to do as minimal things as possible? (e.g. one for each scenario)

do you agree that these "❌ No active Manim session found to exit." should NOT appear?

Yes, I do agree that this should not appear and it also doesn't for me. If this is still the case in this PR (the error message is shown): could you verify that it is not the case in the main branch.

---

why do we need to kill the old Manim window before starting a new one?
Can't we have multiple Manims running at the same time - just in multiple terminals?

In theory it'd be possible to spawn multiple Manim instances. But the situation I try to avoid is the following:

• User wants to preview some code, so Manim is started in a new terminal.
• User manually opens up another terminal and types in manimgl. This is fine and should already work: we detect the new terminal as "Manim host" when we see the ManimGL string in the Terminal (this exact capitalization), followed by the first IPython cell. So this new terminal will be used to preview cells. At this state it'd be perfectly fine to have multiple Manim sessions open.
• Now image we didn't kill the session that is still running in the first terminal. The user decides to quit the second terminal and switches over to the first one. They expect that "Preview Manim Cell" will use this terminal now, which is not the case. The problem is that Manim was already spawned in this terminal, but later we used the second terminal. So from the point of view of the code, we don't know that manim is still running in the first one. And we didn't retrieve the ManimGL string from that terminal a second time, so we don't know that this is a "Manim host". Therefore, clicking on "Preview Manim Cell" would again open a new Manim session and the user would be confused.

In theory, supporting the scenario would be possible. But that'd require even more state management, e.g. we'd have to track the terminals and multiple Manim executions across different terminals. I just didn't want to do that, which is why I settled with this limitation (which I don't think is a serious limitation: why would you want to have two Manim previews open at the same time anyways?)

---

(Note the fact that we spawn each new Manim session in a completely new terminal is a different story that is more easily fixable. We could just reuse existing terminals that don't have another command running in them. But that's for another PR...)

[VladimirFokow]

Currently, it doesn't work (click to expand)

Log: Manim Notebook.log

vid.mov

from manimlib import *


class MyScene(Scene):
    def construct(self):
        ## Test
        print('1')

But maybe I should review this after main is merged again?

[Splines]

@VladimirFokow Ready for another review ;)

[Splines]

I just want to check one more thing, could you please send me the log for the following steps, that'd be awesome.

• Start scene at any line. (But don't preview code)
• Then use Quit preview

Your checked-out commit should be 293b93d.

[VladimirFokow]

My behavior on commit 293b93d:

• preview long-running scene
• while it's running, run the command: "Manim Notebook: Quit preview"
• the scene ends immediately (as if we pressed Ctrl+C in the terminal), but terminal is NOT disposed. In that terminal I see the warning

[Splines]

I see that some resourced leaked:

The fact that you see this when you press Ctrl+C shows that this is a Manim-problem and not ours, I guess. And this shouldn't be a big problem, we're trashing the terminal afterwards anyways and Python itself will do some resource-cleanup as well. And even if something leaks... well, then it leaks and the next time you start VSCode this leakage is probably gone...

But where this is of course a problem is when this hinders us somehow from disposing the terminal altogether. Could you maybe record a video for me to better understand what is going on at your end, i.e. what you describe in your comment here.

[VladimirFokow]

yes, it's a manim problem that it doesn't handle such an interruption well.
(while executing a long scene, we Ctrl+C in the terminal -> warning of leakage)
Therefore, I thought that we should not do this;
we should let it finish running the scene..
maybe it has its own reasons for it

[Splines]

But it should always be possible to just force-quit any task. I mean we're in a terminal so we can stop anything if we want to. When using manimgl via the command line I'm employing Ctrl+C all the time if I don't want to continue to watch my preview. That's why I'd like to be able to quit any preview via the extension as well. It's the user's power: of course you might risk some leakage when you quit mid-preview, but is that really an issue? This is just a leakage warning; yes: possibly a small leakage did occur. But it's probably also so small as to go unnoticed. In the end, the OS will free up resources anyways in much deeper layers...

Edit: And when starting a new scene while one is already running, we even ask the user to confirm Kill it. It's their choice.

[VladimirFokow]

Log 1:

for this: #48 (comment)

Manim Notebook.log

vid.mov

[VladimirFokow]

Log 2:

for this: #48 (comment)

Manim Notebook.log

(accidentally clicked twice, but it shouldn't matter)

vid.mov

[VladimirFokow]

we even ask the user to confirm Kill it. It's their choice

ok, I didn't notice..
this would cause the same leakage, right? ok if so

[Splines]

Log 2:

Thank you very much! In the background (other VSCode window) there is a trace shown for startStopScene, is that relevant? Or was it from a previous session?

Sorry nevermind I apparently didn't watch the end of the video...

So thanks for the log. Will investigate...

[Splines]

The issue was again how MacOS handles the keyboard interrupt. It will end the whole IPython session, which is why a TerminalShellExecutionEndEvent is triggered. This in turn calls this.resetActiveShell() which sets this.activeShell = null;. Therefore, we didn't have the reference to the active shell anymore.

On Linux, we still have this reference since a TerminalShellExecutionEndEvent is never triggered in this case due to Ctrl+C just exiting the command inside the IPython kernel, but not the whole IPython kernel itself. Therefore, I didn't encounter this problem.

Now to mitigate this, I temporarily store the last active shell before sending the keyboard interrupt. This way, when it comes to disposing the terminal, we still have a valid reference. See 9abc645

[VladimirFokow]

This fixed the issue: #48 (comment)

[Splines]

Since you've already approved: just write a comment once you approve this PR ;)

[VladimirFokow]

can merge

[Splines]

Wohoo, nice that it works now for you as well 💫 Lesson learned, exiting is not as easy as expected ;) This would be a lot easier if there was a fix for 3b1b/manim#2236

A future PR can now tackle the case where the user manually disposes a terminal. In this case, we want to detect this event and then reset the state accordingly.

[Splines]

And thanks a lot for your on-going manual testing support in this one @VladimirFokow ❤

[VladimirFokow]

I should have done all this myself,
I just didn't have the capacity to dive into the code yet

[Splines]

Don't worry there will be enough future issues waiting for you ;)

[VladimirFokow]

please no😅
Let's try writing code without any issues the 1st time)

[Splines]

Let's try writing code without any issues the 1st time)

Haha, that's the goal, but rarely achievable in real life (I speak from experience, even with a nice testing harness, there will always be bugs or unexpected behavior). Otherwise it'd be too boring 😅

[Splines]

And just a little remark for the future @bhoov @VladimirFokow : you don't have to review code in PRs that are marked as draft 😇. Just to not waste any time since those PRs are usually a little bit of my playground until I finalize them. Beforehand, lots of things won't work even though the PR itself might look like all the features are implemented.