Skip to content

Conversation

@annxingyuan
Copy link
Contributor

@annxingyuan annxingyuan commented Aug 13, 2019

Changes

  • Add stubbable error UserException
  • Remove call to dataSync from debugger profiler
  • Add tf.time to WebGPU using CPUTimer for now
  • Move checkComputationForErrors from util to profiler for testing purposes
  • Bump tfjs-core dependency

To see the logs from the Cloud Build CI, please join either our discussion or announcement mailing list.


This change is Reviewable

@annxingyuan annxingyuan self-assigned this Aug 14, 2019
Copy link
Contributor

@dsmilkov dsmilkov left a comment

Choose a reason for hiding this comment

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

Reviewed 11 of 11 files at r1.
Reviewable status: 0 of 1 approvals obtained (waiting on @annxingyuan, @dsmilkov, and @nsthorat)


tfjs-core/src/debug_mode_test.ts, line 62 at r1 (raw file):

    const b = tf.tensor1d([2, -1, 0, 3]);

    spyOn(tf.util, 'UserException');

since these tests live in core, you can access exports from any file. Let's declare UserException in profile.ts and import it here.


tfjs-webgpu/package.json, line 46 at r1 (raw file):

    "@webgpu/shaderc": "0.0.6",
    "@webgpu/types": "0.0.6",
    "rollup-plugin-terser": "^5.1.1"

did you mean to add it as devDep?


tfjs-webgpu/tsconfig.json, line 11 at r1 (raw file):

  "compilerOptions": {
    "outDir": "./dist",
    "target": "es2017"

just curious, did you change this since it helps debugging (better stack traces, etc)?


tfjs-webgpu/src/backend_webgpu.ts, line 44 at r1 (raw file):

// START TO-IMPORT-FROM-CORE ============================
// TODO: Delete definitions in this section and import from core once new

now that you depend on new core, is this obsolete? Also can you say TODO(annyuan): ... so the reader knows who has the best context for this todo

Copy link
Contributor

@dsmilkov dsmilkov left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 approvals obtained (waiting on @annxingyuan, @dsmilkov, and @nsthorat)


tfjs-core/src/profiler.ts, line 70 at r1 (raw file):

    const num = vals[i] as number;
    if (isNaN(num) || !isFinite(num)) {
      throw util.UserException(`The result of the '${name}' is ${num}.`);

declare userException in this file and also rename to profileException to be more specific

Copy link
Contributor

@nsthorat nsthorat left a comment

Choose a reason for hiding this comment

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

Reviewed 11 of 11 files at r1.
Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @annxingyuan, @dsmilkov, and @nsthorat)


tfjs-core/src/profiler.ts, line 42 at r1 (raw file):

        Array.isArray(result) ? result : [result] as Tensor[];
    results.forEach(r => {
      r.data().then(vals => {

can you add a quick comment here about this dangling .then() because we don't want the async await to propagate upwards?


tfjs-core/src/profiler.ts, line 70 at r1 (raw file):

    const num = vals[i] as number;
    if (isNaN(num) || !isFinite(num)) {
      throw util.UserException(`The result of the '${name}' is ${num}.`);

also a quick comment here about why a custom exception

Copy link
Contributor Author

@annxingyuan annxingyuan left a comment

Choose a reason for hiding this comment

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

Reviewable status: :shipit: complete! 1 of 1 approvals obtained (waiting on @dsmilkov)


tfjs-core/src/debug_mode_test.ts, line 62 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

since these tests live in core, you can access exports from any file. Let's declare UserException in profile.ts and import it here.

Done


tfjs-core/src/profiler.ts, line 42 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

can you add a quick comment here about this dangling .then() because we don't want the async await to propagate upwards?

Done


tfjs-core/src/profiler.ts, line 70 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

declare userException in this file and also rename to profileException to be more specific

Done


tfjs-core/src/profiler.ts, line 70 at r1 (raw file):

Previously, nsthorat (Nikhil Thorat) wrote…

also a quick comment here about why a custom exception

Done


tfjs-webgpu/package.json, line 46 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

did you mean to add it as devDep?

Done


tfjs-webgpu/tsconfig.json, line 11 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

just curious, did you change this since it helps debugging (better stack traces, etc)?

Yep - mainly so it would be easier to read async code in the build.


tfjs-webgpu/src/backend_webgpu.ts, line 44 at r1 (raw file):

Previously, dsmilkov (Daniel Smilkov) wrote…

now that you depend on new core, is this obsolete? Also can you say TODO(annyuan): ... so the reader knows who has the best context for this todo

Not yet, turns out I was exporting the wrong thing, the next version should fix this though.

@annxingyuan annxingyuan merged commit a015b38 into master Aug 14, 2019
@annxingyuan annxingyuan deleted the add_webgpu_timing branch August 14, 2019 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants