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

Refactor TensorBoard prompt and import tracking and add tests #15073

Merged
merged 11 commits into from
Jan 7, 2021

Conversation

joyceerhl
Copy link

@joyceerhl joyceerhl commented Dec 30, 2020

  • Pull request represents a single change (i.e. not fixing disparate/unrelated things in a single PR).
  • Title summarizes what is changing.
  • Has a news entry file (remember to thank yourself!).
  • Appropriate comments and documentation strings in the code.
  • Has sufficient logging.
  • Has telemetry for enhancements.
  • Unit tests & system/integration tests are added/updated.
  • Test plan is updated as appropriate.
  • package-lock.json has been regenerated by running npm install (if dependencies have changed).
  • The wiki is updated with any design decisions/details.

@joyceerhl joyceerhl added the no-changelog No news entry required label Dec 30, 2020
@joyceerhl joyceerhl closed this Jan 1, 2021
@joyceerhl joyceerhl reopened this Jan 1, 2021
@rchiodo
Copy link

rchiodo commented Jan 4, 2021

Is it possible to use this instead?
https://www.npmjs.com/package/xterm

If not, you'll need to update the https://github.com/microsoft/vscode-python/blob/main/ThirdPartyNotices-Repository.txt.

That file indicates code we've 'copied' to our source.

@joyceerhl
Copy link
Author

@rchiodo Thanks for pointing that out, I think I looked at using xterm early on and wrote it off because xterm doesn't expose the parser in its public API (the source files I checked in are from the xterm package you linked), but now that I've gone the route of creating a CoreTerminal (because we need buffer management and parsing) it makes sense to just use xterm.

The only catch is that xterm.js doesn't work with node.js out of the box: xtermjs/xterm.js#2749 The solution recommended in that thread is to set (global as any).window = undefined; right before importing xterm. I don't think this should cause any issues in the extension but please correct me if I'm wrong. I've added a commit that makes this change.

Copy link

@DonJayamanne DonJayamanne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes to dispose more stuff & teardown

@joyceerhl
Copy link
Author

joyceerhl commented Jan 6, 2021

TLDR: Changing scope of this PR to cover adding some missing tests and refactoring tensorBoardPrompt.ts for consistency and SRP.

Longer: Just demoed this to Graham, Jim and Brett + PMs. We decided not to prompt on seeing a tensorboard command being run in the terminal due to concerns about perf of the onDidWriteTerminalData API and the hackery required to get xterm to load in the extension host. Instead I'll implement a pared down version of this PR where we do best-effort detection of tensorboard (no parser or xterm, just a circular buffer, and we check the buffer contents on a whitespace character). If the perf impact of that isn't too bad (since we'd still be using onDidWriteTerminalData), I'll submit that as a separate review. The goal of that is to collect telemetry about how often users are launching tensorboard from the command line, in case all the other entrypoints prove inadequate for promoting usage.

@joyceerhl joyceerhl changed the title Show native TensorBoard prompt when user launches TensorBoard from integrated terminal Refactor TensorBoard prompt and import tracking and add tests Jan 6, 2021
@joyceerhl joyceerhl requested a review from DonJayamanne January 6, 2021 23:35
@joyceerhl
Copy link
Author

I've updated this PR (reducing scope because we're not likely to ship terminal command detection). Would appreciate re-review (@DonJayamanne I think you're still blocking on this one), thanks all!

@joyceerhl joyceerhl merged commit 3fd3b9e into main Jan 7, 2021
@joyceerhl joyceerhl deleted the joyceerhl/parse-terminal-data branch January 7, 2021 23:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog No news entry required
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants