Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tensorboard should support --version #2087

Closed
wchargin opened this issue Apr 3, 2019 · 7 comments
Closed

tensorboard should support --version #2087

wchargin opened this issue Apr 3, 2019 · 7 comments

Comments

@wchargin
Copy link
Contributor

wchargin commented Apr 3, 2019

When dealing with path issues, such as when users (mistakenly) have
multiple versions of TensorBoard installed or are trying to coordinate
modules from multiple sources (e.g., a virtualenv plus system site
packages), having tensorboard --version would be helpful. It’s also
useful as a minimal “is TensorBoard installed?” command that’s less
verbose than tensorboard --helpshort (131 lines) and more portable
than which tensorboard (it’s where tensorboard on Windows).

Plus, come on: everything supports --version. :-)

@thealphacod3r
Copy link
Contributor

@wchargin can you guide me on this I would love to work on this!!

@wchargin
Copy link
Contributor Author

wchargin commented Apr 5, 2019

Hi @amanp592! Yes, this is probably a good starter issue.

You’ll want to define a new --version flag. There’s not a clearly
correct place for this flag to live, but it seems reasonable enough to
add it to core_plugin.py’s define_flags method. Then, in fix_flags
in the same file, you can print out the version and exit with success if
the --version flag was passed.

Let us know if you have questions.

@nfelt
Copy link
Contributor

nfelt commented Apr 5, 2019

I think it might be a bit better to implement the version printing in tensorboard.program.Tensorboard.main, which is the way it works for --inspect:

if self.flags.inspect:
logger.info('Not bringing up TensorBoard, but inspecting event files.')
event_file = os.path.expanduser(self.flags.event_file)
efi.inspect(self.flags.logdir, event_file, self.flags.tag)
return 0

That seems slightly cleaner than just calling sys.exit() in the flag processing code itself since it keeps the functional behavior of main() all in one place and the flag processing just focused on flag processing.

@wchargin
Copy link
Contributor Author

wchargin commented Apr 5, 2019

I think it might be a bit better to implement the version printing in
tensorboard.program.Tensorboard.main, which is the way it works for
--inspect:

That’s also fine with me, given that inspect works that way already.

(For what it’s worth, IMHO this isn’t entirely satisfactory, either,
because really main should not depend on any plugin… and really it’s
questionable whether core_plugin should be a plugin at all.)

@nfelt
Copy link
Contributor

nfelt commented Apr 5, 2019

Yes, I think the problem there is that we have been putting flags that really relate mostly to program itself into CorePlugin for lack of a better place.

@shashvatshahi1998
Copy link
Contributor

@wchargin, @nfelt can you please tell me what else i have to add in this PR.

@nfelt
Copy link
Contributor

nfelt commented May 15, 2019

Fixed by #2097.

@nfelt nfelt closed this as completed May 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants