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.

Sign up for GitHub

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

Jump to bottom

Remove extra log in line with rusheb discussion #131

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Remove extra log in line with rusheb discussion #131

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d987788
Remove extra log in line with rusheb discussion
luciaquirke Mar 26, 2023
d20721a
rusheb review
luciaquirke Mar 26, 2023
0d41139
Update maze_transformer/training/training.py
luciaquirke Mar 31, 2023
64f7881
remove logging terminology from wandb client
luciaquirke Apr 1, 2023
319b013
fixup
luciaquirke Apr 1, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion maze_transformer/training/training.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ 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.progress({"dataset_seq_len_stats_summary": length_stats.summary()})
logger.summary(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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?

Copy link
Contributor Author

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?

Copy link
Contributor Author

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

Copy link
Contributor Author

@luciaquirke luciaquirke Mar 26, 2023 •
edited
Loading

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.

Copy link
Collaborator

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!

Copy link
Contributor Author

@luciaquirke luciaquirke Apr 1, 2023 •
edited
Loading

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

{"dataset_seq_len_stats": length_stats.serialize(typecast=lambda x: str(x))}
)
luciaquirke marked this conversation as resolved.
Show resolved Hide resolved
Expand Down