Move uploader flag parsing to its own file and add basic tests #3471
Conversation
bmd3k
changed the title
wchargin
left a comment
wchargin
left a comment
There was a problem hiding this comment.
Thanks! This is a good start. It would be great to follow up to expand
this to cover the actual logic as well as the remaining plumbing, so
that we could catch regressions like the one in #3377.
nit: OSS commit style is imperative with no trailing punctuation with
optional section prefix, so: “Move uploader flag parsing to its own
file”, or: “uploader: extract and test flag parsing”. Could you please
update the PR title accordingly?
* Why? For consistency; this is the style of commits generated by Git
itself, as in git revert, git merge, etc.
| flags_parser.SUBCOMMAND_KEY_UPLOAD, | ||
| getattr(flags, flags_parser.SUBCOMMAND_FLAG), | ||
| ) | ||
| self.assertEqual("some/log/dir", getattr(flags, "logdir")) |
There was a problem hiding this comment.
This getattr(flags, "logdir") is just flags.logdir; likewise for
other getattrs with constant attribute name.
| DEFAULT_ORIGIN = "https://tensorboard.dev" | ||
|
|
||
|
|
||
| def parse_flags(argv): |
There was a problem hiding this comment.
OK: For what it’s worth, this function is vestigial, and we can really
remove the entire //tensorboard/uploader entry point in favor of
//tensorboard/uploader:uploader_main_lib (which should probably be
renamed; it’s pretty confusing). But fine with me to keep deferring that
change, as it’s not hurting anything.
There was a problem hiding this comment.
I can do it in a followup PR. But first I'll warn the TensorBoard team that we'll be getting rid of //tensorboard/uploader just in case anybody is still using it. And this function would essentially be moved into the test.
|
|
||
| import argparse | ||
|
|
||
| import flags_parser |
There was a problem hiding this comment.
Please always use the full package name for imports: here, you want
from tensorboard.uploader import flags_parser (after the tb_test
import).
tensorboard/uploader/BUILD
Outdated
| srcs = ["flags_parser_test.py"], | ||
| deps = [ | ||
| ":flags_parser", | ||
| "//tensorboard:expect_futures_installed", |
bmd3k
changed the title
I should have clarified: This is not intended to help with the regression in #3377 or the followup work in Issue #3404. I still intend to do that work. It's just not the goal here. Instead this is to prepare for some changes I'm making to the --plugins flag and want to have tested.
Done. |
wchargin
left a comment
wchargin
left a comment
There was a problem hiding this comment.
All sounds good; thanks!
…rflow#3471) * Motivation for features / changes Move flag-defining and parsing logic out of uploader_main.py into its own flag_parser.py file. Add some very basic tests for the moved logic. * Detailed steps to verify changes work correctly (as executed by you) Ran new tests for flag_parser.py. Ran several commands and ensured they ran to completion: bazel run //tensorboard -- dev list bazel run //tensorboard -- dev upload --logdir --plugins badplugin1 badplugin2 bazel run //tensorboard/uploader -- upload --logdir --plugins badplugin1 badplugin2
* Motivation for features / changes Move flag-defining and parsing logic out of uploader_main.py into its own flag_parser.py file. Add some very basic tests for the moved logic. * Detailed steps to verify changes work correctly (as executed by you) Ran new tests for flag_parser.py. Ran several commands and ensured they ran to completion: bazel run //tensorboard -- dev list bazel run //tensorboard -- dev upload --logdir --plugins badplugin1 badplugin2 bazel run //tensorboard/uploader -- upload --logdir --plugins badplugin1 badplugin2
3 participants
Motivation for features / changes
Move flag-defining and parsing logic out of uploader_main.py into its own flag_parser.py file. Add some very basic tests for the moved logic.
Detailed steps to verify changes work correctly (as executed by you)
Ran new tests for flag_parser.py.
Ran several commands and ensured they ran to completion:
bazel run //tensorboard -- dev list
bazel run //tensorboard -- dev upload --logdir --plugins badplugin1 badplugin2
bazel run //tensorboard/uploader -- upload --logdir --plugins badplugin1 badplugin2