Skip to content

Conversation

chihuahua
Copy link
Member

Previously, the profile plugin had been outputting build warnings. This change removes them. Interestingly, the warnings (outputted by Closure) indicate that we should not use optional function parameters in our code. TypeScript allows for parameters with default values, but Closure will issue a warning.

chizeng-macbookpro:tensorboard chizeng$ bazel run tensorboard:tensorboard -- --logdir=~/Desktop/mnist-logs
INFO: Found 1 target...
INFO: From Vulcanizing /tensorboard.html:
2017-08-25 00:36:26.559 WARNING /tf-op-profile/tf-op-details.html-2.js:16: WARNING - Function module$tf_op_profile$utils.flameColor: called with 2 argument(s). Function requires at least 3 argument(s) and no more than 3 argument(s).
var color = flameColor(utilization(node), 0.7);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

2017-08-25 00:36:26.570 WARNING /tf-op-profile/tf-op-details.html.js:15: WARNING - Function module$tf_op_profile$utils.flameColor: called with 1 argument(s). Function requires at least 3 argument(s) and no more than 3 argument(s).
this.style.background = "linear-gradient(to right, " + flameColor(value) +
^^^^^^^^^^^^^^^^^

2017-08-25 00:36:26.570 WARNING /tf-op-profile/tf-op-profile.html.js:62: WARNING - Function module$tf_op_profile$utils.flameColor: called with 2 argument(s). Function requires at least 3 argument(s) and no more than 3 argument(s).
_textColor: function(node) { return flameColor(utilization(node), 0.7); },
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

2017-08-25 00:36:26.571 WARNING /tf-op-profile/tf-op-table.html-2.js:51: WARNING - Object expressions are not callable
this.headerClick(this);
^^^^^^^^^^^^^^^^^^^^^^

2017-08-25 00:36:26.571 WARNING /tf-op-profile/tf-op-table.html-2.js:53: WARNING - Object expressions are not callable
_handleHeaderMouseEnter: function(e) { this.headerHover(this); },
^^^^^^^^^^^^^^^^^^^^^^

2017-08-25 00:36:26.571 WARNING /tf-op-profile/tf-op-table.html-2.js:54: WARNING - Object expressions are not callable
_handleHeaderMouseLeave: function(e) { this.headerHover(null); },
^^^^^^^^^^^^^^^^^^^^^^

2017-08-25 00:36:26.572 WARNING 0 error(s), 6 warning(s), 63.2% typed
Target //tensorboard:tensorboard up-to-date:
bazel-bin/tensorboard/tensorboard
INFO: Elapsed time: 23.585s, Critical Path: 23.28s

INFO: Running command line: bazel-bin/tensorboard/tensorboard '--logdir=~/Desktop/mnist-logs'

@chihuahua chihuahua requested a review from jart August 25, 2017 07:40
Copy link
Contributor

@wchargin wchargin left a comment

Choose a reason for hiding this comment

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

Thanks a bunch for making sure that we have no new warnings. (I assume that this brings us back down to 9?) Once we've excised these and the ones in the graph plugin, we should make warnings fail the build.

Interestingly, the warnings (outputted by Closure) indicate that we should not use optional function parameters in our code. TypeScript allows for parameters with default values, but Closure will issue a warning.

This is known: see #149. If you have a function foo(x: number, y = 'I am optional') in TypeScript, and from non-TypeSciprt you call foo(1), then the Closure compiler will emit an error…but if you add /** @param {string=} y */ to the TypeScript file then this error is silenced. That is, you can still use default arguments, but you must explicitly tell the closure compiler about them.

Could you try adding these annotations instead? They're a better solution: if the default value of the argument changes, under this approach existing code will use the new value. (I.e., only under this approach can you actually use default arguments.)

_updateValue: function(value) {
this.style.background = "linear-gradient(to right, " + flameColor(value) +
" " + percent(value) + ", #ccc " + percent(value) + ")";
this.style.background = "linear-gradient(to right, " +
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider rewriting as:

const color = flameColor(value);
const length = percent(value);
const background =
  `linear-gradient(to right, ${color} ${length}, #ccc ${length})`;
this.style.background = background;

?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yes. Lets clean it up. Done.

Copy link
Member Author

@chihuahua chihuahua left a comment

Choose a reason for hiding this comment

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

The Closure annotations for optional parameters work! That rids us of all warnings during compilation.

_updateValue: function(value) {
this.style.background = "linear-gradient(to right, " + flameColor(value) +
" " + percent(value) + ", #ccc " + percent(value) + ")";
this.style.background = "linear-gradient(to right, " +
Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yes. Lets clean it up. Done.

Copy link
Contributor

@wchargin wchargin left a comment

Choose a reason for hiding this comment

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

Much better; thanks! Thanks for fixing.

@chihuahua chihuahua merged commit 68fe042 into master Aug 25, 2017
@chihuahua chihuahua deleted the chihuahua-remove-warnings branch August 25, 2017 19:41
jart pushed a commit to jart/tensorboard that referenced this pull request Sep 23, 2017
Previously, the profile plugin had been causing warnings to appear during building. This change removes those warnings by adding Closure annotations.

@ioeric @sam-mccall
jart pushed a commit that referenced this pull request Sep 26, 2017
Previously, the profile plugin had been causing warnings to appear during building. This change removes those warnings by adding Closure annotations.

@ioeric @sam-mccall
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants