Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

RBL/Debugging code cleanup #7346

Closed
Tracked by #5607
roblourens opened this issue Aug 30, 2021 · 5 comments · Fixed by #7416
Closed
Tracked by #5607

RBL/Debugging code cleanup #7346

roblourens opened this issue Aug 30, 2021 · 5 comments · Fixed by #7416
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug debt Code quality issues notebook-debugging notebook-run-by-line

Comments

@roblourens
Copy link
Member

roblourens commented Aug 30, 2021

  • Look at how the debug adapter tracker running the variables view interacts with the KernelDebugAdapter
  • Split KernelDebugAdapter into multiple classes that only have one job
  • Move the DebugAdapterDescriptorFactory into its own class (depends on variables stuff, and not setting global context keys for debug)
  • We send disconnect multiple times which triggers errors from the kernel
@roblourens roblourens added bug Issue identified by VS Code Team member as probable bug debt Code quality issues labels Aug 30, 2021
@roblourens roblourens added this to the September 2021 milestone Aug 30, 2021
@DavidKutu
Copy link

dump all cells is not working

@roblourens
Copy link
Member Author

roblourens commented Sep 2, 2021

Variables cleanup: Currently, the flow is like this

  • kernelDebugAdapter sends stackTrace/scopes/variables requests and ignores the responses of those requests
  • DebuggerVariables is a DebugAdapterTracker, and updates itself when the variables request goes through
  • DebuggingManager sees the variables response go through, and fires onDidFireVariablesEvent
  • VariableView listens to that event and prompts the UI to refresh itself

So the logic is distributed in multiple places. It would be cleaner to have DebuggerVariables be responsible for updating itself and knowing what logic is needed to do that, so @DavidKutu and I plan to do it this way instead:

  • DebuggerVariables sees the stopped event emitted by the DA
  • It sends the stackTrace/scopes/variables requests needed to refresh its variables list
  • It fires an event onDidUpdateVariables
  • VariableView listens to that event and prompts the UI to refresh

Then we can lose the DA sending variables requests to trigger a side effect somewhere else, and the weird onDidFireVariablesEvent. And this will essentially fix #7365. One issue I can think of is that DebuggerVariables may need to know whether we are in RBL or full debugging, so it knows which scopes to update variables for? Not sure whether that is true or not.

Does this sound ok to you @IanMatthewHuff?

@IanMatthewHuff
Copy link
Member

IanMatthewHuff commented Sep 2, 2021

@roblourens and @DavidKutu . I like this cleanup. Currently it feels pretty shoehorned in. In particular DebuggerVariables owning sending and handling the stackTrace ect... requests feels much nicer.

I'd actually love to meet sometime to discuss moving totally off the old Variable View and onto using the VS Code variables control at all times for notebooks and interactive window both in debugging and normal execution. What we are doing now still feels to me like bolting VS Code debugging concepts into a funky old webview control. I'm a bit unsure of how much work that would be, but once the basics are working to not regress current customers that time investment feels better than sinking more time into the webview control.

@roblourens is that something that feels feasible to you? Could we have the VS Code variables control available in a non-debugging scenario where we could have our old non-debugging method of fetching variables (the KernelVariables classes) respect the debug messages?

@roblourens
Copy link
Member Author

Well we wouldn't want to run a debug session all the time. If we show vscode's variables view in some other context, I don't know how that generalizes to other situations. I think we would be talking about something notebook-specific. I suppose that any notebook that has state could use something like this though. Honestly the webview UI feels fine right now. Providing this to other notebooks in general would be a big task but there is no other consumer right now. Maybe Don's TS notebook some day. I dunno if @rebornix feels differently

@roblourens roblourens mentioned this issue Sep 3, 2021
9 tasks
DavidKutu pushed a commit that referenced this issue Sep 13, 2021
* Refactor KernelDebugAdapter and add RunByLineController
#7346

* Move initializeExecute into RunByLineController

* Add cell debug controller

Co-authored-by: David <dakutuga@microsoft.com>
@roblourens roblourens reopened this Sep 15, 2021
@roblourens
Copy link
Member Author

Remaining:

  • We send disconnect multiple times which triggers errors from the kernel
  • The initial dumpAllCells() call doesn't seem to work correctly and triggers errors from the kernel

@DavidKutu DavidKutu mentioned this issue Sep 16, 2021
9 tasks
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug debt Code quality issues notebook-debugging notebook-run-by-line
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants