tbdev turndown: replace dev subcommand with shim #6707
Conversation
tensorboard/main.py
Outdated
| @@ -40,7 +40,7 @@ def run_main(): | |||
|
|
|||
| tensorboard = program.TensorBoard( | |||
| plugins=default.get_plugins(), | |||
| subcommands=[uploader_subcommand.UploaderSubcommand()], | |||
| subcommands=[legacy_tbdev_subcommand.LegacyTbdevSubcommand()], | |||
There was a problem hiding this comment.
Maybe it's not worthwhile but it seems like you could at least delete uploader_subcommand.py.
There was a problem hiding this comment.
That's true, and there are some other related files. But I think it might actually be more manageable than I thought to remove all the uploader code, and doing it all at once is less effort than trying to carve out just the "clearly deleteable" chunk, so if you don't mind I will punt (and if we run into trouble getting all of it deleted I can come back and delete the easier parts).
|
Also, out of curiosity, why develop this in the tensorflow/tensorboard repo instead of your own repo? |
| def define_flags(self, parser): | ||
| pass # no flags to define | ||
|
|
||
| def run(self, flags): |
There was a problem hiding this comment.
This works great for tensorboard dev but for tensorboard dev upload or tensorboard dev auth revoke you get a somewhat-less-helpful error message like "tensorboard: error: unrecognized arguments: upload".
Is that intentional?
Very few people will just run tensorboard dev.
There was a problem hiding this comment.
Ah, great point. There unfortunately doesn't seem to be any way to tell an argparse subcommand to simply swallow all remaining arguments - you can swallow all positional arguments, but then it would still reject things like tensorboard dev export --outdir foo unless we define --outdir as a flag, and so on.
So I've reworked this to instead just intercept directly at the sys.argv level. That is slightly less complete since it won't trigger for things like tensorboard --verbosity 2 dev export --outdir, but that's too niche to care, and even then the error is actually still better than before: tensorboard: error: argument {serve}: invalid choice: 'dev' (choose from 'serve').
There was a problem hiding this comment.
Also, out of curiosity, why develop this in the tensorflow/tensorboard repo instead of your own repo?
I usually prefer my own repo for one-off PRs, but you can only make a given PR a "diffbase" of another PR if its branch is in the main repo, since that allows it to be the merge target for the subsequent PR and generate the diffs relative to it during review (and then to submit you would re-point it to merge into master).
So I did that in this case under the assumption that I would stage the rest of the deletion as its own child PR, but then didn't quite get around to that.
| def define_flags(self, parser): | ||
| pass # no flags to define | ||
|
|
||
| def run(self, flags): |
There was a problem hiding this comment.
Ah, great point. There unfortunately doesn't seem to be any way to tell an argparse subcommand to simply swallow all remaining arguments - you can swallow all positional arguments, but then it would still reject things like tensorboard dev export --outdir foo unless we define --outdir as a flag, and so on.
So I've reworked this to instead just intercept directly at the sys.argv level. That is slightly less complete since it won't trigger for things like tensorboard --verbosity 2 dev export --outdir, but that's too niche to care, and even then the error is actually still better than before: tensorboard: error: argument {serve}: invalid choice: 'dev' (choose from 'serve').
tensorboard/main.py
Outdated
| @@ -40,7 +40,7 @@ def run_main(): | |||
|
|
|||
| tensorboard = program.TensorBoard( | |||
| plugins=default.get_plugins(), | |||
| subcommands=[uploader_subcommand.UploaderSubcommand()], | |||
| subcommands=[legacy_tbdev_subcommand.LegacyTbdevSubcommand()], | |||
There was a problem hiding this comment.
That's true, and there are some other related files. But I think it might actually be more manageable than I thought to remove all the uploader code, and doing it all at once is less effort than trying to carve out just the "clearly deleteable" chunk, so if you don't mind I will punt (and if we run into trouble getting all of it deleted I can come back and delete the easier parts).
Resolves #6603 This removes a sizable chunk of the code in the `tensorboard/uploader` subdirectory, now that we've disabled the TensorBoard.dev uploader in #6707. It also removes several Pip package deps that were only needed for the removed code. The remaining code (`logdir_loader.py`, `upload_tracker.py`, `util.py`, their tests, and the protos) is preserved because it is still depended on either by internal code or by [Cloud AI Platform](https://github.com/googleapis/python-aiplatform/blob/cd31c1306a9a00a01fbc1dda56fe99ed567a4cfb/google/cloud/aiplatform/tensorboard/uploader.py#L62-L65). Note that for `util.py`, I've removed everything except `RateLimiter` since that was the only symbol where I could find outside dependencies. To simplify the test code I also folded `FakeTime` from `test_util.py` into `util_test.py`. Tested with an internal sync to confirm tests will still pass (once a few other changes land).
This replaces the
devsubcommand with a shim that prints out a message about the turndown.Unfortunately, the actual uploader code can't quite be deleted yet because Cloud AI Platform is depending on it still:
https://github.com/googleapis/python-aiplatform/blob/cd31c1306a9a00a01fbc1dda56fe99ed567a4cfb/google/cloud/aiplatform/tensorboard/uploader.py#L62-L65