-
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
Add support for multiple loggers #903
Add support for multiple loggers #903
Conversation
Hello @ethanwharris! Thanks for updating this PR. There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2020-02-25 18:55:02 UTC |
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.
Nice work! it is just my personal taste, I would simplify the class names...
pytorch_lightning/loggers/base.py
Outdated
|
||
@property | ||
def experiment(self): | ||
return [logger.experiment() for logger in self._logger_list] |
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 may be tricky since it returns list instead object...
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.
Yeah, this is really just a precaution anyway as we don't use the experiment
property internally. However, the experiment
property is a different object for every logger so I think this is acceptable
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.
I know, but we do not know about all user usage of these loggers lol
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.
I'm not sure there's any other option here, any suggestions?
we shall resolve the MNIST issue very soon #859 |
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.
Great job with adding types! 🚀
pls have look at my suggestion with an abstract method
Ok, have fixed those issues - this should be ready to go :) |
Perfect, but rather I would wait till master is fixed, @williamFalcon #938 |
@ethanwharris @Borda tests are fixed. let's rebase! |
@williamFalcon done but does not show any change, strange... |
@Borda That should have fixed it, hopefully the tests will run now :) |
@ethanwharris this is awesome. Can you also make the appropriate changes to: |
@williamFalcon Done :) |
@ethanwharris amazing! |
@ethanwharris Very nice addition!! You even annotated the types :) |
@ethanwharris is making our code look sloppy by comparison haha. |
* Add support for multiple loggers * Fix PEP * Cleanup * Cleanup * Add typing to loggers * Update base.py * Replace duck typing with isinstance check * Update CHANGELOG.md * Update comet experiment type, Switch to abstractmethod in logging.py * Fix test * Add passes to LightningLoggerBase * Update experiment_logging.rst
Before submitting
What does this PR do?
Fixes #317
PR review
Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.
Did you have fun?
Make sure you had fun coding 🙃