Filter scalar values based on optional param experiment
#1350
Conversation
stephanwlee
force-pushed
the
int2
branch
2 times, most recently
from
57b8168
to
c1c8bf7
Compare
| ORDER BY Tensors.step | ||
| ''', (run, tag, metadata.PLUGIN_NAME)) | ||
| if experiment is None: | ||
| # TODO(stephanwlee): Deprecate this if-block when experiment_id becomes |
There was a problem hiding this comment.
If the IS operator is used on Runs.experiment_id then it'll probably do what you want with one query. Our schema only defines fields as NOT NULL when it must, because required fields in SQL are similarly problematic to required fields in protobuf.
There was a problem hiding this comment.
I am sure I am misunderstanding you here. Sorry in advance.
What this code is trying to do is, in case one is NOT using the data-selector (flippable only via frontend flag), the experiment param is not passed and, in such case, we'd like to fallback to the previous behavior: returning all tensor that matches run name even across (sometimes, non-null) experiments. Is there a better way to conditionally provide the Where clause?
There was a problem hiding this comment.
If the user sets the experiment_name in the DB, then the user should need to enable experiment support in UI to see it. Otherwise, TB must ignore the data. It's necessary for forwards compatibility. These DBs are designed to be potentially TBs in size.
|
If the GUI is in legacy mode, then it makes sense that it would only
display legacy data.
…On Wed, Aug 15, 2018, 4:09 PM Stephan Lee ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In tensorboard/plugins/scalar/scalars_plugin.py
<#1350 (comment)>
:
> - Tensors.dtype
- FROM Tensors
- JOIN Tags
- ON Tensors.series = Tags.tag_id
- JOIN Runs
- ON Tags.run_id = Runs.run_id
- WHERE
- Runs.run_name = ?
- AND Tags.tag_name = ?
- AND Tags.plugin_name = ?
- AND Tensors.shape = ''
- AND Tensors.step > -1
- ORDER BY Tensors.step
- ''', (run, tag, metadata.PLUGIN_NAME))
+ if experiment is None:
+ # TODO(stephanwlee): Deprecate this if-block when experiment_id becomes
I am sure I am misunderstanding you here. Sorry in advance.
What this code is trying to do is, in case one is NOT using the
data-selector (flippable only via frontend flag), the experiment param is
not passed and, in such case, we'd like to fallback to the previous
behavior: returning all tensor that matches run name even across
(sometimes, non-null) experiments. Is there a better way to conditionally
provide the Where clause?
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#1350 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AADAbkVkxp80dp87b561o6mjay-JfTIxks5uRKofgaJpZM4V65bs>
.
|
| @@ -141,13 +141,14 @@ def scalars_impl(self, tag, run, output_format): | |||
| JOIN Runs | |||
| ON Tags.run_id = Runs.run_id | |||
| WHERE | |||
| Runs.run_name = ? | |||
| Runs.experiment_id = ? | |||
There was a problem hiding this comment.
Please use the IS operator here.
The IS and IS NOT operators work like = and != except when one or both of the operands are NULL. In this case, if both operands are NULL, then the IS operator evaluates to 1 (true) and the IS NOT operator evaluates to 0 (false). If one operand is NULL and the other is not, then the IS operator evaluates to 0 (false) and the IS NOT operator is 1 (true). It is not possible for an IS or IS NOT expression to evaluate to NULL. Operators IS and IS NOT have the same precedence as =.
|
|
||
| def generate_run_to_db(self, experiment_name, run_name): | ||
| tf.reset_default_graph() | ||
| global_step = tf.get_variable( |
There was a problem hiding this comment.
I know I said this was best practice for simulating actual V2 summaries usage, but it's probably not necessary for tests - you can just set the step= parameter on .scalar() directly and feed it with a placeholder value, which is simpler. Then there's no need to run a fake "train_op" or run tf.global_variables_initializer().
There was a problem hiding this comment.
Done! I totally forgot about how step argument is a tensor. It looks much cleaner now. Thank you 🙇
| for step in xrange(self._STEPS): | ||
| feed_dict = {placeholder: [1 + step, 2 + step, 3 + step]} | ||
| sess.run(tf.contrib.summary.all_summary_ops(), feed_dict=feed_dict) | ||
| sess.run([train_op, flush_op]) |
There was a problem hiding this comment.
You can just sess.run(flush_op) once at the end.
|
|
||
| scalar_ops = None | ||
| with db_writer.as_default(), tf.contrib.summary.always_record_summaries(): | ||
| tf.contrib.summary.scalar(self._SCALAR_TAG, tf.reduce_mean(placeholder)) |
There was a problem hiding this comment.
Optional, but FWIW, given that you aren't actually using the summary value in this case, it's a lot simpler I think to just use some constant value like 1 or 42 rather than adding complexity to the test with reduce_mean and a placeholder.
| tf.contrib.summary.scalar(self._SCALAR_TAG, 42, step=global_step) | ||
|
|
||
| with tf.Session() as sess: | ||
| sess.run(tf.global_variables_initializer()) |
There was a problem hiding this comment.
Do you still need this? I thought it wouldn't be necessary now
| sess.run(tf.contrib.summary.summary_writer_initializer_op()) | ||
| for step in xrange(self._STEPS): | ||
| feed_dict = {global_step: step} | ||
| sess.run(tf.contrib.summary.all_summary_ops(), feed_dict=feed_dict) |
There was a problem hiding this comment.
I think you still need sess.run(flush_op)? I just meant it could be once at the end of the for loop, rather than inside the for loop.
Multiple experiments can have the same runName/tagName tuple. In such case, we want to be able to return scalar tensors of an experiment instead of all. The change also includes basic tests for fetching scalar values and exercising the `experiment` argument.
Now that selection is populated and categorized by the scalar plugin, it now needs to
filter out scalar values based on the experiment id.
About test: although not calling implementation method is the preferred way in general,
I decided to honor established pattern at least for this change.