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

wandb/tensorboard loggers set default init to False #1235

Merged
merged 2 commits into from
Mar 11, 2025

Conversation

brian-dellabetta
Copy link
Collaborator

SUMMARY:
If wandb is installed, an empty logging config will default to including wandb, causing workflows to hang on the following prompt:

wandb: Using wandb-core as the SDK backend.  Please refer to https://wandb.me/wandb-core for more information.
wandb: (1) Create a W&B account
wandb: (2) Use an existing W&B account
wandb: (3) Don't visualize my results
wandb: Enter your choice: 

This resolves by setting default behavior to False, expecting user to provide a config in order to use wandb.
Resolves #976
Resolves #934

TEST PLAN:
No new src code

Copy link

github-actions bot commented Mar 7, 2025

👋 Hi! Thank you for contributing to llm-compressor. Please add the ready label when the PR is ready for review.

Note: This is required to complete the testing suite, please only add the label once the PR is code complete and local testing has been performed.

@brian-dellabetta brian-dellabetta added the ready When a PR is ready for review label Mar 7, 2025
@brian-dellabetta brian-dellabetta enabled auto-merge (rebase) March 7, 2025 20:06
@markurtz
Copy link
Collaborator

@brian-dellabetta do we not have a better pathway for determining if wandb is configured and in a non-blocking usable state? Since the user doesn't have easy pathways into configuring the loggers currently, especially for our oneshot APIs, I'm a bit cautious of essentially removing metric logging for those completely.

Separately, though, we do need to refactor how we're handling loggers and their general setup, so that can wait til then.

@brian-dellabetta
Copy link
Collaborator Author

@brian-dellabetta do we not have a better pathway for determining if wandb is configured and in a non-blocking usable state? Since the user doesn't have easy pathways into configuring the loggers currently, especially for our oneshot APIs, I'm a bit cautious of essentially removing metric logging for those completely.

Separately, though, we do need to refactor how we're handling loggers and their general setup, so that can wait til then.

@markurtz this part of the codebase is super opaque to me, and because oneshot just takes kwargs it is not clear to me where a user could configure this. Right now this is blocking two users from running anything, so this is a hotfix that should be considered more deeply later on (probably overlapping current work of Kyle and George for their refactoring).

But i think this indicates there is a whole lot of code involved in training that axolotl/llm-foundry/instructlab take care of, and that we're better off connecting into them for more sophisticated workflows that take care of all that

@brian-dellabetta brian-dellabetta merged commit 64175da into main Mar 11, 2025
7 of 8 checks passed
@brian-dellabetta brian-dellabetta deleted the bdellabe/fix-wand-init-error branch March 11, 2025 04:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready When a PR is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wandb logging cannot be disabled Several wandb init
5 participants