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

Switch from print to logging #282

Closed
williamFalcon opened this issue Oct 2, 2019 · 16 comments · Fixed by #457
Closed

Switch from print to logging #282

williamFalcon opened this issue Oct 2, 2019 · 16 comments · Fixed by #457
Labels
feature Is an improvement or enhancement help wanted Open to be worked on

Comments

@williamFalcon
Copy link
Contributor

williamFalcon commented Oct 2, 2019

Use logging instead of print. (@Ir1d you brought this up before)

Suggested by @adefazio

@williamFalcon williamFalcon added feature Is an improvement or enhancement help wanted Open to be worked on labels Oct 2, 2019
@williamFalcon williamFalcon added this to Todo (next release) in Key features - Roadmap v1.0 Oct 4, 2019
@williamFalcon williamFalcon moved this from Todo (next release) to backlog in Key features - Roadmap v1.0 Oct 4, 2019
@humandotlearning
Copy link
Contributor

humandotlearning commented Oct 30, 2019

hey @williamFalcon could you just guide me to the files that needs print to be changed to logging and i'll do it. Thanks.

@Borda
Copy link
Member

Borda commented Oct 30, 2019

I think that we were talking about it already a few months back :] happy to see it pass O:-)
@williamFalcon could you draw what kind of messages should be info and debug?

@humandotlearning please use lazy formating

logging.info('Epoch %05d: %s did not improve', epoch + 1, self.monitor)

see: https://stackoverflow.com/a/52012660/4521646

@Ir1d
Copy link
Contributor

Ir1d commented Nov 4, 2019

I suppose f-strings are preferred?
should we exposure logger as an option, since I prefer loguru, while others might like something else.

@Ir1d
Copy link
Contributor

Ir1d commented Nov 4, 2019

Given a second thought, what about adding a info and some debug function into LightningLoggerBase?
Something like: self.logger.info() , and users will be free to use whatever logger they like by specifying in custom logger.

However: after a quick search. their are certain functions which dont have access to LoggerBase at this moment. For example the callback

@williamFalcon
Copy link
Contributor Author

Please use f-strings!

@humandotlearning
Copy link
Contributor

okay as @williamFalcon suggested i'll go with f-string. I have done a performance comparison if anyone is interested.
https://colab.research.google.com/drive/1J6-SFq5nDGTYDH7GIpzixbjYS0UvZ30E

@Borda
Copy link
Member

Borda commented Nov 5, 2019

@humandotlearning @lr1d already started in #457

@Ir1d
Copy link
Contributor

Ir1d commented Nov 5, 2019

actually I'm stuck with how to let users select custom printing function? Any idea on this?

@Borda
Copy link
Member

Borda commented Nov 5, 2019

@Ir1d what do you mean by custom printing function? logging.info/debug/warning / print()... ?

@Ir1d
Copy link
Contributor

Ir1d commented Nov 5, 2019

@Borda I mean using other libraries such as loguru. But yes, what if someone wants to use debug/warning or other stuffs

@Borda
Copy link
Member

Borda commented Nov 5, 2019

it is nice library, but I would follow the same strategy as for PyTorch, do not include anything extra until it is really needed so I would sta just with the standard logging , @williamFalcon ?

Key features - Roadmap v1.0 automation moved this from backlog to Done Nov 5, 2019
@Borda
Copy link
Member

Borda commented Nov 7, 2019

@Ir1d
Copy link
Contributor

Ir1d commented Nov 7, 2019

@Borda Yes, and I believe @williamFalcon is aware of this fact 😂

@ethanjperez
Copy link

It's possible to use f2format to convert f-strings to str.format, to be compatible with Python <=3.5

@williamFalcon
Copy link
Contributor Author

@ethanjperez amazing haha. it’s finally happening :)

i think it’s a good place to start for a PR @ethanjperez ?

@Borda
Copy link
Member

Borda commented Nov 25, 2019

@ethanjperez it is true, but I am not sure that we do not want to have this "".format() formatting...
it works exactly as it is written in the package description:

# the original code
var = f'foo{(1+2)*3:>5}bar{"a", "b"!r}boo'
# after `f2format`
var = 'foo{:>5}bar{!r}boo'.format((1+2)*3, ("a", "b"))

@williamFalcon ^^

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Is an improvement or enhancement help wanted Open to be worked on
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

5 participants