-
Notifications
You must be signed in to change notification settings - Fork 298
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
Add tensorboard to trainers tutorial #1163 #1189
Add tensorboard to trainers tutorial #1163 #1189
Conversation
@microsoft-github-policy-service agree |
Thanks for the PR! Let's merge #1124 first so that there are fewer conflicts. |
#1124 has now been merged. Can you rebase? |
cebf848
to
edb2dd8
Compare
Can you strip output and make sure the PR doesn't change the indentation of the file? Otherwise I can't easily see what changed. |
@adamjstewart had issues with rebase -hence the commits- but now I believe it is clear. |
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.
This looks beautiful on Colab! Thanks as always for your Lightning logger help.
Co-authored-by: Adam J. Stewart <ajstewart426@gmail.com>
Co-authored-by: Adam J. Stewart <ajstewart426@gmail.com>
Co-authored-by: Adam J. Stewart <ajstewart426@gmail.com>
Deleted unnecessary `"attachment"{}` lines
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.
Tests are failing because tensorboardX is not sufficient. We have two options here:
- Change
lightning[extra]
dependency (which pulls in tensorboardX) tolightning
andtensorboard
(fewer deps from extra, but tensorboard has more deps) - Add
tensorboard
topip install
in this file,.github/workflows/tutorials.yaml
, and.github/workflows/release.yaml
To get this PR merged as quickly as possible, I think we should go with 2 for now, but I'll ask around and see if anyone has opinions about 1.
* Added tensorboard logger to trainers tutorial * add-tensorboard-for-trainers-tutorial * Added tensorboard logger and reverted conflicts * Add TensorboardLogger and remove CSV * Update docs/tutorials/trainers.ipynb Co-authored-by: Adam J. Stewart <ajstewart426@gmail.com> * Update docs/tutorials/trainers.ipynb Co-authored-by: Adam J. Stewart <ajstewart426@gmail.com> * Update docs/tutorials/trainers.ipynb Co-authored-by: Adam J. Stewart <ajstewart426@gmail.com> * Deleted attachments Deleted unnecessary `"attachment"{}` lines * Changed --logdir to use `default_root_dir` variable * Use logdir with `default_root_dir` * add tensorboard to `release.yaml` and `workflows.yaml` * add tensorboard to pip install * remove unnecessary tensorboard installs * Update trainers.ipynb --------- Co-authored-by: Adam J. Stewart <ajstewart426@gmail.com>
* Added tensorboard logger to trainers tutorial * add-tensorboard-for-trainers-tutorial * Added tensorboard logger and reverted conflicts * Add TensorboardLogger and remove CSV * Update docs/tutorials/trainers.ipynb Co-authored-by: Adam J. Stewart <ajstewart426@gmail.com> * Update docs/tutorials/trainers.ipynb Co-authored-by: Adam J. Stewart <ajstewart426@gmail.com> * Update docs/tutorials/trainers.ipynb Co-authored-by: Adam J. Stewart <ajstewart426@gmail.com> * Deleted attachments Deleted unnecessary `"attachment"{}` lines * Changed --logdir to use `default_root_dir` variable * Use logdir with `default_root_dir` * add tensorboard to `release.yaml` and `workflows.yaml` * add tensorboard to pip install * remove unnecessary tensorboard installs * Update trainers.ipynb --------- Co-authored-by: Adam J. Stewart <ajstewart426@gmail.com>
Added
TensorBoardLogger
and removedCSVLogger
Closes #1163