search

victoresque / pytorch-template

PyTorch deep learning projects made easy.
MIT License
4.7k stars 1.08k forks source link

Add pytorch 1.1 utils.tensorboard support. #55

Closed christopherbate closed 5 years ago

christopherbate commented 5 years ago

This pull request adds the ability to choose to use PyTorch 1.1'storch.util.tensorboard.SummaryWriter. The config is changed so that the user passes in an array of modules names. If tensorboardX is not available, it tries to use torch.utils.tensorboard. The user can switch the ordering to change priority.

Tested with the default example using Pytorch 1.1 and Tenorboard 1.14. Also updated Readme to reflect requirements.

SunQpark commented 5 years ago

I think having option to choose backend for tensorboard is not a good idea, since those two are expected to do same thing in the same way.

Can you change the logic for selecting writer in the visualization.py to

  1. try to import tensorboardX(since builtin tensorboard is now in beta)
  2. try to import torch.utils.tensorboard when (1) fails
  3. display warning if both (1) and (2) failed

and revert the tensorboard part of config.json as before?

The remaining changes looks nice. Thank you for contribution, @christopherbate.

christopherbate commented 5 years ago

@SunQpark Will take a look tonight, thanks.

Also just wanted to note that the logic you specified is currently how it works. So the only change you want is for the user to not be able to change the priority ordering of tensorboardX vs. torch.utils.tensorboard and force the priority order to be 1) tensorboardX and then 2) torch.utils.tensorboard?

SunQpark commented 5 years ago

So the only change you want is for the user to not be able to change the priority ordering of tensorboardX vs. torch.utils.tensorboard and force the priority order to be 1) tensorboardX and then 2) torch.utils.tensorboard?

It's not because this functionality is not needed, but because it should be somewhere else than the config file. I think the config file should be as simple as possible, holding configurations about experiments only. Few people will want to change backend for visualization and they will have no problem doing that on the source code.

I have made some local commits about this issue, will upload these if you don't mind.

christopherbate commented 5 years ago

OK, sounds good.

SunQpark commented 5 years ago

My changes are now uploaded here. Please check these and comment.