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 integration #9

Closed
araffin opened this issue May 9, 2020 · 15 comments · Fixed by #30
Closed

Tensorboard integration #9

araffin opened this issue May 9, 2020 · 15 comments · Fixed by #30
Assignees
Labels
enhancement New feature or request help wanted Help from contributors is welcomed
Milestone

Comments

@araffin
Copy link
Member

araffin commented May 9, 2020

Maybe do that in the logger...
It should be optional as it requires tensorflow to be installed...

@rolandgvc is working on it

@araffin araffin added the enhancement New feature or request label May 9, 2020
@araffin araffin added this to the v1.1 milestone May 9, 2020
@araffin araffin added the help wanted Help from contributors is welcomed label May 9, 2020
@AdamGleave
Copy link
Collaborator

Good news is I think if we log with e.g. tensorboardX there would be no need to have a TensorFlow dependency. The TensorBoard UI itself I believe does not require TensorFlow: it's requirements.txt doesn't list it and in a fresh virtual environment after I pip install tensorboard I get:

$ pip list | grep tensorboard
tensorboard            2.2.1
tensorboard-plugin-wit 1.6.0.post3
$ pip list | grep tensorflow
$

That said, we should probably still keep this dependency optional.

@araffin
Copy link
Member Author

araffin commented May 11, 2020

Good news is I think if we log with e.g. tensorboardX there would be no need to have a TensorFlow dependency.

I wanted to use the "native" integration of PyTorch because I don't know how "tensorboardX" is maintained (it seems quite active though), but yes, something to consider.
In fact, if we go to tensorboardX, we could make use of dowel which is a cleaner implementation of the logger (in fact the base code is the same as SB: it comes from rllab).

The TensorBoard UI itself I believe does not require TensorFlow:

Interesting, I was convinced that it did.

@rolandgvc
Copy link
Contributor

Here's a list of tensorboard alternatives worth considering: https://neptune.ai/blog/the-best-tensorboard-alternatives

@Ankur-Deka
Copy link

Ankur-Deka commented May 15, 2020

Could you guys use torch.utils.tensorboard? [Link]
I added a makeshift version of the feature here. [Link]

@araffin
Copy link
Member Author

araffin commented May 15, 2020

Could you guys use torch.utils.tensorboard? [Link]

This is the initial plan, but it requires tensorflow for now (as far as I know).

I added a makeshift version of the feature here. [Link]

@Ankur-Deka

nice, are you aware of the rl zoo? (it looks like you re-implemented part of its features)
If you want you can start a draft PR that add that feature to the logger. Make sure to read the contribution guide first ;)

@Ankur-Deka
Copy link

@araffin Looks like I reinvented the wheel, haha! Thanks for the link!

@rolandgvc
Copy link
Contributor

@Ankur-Deka are you currently working on this?

@Ankur-Deka
Copy link

@rolandgvc I am not

@rolandgvc
Copy link
Contributor

Ok. @araffin can you assign this to me then? I'll start looking into it 👍

@rolandgvc
Copy link
Contributor

rolandgvc commented May 22, 2020

@araffin would you like verbose and tensorboard_log functionalities to behave exactly like in sb2?

@araffin
Copy link
Member Author

araffin commented May 22, 2020

@araffin would you like verbose and tensorboard_log functionalities to behave exactly like in sb2?

i would say yes, depends on what you mean exactly

@rolandgvc
Copy link
Contributor

rolandgvc commented May 23, 2020

In that case, do we make tensorboard a requirement for the library or do we add the option to log all the variables that would otherwise have been written to tb? Also, I couldn't find instances when verbose==2 in sb2. Where should I look?

@araffin
Copy link
Member Author

araffin commented May 23, 2020

o we make tensorboard a requirement for the library or do we add the option to log all the variables that would otherwise have been written to tb? Also, I couldn't find instances when verbose==2 in sb2. Where should I look?

so, no, it should be optional (the same we make documentation and test packages optional).
my idea is to integrate tensorboard in the logger (a bit as it is done in SB2), so we would log to tb the variables that are already logged in the terminal.
However, we should rely on tensorboard_log parameter but not an environment variable.

EDIT: I did not get the part for the verbose=2

@rolandgvc rolandgvc mentioned this issue May 23, 2020
15 tasks
@araffin
Copy link
Member Author

araffin commented May 30, 2020

After some tests, it seems that tensorboard does not require tensorflow anymore.
@AdamGleave @Miffyli @hill-a @erniejunior what type of dependency should it be?

I would put it either in core dependencies or in extra. What do you think?

@AdamGleave
Copy link
Collaborator

After some tests, it seems that tensorboard does not require tensorflow anymore.
@AdamGleave @Miffyli @hill-a @erniejunior what type of dependency should it be?

I would put it either in core dependencies or in extra. What do you think?

I'd lean slightly towards it being an extra, since not everyone will want to use TensorBoard logging. Don't have strong feelings though: since TensorBoard is fairly lightweight making it mandatory is not a big deal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Help from contributors is welcomed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants