Added support for pathlib paths as a logdir using the recommended os.PathLike class.#5905
Added support for pathlib paths as a logdir using the recommended os.PathLike class.#5905mspils wants to merge 6 commits intotensorflow:masterfrom
Conversation
arcra
left a comment
arcra
left a comment
There was a problem hiding this comment.
This seems reasonable. I'll check with the team, but I don't think there would be objections to this. Would you mind adding a small test for it? There seems to already be a test for using an explicit writer:
github.com/tensorflow/tensorboard/blob/master/tensorboard/plugins/hparams/_keras_test.py#L133
|
Sounds very reasonable. |
arcra
left a comment
arcra
left a comment
There was a problem hiding this comment.
In addition to the suggestions Riley left, there's a test failing, because the error message needs to be updated in this test:
github.com/tensorflow/tensorboard/blob/master/tensorboard/plugins/hparams/_keras_test.py#L168
additional linebreak Co-authored-by: Riley Jones <78179109+rileyajones@users.noreply.github.com>
removed trailing whitespace Co-authored-by: Riley Jones <78179109+rileyajones@users.noreply.github.com>
|
Thanks for the Change requests. |
| if writer is None: | ||
| raise TypeError( | ||
| "writer must be a `SummaryWriter` or `str`, not None" | ||
| "writer must be a `SummaryWriter`, `str` or `PathLike`, not None" |
There was a problem hiding this comment.
Just a nitpick here but it would be nice to keep the formatting consistent
| "writer must be a `SummaryWriter`, `str` or `PathLike`, not None" | |
| "writer must be a `SummaryWriter`, `str`, or `PathLike`, not None" |
| with self.assertRaisesRegex( | ||
| TypeError, | ||
| "writer must be a `SummaryWriter` or `str`, not None", | ||
| "writer must be a `SummaryWriter`, `str` or `PathLike`, not None", |
There was a problem hiding this comment.
Same comment as above
| "writer must be a `SummaryWriter`, `str` or `PathLike`, not None", | |
| "writer must be a `SummaryWriter`, `str`, or `PathLike`, not None" |
3 participants
Motivation for features / changes
Most other Tensorflow functions accept Paths in other form than strings.
This small change makes the plugin more consistent and users do not need to cast their Path objects to string manually.
Technical description of changes
elif isinstance(writer, os.PathLike):
self._writer = tf.compat.v2.summary.create_file_writer(os.fsdecode(writer))
Detailed steps to verify changes work correctly (as executed by you)
Barely tested to be honest. Just ran the tutorial code here: https://www.tensorflow.org/tensorboard/hyperparameter_tuning_with_hparams
with my version
Alternate designs / implementations considered
Once all Tensorflow functions accept pathlikes instead of just strings the distinction between str and PathLike will be unnecessary.