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

Allow usage of any function as log printer #70

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

halcy
Copy link

@halcy halcy commented Oct 28, 2015

Allow passing a function to be used for log output in place of the print() function (Relating to #36 )

@Qwlouse
Copy link
Collaborator

Qwlouse commented Oct 28, 2015

Thank you for our first feature PR! As mentioned in #36 we agree on somehow integrating that possibility.
However passing a print_function separately to every hook seems inconvenient and adds lots of extra code to all the hooks.
We've discussed a bit about how to do this in a more elegant way by having the trainer to pass an actual logger object to all the hooks as an argument for Hook.start. This way we can just specify the logger in the trainer and it will automatically be used by all the hooks, while retaining the option to set it individually.
If you are willing to work on this cleaner integration, we'd be happy to guide you.

@halcy
Copy link
Author

halcy commented Oct 29, 2015

Hm, would you pass a full-blown object that does more than just print, or simply a function (such as print or, as I am doing right now with my variant, logging.debug)? I somewhat like the relatively minimalist latter approach.

Definitely a lot nicer to have it in Hook.start, in any case.

edit: having an object that can do slightly more would have the advantage of being able to have more than one error stream (separate handling of things that right now go to either stdout or stderr, which otherwise would be merged).

@flukeskywalker
Copy link
Collaborator

Passing a Logger object is nicer. Additionally, Hook.message() can take an optional level argument (perhaps "info" by default) so that it can log at specified levels. This way, any hook can log at multiple levels if needed.

The other main logging entity is the Trainer, so it should probably also have a message() method similar to Hook.

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.

3 participants