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

Remove extra log in line with rusheb discussion #131

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

Conversation

luciaquirke
Copy link
Contributor

@luciaquirke luciaquirke commented Mar 26, 2023

  • summary currently included in serialize method
  • split out W&B client and logger
    Next:
  • add config files and dataset reference to the model artifact

@luciaquirke luciaquirke requested a review from rusheb March 26, 2023 05:41
@@ -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(
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

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

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

@rusheb rusheb self-requested a review March 26, 2023 10:07
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@luciaquirke luciaquirke force-pushed the luciaq-fixup-wandb branch 4 times, most recently from 5f63cca to cd02666 Compare April 1, 2023 07:40
@luciaquirke luciaquirke requested a review from rusheb April 1, 2023 07:40
@rusheb
Copy link
Collaborator

rusheb commented Apr 12, 2023

@luciaquirke would you still like to try and merge this or should we close it?

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