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

fix the wandb logging issue #33464

Merged
merged 6 commits into from
Sep 18, 2024
Merged

Conversation

ZIYU-DEEP
Copy link
Contributor

What does this PR do?

Fixes #33320.

The existing error handling for wandb misses ConfigError when attempting to change the number of parameters, thus still blocking training; just added ConfigError to fix.

@vasqu
Copy link
Contributor

vasqu commented Sep 13, 2024

Duplicate of #33440?

@ZIYU-DEEP
Copy link
Contributor Author

ooooh same issue. the implementation difference is that ConfigError does not depend on self (cf., #33440 (comment)).

@vasqu
Copy link
Contributor

vasqu commented Sep 13, 2024

Is it guaranteed that we have access to the wandb pkg in the file? If so, I'd also appreciate your variant of doing it by top-level import. Maybe you can help out with the other PR so that we can concentrate our focus on one :)

@vasqu
Copy link
Contributor

vasqu commented Sep 16, 2024

Ohh whoops, ig the import can't be seen in __init__ for the subsequent use (at least by the linter used here).

Guess it has to be moved into setup after we guarantee the successful import of wandb, i.e.

if self._wandb is None:
return
self._initialized = True

So based on self._initialized / after passing the first check that wandb is not None, it would be safe to import then.

Copy link
Member

@LysandreJik LysandreJik left a comment

Choose a reason for hiding this comment

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

Thanks!

@LysandreJik
Copy link
Member

There are a few code quality issues as @vasqu mentions above and can be seen in the code quality check; please fix those and I'll merge the PR.

Copy link
Member

@LysandreJik LysandreJik left a comment

Choose a reason for hiding this comment

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

Thanks!

@LysandreJik LysandreJik merged commit 4f1e9ba into huggingface:main Sep 18, 2024
21 checks passed
@HuggingFaceDocBuilderDev

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

itazap pushed a commit to NielsRogge/transformers that referenced this pull request Sep 20, 2024
* fix the wandb logging issue

* handle ConfigError in WandbCallback; move import to local scope

* update integration_utils.py; move import of ConfigError

* Update integration_utils.py: remove trailing whitespace
amyeroberts pushed a commit to amyeroberts/transformers that referenced this pull request Oct 2, 2024
* fix the wandb logging issue

* handle ConfigError in WandbCallback; move import to local scope

* update integration_utils.py; move import of ConfigError

* Update integration_utils.py: remove trailing whitespace
BernardZach pushed a commit to BernardZach/transformers that referenced this pull request Dec 5, 2024
* fix the wandb logging issue

* handle ConfigError in WandbCallback; move import to local scope

* update integration_utils.py; move import of ConfigError

* Update integration_utils.py: remove trailing whitespace
BernardZach pushed a commit to innovationcore/transformers that referenced this pull request Dec 6, 2024
* fix the wandb logging issue

* handle ConfigError in WandbCallback; move import to local scope

* update integration_utils.py; move import of ConfigError

* Update integration_utils.py: remove trailing whitespace
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.

can't resume lora training due to wandb logging num params
5 participants