-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
default logger is now tensorboard #609
Conversation
I was hoping to have a few people try it out before we make it the default... |
sure. let's keep the PR open for a few days while we try it out. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
replace tensorboard
longer also in tests as default logger
@@ -4,5 +4,4 @@ numpy>=1.16.4 | |||
torch>=1.1 | |||
torchvision>=0.4.0 | |||
pandas>=0.24 # lower version do not support py3.7 | |||
test-tube>=0.7.5 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add test-tube
to test/requirements.txt
The logger is broken:
Lets fix the CI - Travis, Appveyor and CircleCI so we do not merge code which fails... |
@williamFalcon rebase master, pls... |
Any update on this? |
I get the following error due to a call to SummaryWriter.add_hparams: {'batch_size': 128, 'epochs': 10, 'lr': 0.1, 'weight_decay': 0.0005, 'save_model': False, 'load_model': '', 'droplr': 5, 'opt': 'nesterov', 'loss': 'nll', 'model': 'mlp_100_100', 'dataset': 'fashion', 'datapath': '~/data/', 'log_interval': 2, 'train_frac': 1.0, 'test_frac': 1.0, 'preprocess': False, 'gpus': None, 'use_16bit': False, 'seed': 872846804}
ERROR:LightDNN:Failed after 0:00:00!
Traceback (most recent calls WITHOUT Sacred internals):
File "scripts/light_dnn.py", line 220, in main
trainer.fit(model)
File "/home/carlo/Git/pytorch-lightning/pytorch_lightning/trainer/trainer.py", line 403, in fit
self.run_pretrain_routine(model)
File "/home/carlo/Git/pytorch-lightning/pytorch_lightning/trainer/trainer.py", line 453, in run_pretrain_routine
self.logger.log_hyperparams(ref_model.hparams)
File "/home/carlo/Git/pytorch-lightning/pytorch_lightning/logging/base.py", line 14, in wrapped_fn
fn(self, *args, **kwargs)
File "/home/carlo/Git/pytorch-lightning/pytorch_lightning/logging/tensorboard.py", line 76, in log_hyperparams
self.experiment.add_hparams(hparam_dict=dict(params), metric_dict={})
File "/home/carlo/miniconda3/envs/sacred37/lib/python3.7/site-packages/torch/utils/tensorboard/writer.py", line 292, in add_hparams
exp, ssi, sei = hparams(hparam_dict, metric_dict)
File "/home/carlo/miniconda3/envs/sacred37/lib/python3.7/site-packages/torch/utils/tensorboard/summary.py", line 156, in hparams
raise ValueError('value should be one of int, float, str, bool, or torch.Tensor')
ValueError: value should be one of int, float, str, bool, or torch.Tensor This is because I have a NoneType value for the gpus params: hparams = { 'gpus': None} It can also be a problem, for list of gpus indexes, since lists are not supported as well. |
good point, for list I would assume string with some common separator and of |
1 similar comment
good point, for list I would assume string with some common separator and of |
It looks like somehow the test-tube dependency got added back in with the merge commit. |
Oh, wait. I was just doing weird things with the merge commit summary. LGTM. |
Quick question, why is test-tube still in requirements.txt? |
we are fixing it in #609 together with some other minor issues... :] |
@williamFalcon @Borda I am still facing this issue for Basically, I tried to pass a list of gpus in the NER example in the transformers repo here and was facing the same value error Instead of passing all the hparams to tensorboard via pl here, we put a small check to pass
IMO, there could be cases when the user wants to parse a list or maybe nonetype objects through argparse but doesnt want them to be logged via tensorboard. Please let me know if this makes sense and I should raise a PR! |
@shubhamagarwal92 hs for this elaboration, sure PR is welcome! |
Sets Tensorboard logger as default instead of test-tube.
Also removes the test-tube dep from the package and thus the tensorflow dep