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

Add TrainsLogger #1020

Merged
merged 21 commits into from
May 10, 2020
Merged

Add TrainsLogger #1020

merged 21 commits into from
May 10, 2020

Conversation

jkhenning
Copy link
Contributor

Fixes #892

Description:

  • Adds TrainsLogger to support Trains Experiment Manager & Version Control, allowing you to log your experiment metadata to the Trains Server of your choice.
  • Includes all handlers for Output, Grads(scalars), Optimizer Params, Model Weights (scalars) as well as logging model checkpoints to the server.

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)

@jkhenning jkhenning mentioned this pull request May 7, 2020
@vfdev-5
Copy link
Collaborator

vfdev-5 commented May 7, 2020

@jkhenning thanks for the PR !
@erip could you please review. Thanks !

@vfdev-5
Copy link
Collaborator

vfdev-5 commented May 7, 2020

@jkhenning there is also a redefinition of test flake error to fix : https://travis-ci.org/github/pytorch/ignite/jobs/684300841#L238
Thanks !

@erip
Copy link
Contributor

erip commented May 7, 2020

Code looks clean and I didn't see any issues. One question for @jkhenning: why are you pinning to the release candidate in the requirements-dev.txt?

@jkhenning
Copy link
Contributor Author

@erip The reason for the release candidate is a small adjustment I had to make in the Trains binding code to make the integration cleaner. I wanted to make sure this PR was cleared so I can release the official Trains v0.14.3.
I'd appreciate it if you can wait with this PR's merge so I can release Trains v0.14.3 in the next day or two, change this dependency in the PR and it'll be ready to merge.

ignite/contrib/handlers/trains_logger.py Outdated Show resolved Hide resolved
ignite/contrib/handlers/trains_logger.py Show resolved Hide resolved
if artifact:
return artifact.get_local_copy()
self.task.get_logger().report_text("Can not find artifact {}".format(filename))
return None
Copy link
Contributor

Choose a reason for hiding this comment

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

This return isn't necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@erip
Copy link
Contributor

erip commented May 7, 2020

Excellent. I don't have write access, so @vfdev-5 will have to make sure this doesn't get merged until the new trains release is cut.

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.

LGTM! @jkhenning thanks !

@vfdev-5 vfdev-5 merged commit 15eeb87 into pytorch:master May 10, 2020
@jkhenning jkhenning deleted the trains-integration branch May 10, 2020 14:29
- ``TaskTypes.train``
- ``TaskTypes.testing``
- ``TaskTypes.inference``
report_freq (int): Optional. Histogram processing frequency (handle hist values every X calls to the handler).
Copy link
Collaborator

Choose a reason for hiding this comment

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

@jkhenning I was checking documentation layout of TrainsLogger and seems like I missed those args:

  • report_freq
  • histogram_update_freq_multiplier

In ignite we can easily filter out how frequently call an event handler, so, does report_freq param mean to call WeightsScalarHandler every report_freq times :

            trains_logger.attach(trainer,
                                 log_handler=WeightsScalarHandler(model),
                                 event_name=Events.ITERATION_COMPLETED(every=report_freq))

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @vfdev-5 ,

I think you're right, and it was my mistake to expose these values (they are related to the internal working of how we process and average the reports internally). I'll open a PR and remove them, if you'd like.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, if it does almost the same as event filtering, so, yes please send a PR and remove them...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@vfdev-5 opened #1036

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Trains logger
3 participants