Skip to content
This repository has been archived by the owner on Aug 15, 2019. It is now read-only.

engine: name all scopes by default #1151

Merged
merged 2 commits into from
Jul 10, 2018
Merged

Conversation

ChALkeR
Copy link
Contributor

@ChALkeR ChALkeR commented Jul 10, 2018

Description

TypeScript doesn't catch that — .name is expected to be a string in runKernel -> profileKernel -> logKernelProfile.

Without this, test-node prints unhandled rejections due to name being undefined in that code path.


For repository owners only:

Please remember to apply all applicable tags to your pull request.
Tags: FEATURE, BREAKING, BUG, PERF, DEV, DOC, SECURITY

For more info see: https://github.com/tensorflow/tfjs/blob/master/DEVELOPMENT.md


This change is Reviewable

TypeScript doesn't catch that --- .name is expected to be a string in
runKernel -> profileKernel -> logKernelProfile.
@ChALkeR
Copy link
Contributor Author

ChALkeR commented Jul 10, 2018

beforeEach(() => {
ENV.engine.startScope();
});
starts unnamed scopes and causes the abovementioned problem.

Alternatively, scope name could be required in startScope, but that would be an incompatible change.

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 1 of 1 files at r1.
Reviewable status: 0 of 1 approvals obtained

@nsthorat nsthorat merged commit b70ef33 into tensorflow:master Jul 10, 2018
ChALkeR added a commit to ChALkeR/tfjs-core that referenced this pull request Jul 10, 2018
dsmilkov pushed a commit that referenced this pull request Jul 10, 2018
This was omitted in PR #1151, sorry for that. 😞 

Refs: #1151
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants