[DebuggerV2] Add shim for loadMonaco() method and supporting external libraries #3374

caisq · 2020-03-14T21:30:13Z

Motivation for features / changes
- DebuggerV2 will feature a source-code viewing component. As we've discussed, we will use monaco-editor for this purpose.
Technical description of changes
- Add filegroup_external blocks to third_party/js.bzl and the consuming BUILD targets to tensorboard/components/tf_imports/BUILD for:
  - monaco-editor-core (core editor) assets: including .js, .css and .ttf files
  - monaco-languages (language parser and syntax-highlighter) assets: .js files
  - require.js: which is needed for dynamic loading of monaco-editor and monaco-languages assets
Detailed steps to verify changes work correctly (as executed by you)
- Added a unit test for the loadMonaco() method from load_monaco_shim.ts by using fake objects.
Alternate designs / implementations considered
- Discussed alternatives during internal meeting
- Pulling in monaco-editor instead of monaco-editor-core
  - Con: Language definition files for 30+ languages will end up being downloaded dynamically. We need only Python support for the V1 of DebuggerV2.

stephanwlee

I spent about 5 hours thinking I could get everything working via cjs using rollup but it seems to be a little bit involved than that. I know I can get everything to work if I tried but bazel wrapper around rollup makes it even more painful and it definitely is not worth the effort.

I do not have large objection to this approach anymore but can you consider https://github.com/caisq/tensorboard/pull/30? Also, can you show me how these packages will be actually used?

stephanwlee · 2020-03-16T17:44:38Z

tensorboard/components/tf_imports/BUILD

+        "@com_microsoft_monaco_editor//:vs/editor/editor.main.nls.js",
+    ],
+    path = "/tf-imports",
+    visibility = ["//visibility:public"],


make this, and others, private please

caisq

Thanks for taking the time investigating and playing with it and reaching a conclusion compatible with him. Thanks for sending caisq#30. I adopted your changes with the following changes:

We do need the three files from monaco-languages (_.contribution.js, python.contribution.js and python.js) for Python syntax highlighting to work, as it turns out. So I added them.
I added proper strip_prefix args to the tf_web_library targets that wrap around the three new npm packages, so that the web paths don't include strings like "node_moduels/requirejs".
I like your requireP function. I renamed it to make it clearer and added a doc string to it.

The usage example can be seen in another branch of mine:

As such, I've listed you as a co-author for this PR in the PR description.

Note that the pattern in the last link above is only for demo purposes. In an actual PR to be sent after this PR, I will create a separate ng component (SourceCodeComponent) to call loadMonaco() from.

caisq · 2020-03-17T12:50:43Z

tensorboard/components/tf_imports/BUILD

+        "@com_microsoft_monaco_editor//:vs/editor/editor.main.nls.js",
+    ],
+    path = "/tf-imports",
+    visibility = ["//visibility:public"],


stephanwlee · 2020-03-17T17:51:03Z

tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/views/source_code/load_monaco_shim.ts

+ *
+ * @param paths
+ */
+async function requireAsPromise(paths: string[]): Promise<void> {


No need for async in this method.

stephanwlee · 2020-03-17T17:55:59Z

...orboard/plugins/debugger_v2/tf_debugger_v2_plugin/views/source_code/load_monaco_shim_test.ts

+
+  let requireSpy: jasmine.Spy;
+  beforeEach(() => {
+    windowWithRequireAndMonaco.require = createFakeRequire();


Use proper jasmine API?

spyOnProperty(window, 'require').and.returnValue({ theRequireMethodWithWeirdProperties });

I tried that and it didn't work, because window.require doesn't exist or have an accessor type.

stephanwlee · 2020-03-17T17:58:08Z

...orboard/plugins/debugger_v2/tf_debugger_v2_plugin/views/source_code/load_monaco_shim_test.ts

+  fit('rejects if require.js is unavailable', async (done) => {
+    delete windowWithRequireAndMonaco.require;
+    try {
+      await loadMonaco();


I forget which version of jasmine we are using but you should be able to use: toBeRejectedWithError https://jasmine.github.io/api/3.5/async-matchers.html

I was looking to use the same thing, but it turns out that the version of jasmine we're using (@bazel/jasmine, 0.32.2) doesn't have async matchers such as toBeRejected(), toBeRejectedWith() or toBeRejectedWithError(). I've added a TODO item here.

stephanwlee · 2020-03-17T17:59:26Z

...orboard/plugins/debugger_v2/tf_debugger_v2_plugin/views/source_code/load_monaco_shim_test.ts

+    expect(requireSpy).not.toHaveBeenCalled();
+  });
+
+  fit('rejects if require.js is unavailable', async (done) => {


Remove fit please

Ooops. Sorry. Thanks for the catch.

caisq

Thanks!

caisq · 2020-03-17T19:01:16Z

tensorboard/plugins/debugger_v2/tf_debugger_v2_plugin/views/source_code/load_monaco_shim.ts

+ *
+ * @param paths
+ */
+async function requireAsPromise(paths: string[]): Promise<void> {


caisq · 2020-03-17T19:05:17Z

...orboard/plugins/debugger_v2/tf_debugger_v2_plugin/views/source_code/load_monaco_shim_test.ts

+
+  let requireSpy: jasmine.Spy;
+  beforeEach(() => {
+    windowWithRequireAndMonaco.require = createFakeRequire();


I tried that and it didn't work, because window.require doesn't exist or have an accessor type.

caisq · 2020-03-17T19:05:40Z

...orboard/plugins/debugger_v2/tf_debugger_v2_plugin/views/source_code/load_monaco_shim_test.ts

+    expect(requireSpy).not.toHaveBeenCalled();
+  });
+
+  fit('rejects if require.js is unavailable', async (done) => {


Ooops. Sorry. Thanks for the catch.

caisq · 2020-03-17T19:17:48Z

...orboard/plugins/debugger_v2/tf_debugger_v2_plugin/views/source_code/load_monaco_shim_test.ts

+  fit('rejects if require.js is unavailable', async (done) => {
+    delete windowWithRequireAndMonaco.require;
+    try {
+      await loadMonaco();


I was looking to use the same thing, but it turns out that the version of jasmine we're using (@bazel/jasmine, 0.32.2) doesn't have async matchers such as toBeRejected(), toBeRejectedWith() or toBeRejectedWithError(). I've added a TODO item here.

caisq · 2020-03-17T19:54:05Z

Thanks, @stephanwlee . Note that I'll need to hold off merging this PR a little bit given that internal copybara change needs to happen before this PR is synced and because there is a parallel effort on the tbdev uploader side that requires fast sync'ing.

…external libraries (#3374)" (#3396) This reverts commit 5b88e94. * Motivation for features / changes * Sync'ing requires more work. In order to avoid blocking other people, let's roll this back first.

caisq added 4 commits March 14, 2020 15:14

[DebuggerV2] Add external filegroups and tf_imports targets for monaco

85a2058

Move toward adding a unit test

4635de9

Add unit test

206bc5b

Adjust function names in load_monaco_shim_test.ts

4effe98

googlebot added the cla: yes label Mar 14, 2020

Add doc string to loadMonaco()

7d6bcc9

caisq changed the title ~~Dbg2 source code monaco require 2~~ [DebuggerV2] Add shim for loadMonaco() method and supporting external libraries Mar 14, 2020

caisq added 4 commits March 14, 2020 17:34

Fix BUILD file formatting

48276ca

Revise doc strings and const names; add reject logic and unit test

db4c5ac

Remove debugging cruft

5c2d9ca

Improve doc string slightly

dcae661

caisq requested a review from stephanwlee March 16, 2020 14:13

caisq marked this pull request as ready for review March 16, 2020 14:13

stephanwlee reviewed Mar 17, 2020

View reviewed changes

Address Stephan's comments; Adopt his suggestions

f9da801

caisq commented Mar 17, 2020

View reviewed changes

Remove obsolete targets in third_party/js.bzl

77b5d5d

caisq requested a review from stephanwlee March 17, 2020 14:12

stephanwlee reviewed Mar 17, 2020

View reviewed changes

Address 2nd round of comments

5e85212

caisq commented Mar 17, 2020

View reviewed changes

caisq requested a review from stephanwlee March 17, 2020 19:24

Remove unused import

f89ed47

stephanwlee approved these changes Mar 17, 2020

View reviewed changes

Merge branch 'master' into dbg2-source-code-monaco-require-2

97c38a0

caisq merged commit 5b88e94 into tensorflow:master Mar 18, 2020

caisq mentioned this pull request Mar 18, 2020

Revert "[DebuggerV2] Add shim for loadMonaco() method and supporting … #3396

Merged

[DebuggerV2] Add shim for loadMonaco() method and supporting external libraries #3374

[DebuggerV2] Add shim for loadMonaco() method and supporting external libraries #3374

Uh oh!

Conversation

caisq commented Mar 14, 2020

Uh oh!

stephanwlee left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

caisq left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

caisq Mar 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

caisq left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

caisq Mar 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

caisq commented Mar 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

caisq Mar 17, 2020 •

edited

Loading

caisq Mar 17, 2020 •

edited

Loading

caisq commented Mar 17, 2020 •

edited

Loading