Skip to content

Improved OutputHandler API#531

Merged
vfdev-5 merged 6 commits intopytorch:masterfrom
anmolsjoshi:feature/tensorboard_logger_fix
May 21, 2019
Merged

Improved OutputHandler API#531
vfdev-5 merged 6 commits intopytorch:masterfrom
anmolsjoshi:feature/tensorboard_logger_fix

Conversation

@anmolsjoshi
Copy link
Contributor

@anmolsjoshi anmolsjoshi commented May 21, 2019 •
edited
Loading

Fixes #530 #519

Description:

  • Updated BaseOutputHandler to remove another_engine and add global_step_transform
  • Updated OutputHandler of each logger accordingly
  • Updated SummaryWriter input as logdir, this seemed to be the breaking change in v1.7
  • Updated tests accordingly
  • Updated example to use updated API

Check list:

  • New tests are added (if a new feature is added)
  • New doc strings: description and/or example code are in RST format
  • Documentation is updated (if required)

@anmolsjoshi
Updated TensorboardLogger API to remove another_engine and use global…
9edbbcb 
…_step_transform instead. Added tests. Ensures that logger works with tensorboardX==1.7
@anmolsjoshi anmolsjoshi changed the title TensorboardLogger - Improved Improved OutputHandler API May 21, 2019
@anmolsjoshi anmolsjoshi requested a review from vfdev-5 May 21, 2019 06:13
vfdev-5
vfdev-5 reviewed May 21, 2019
View reviewed changes
Copy link
Collaborator

@vfdev-5 vfdev-5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! I was just wondering about the BC breaking change of API of loggers, as it is contrib we can change it without a warning...

@vfdev-5
Copy link
Collaborator

vfdev-5 commented May 21, 2019

Thinking more about this BC change, IMO it would be better to keep until 0.2.1 release the previous argument another_engine and show a deprecation. After the 0.2.1 release we'll remove it from the master.

@anmolsjoshi
Copy link
Contributor Author

anmolsjoshi commented May 21, 2019

@vfdev-5 I agree, I added another_engine back to the OutputHandler for each logger, reverted tests and examples as well.

@vfdev-5 vfdev-5 self-requested a review May 21, 2019 13:35
vfdev-5
vfdev-5 approved these changes May 21, 2019
View reviewed changes
Copy link
Collaborator

@vfdev-5 vfdev-5 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks Joshi!

@vfdev-5 vfdev-5 merged commit ee74e60 into pytorch:master May 21, 2019
@anmolsjoshi anmolsjoshi deleted the feature/tensorboard_logger_fix branch May 21, 2019 14:44
@g-karthik g-karthik mentioned this pull request Sep 12, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vfdev-5 vfdev-5 vfdev-5 approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Tensorboard logger won't work with tensorboardX 1.7 (latest release)

2 participants

@anmolsjoshi @vfdev-5