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

Issue 73: Add Debug Logging #79

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

maupind
Copy link

@maupind maupind commented Feb 9, 2024

Added debug logging as per issue 73 for more verbose error messages. I added to all files except for trajectory_gpt2.py as this already had logging imported from transformers.utils.

I kept the logger.debug(f'foobar') as an example of usage, but happy to update this to have more file specific message if required.

@maupind maupind mentioned this pull request Feb 9, 2024
@eihli
Copy link
Contributor

eihli commented Feb 9, 2024

Anyone have any thoughts on what we should do with all of the existing calls to print(..)? I was thinking we could update them to log.info for now. info seems like a reasonable default level. And if anything jumps out as obviously debug or obviously error, then log.<level> appropriately.

@eihli
Copy link
Contributor

eihli commented Feb 9, 2024

TODO: I'm curious if we'll run into anything interesting when it comes to Accelerate and logging from multiple processes.

Also removes logging imports from files that don't do any logging
and removes the example comment. Both of those changes are just minor
non-functional changes to reduce the number of lines of non-functional code.
The logging module is standard to python and there are plenty of examples
in the official Python docs, so I don't think we gain much by including
examples in each file.
@eihli
Copy link
Contributor

eihli commented Feb 11, 2024

@maupind check out this diff and let me know what you think: maupind#1

@maupind
Copy link
Author

maupind commented Feb 12, 2024

@eihli Thanks for the update. I had meant to respond your comment on what to with the existing print statements, but got a little busy! The changes all seemed fine to me, but I am not an expert in logging.

Regarding the interaction with Accelerate and logging from multiple processes, is this something we can test through?

Trying to differentiate between logging and printing. Logging is for informational
and debug text. Printing is like a GUI. Printing is how you interact with the program
on every run. That's how I'm thinking about it in my head right now.
maupind added 2 commits March 13, 2024 12:17
Fix issue with previous PR. I should have made the PR into your issue branch rather than your main branch.
@maupind
Copy link
Author

maupind commented Mar 13, 2024

Apologies @eihli, I was away on leave and missed your previous request. Is there anything else required at this stage?

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.

2 participants