-
Notifications
You must be signed in to change notification settings - Fork 6
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
Remove extra log in line with rusheb discussion #131
base: main
Are you sure you want to change the base?
Conversation
@@ -37,7 +37,6 @@ def get_dataloader( | |||
dataset: MazeDataset, cfg: ConfigHolder, logger: WandbLogger | |||
) -> DataLoader: | |||
length_stats: StatCounter = StatCounter(dataset.get_all_lengths()) | |||
logger.summary({"dataset_seq_len_stats_summary": length_stats.summary()}) | |||
logger.summary( |
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.
logger.summary( | |
logger.progress( |
I reckon we should move it from the summary to a console log, because it clutters up the summary
Side-note, I'm kind of thinking the WandbLogger interface is a bit obscure. Maybe the progress
function should be called something different?
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.
Ah yeah I didn't catch the meaning of progress
. console
?
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.
Hmm I need to think more about this
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.
Python is so weird. ChatGPT tells me that this logging configuration
logging.basicConfig(
stream=sys.stdout,
level=logging.INFO,
format="%(asctime)s %(levelname)s %(message)s",
datefmt="%Y-%m-%d %H:%M:%S",
)
affects the root logger, so it will affects all logs including those outside of this module. Should we put this in a setup module to make that clear? Then we could call logging.info directly rather than having the static progress
method, which lets us decouple the console logger method from the W&B stuff.
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.
That makes sense, happy to do that. Thanks for looking into it!
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've moved it to the utils module for now instead of its own setup class to avoid over-engineering, and renamed the WandbLogger to WandbClient so it can hold non-logging wandb stuff
Co-authored-by: Rusheb Shah <[email protected]>
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
5f63cca
to
cd02666
Compare
cd02666
to
f89c42e
Compare
f89c42e
to
64f7881
Compare
@luciaquirke would you still like to try and merge this or should we close it? |
Next: