Fix comet_ml import and add ensure availability #7933
Conversation
|
FYI: @stas00 |
src/transformers/integrations.py
Outdated
| if comet_ml.config.get_config("comet.api_key"): | ||
| _has_comet = True | ||
| else: | ||
| logger.warning("comet_ml is installed but `COMET_API_KEY` is not set.") |
There was a problem hiding this comment.
excellent, but IMHO this shouldn't be of level warning() - it's an optional feature and it shouldn't be noisy if some other package installed comet_ml as an automatic dependency and the user doesn't have it configured and has no idea why she gets this warning.
Any level above will do, since warning is the default level in transformers
There was a problem hiding this comment.
I merely duplicated the logic and level from the wandb logger (below this code). Let me know if you want to change both, or leave them as-is.
There was a problem hiding this comment.
I may be in minority of someone who looks at test logs, that's why I have been trying to clean up a lot of those unnecessary warnings.
There was a problem hiding this comment.
The reasoning for the Wandb warning if that if users installed it, they probably want to use it, so should be aware there is something missing. That being said, I'm not aware of people getting wandb installed as a dependency from another lib.
The same logic applies to comet_ml so the warning makes sense if someone has not finished setting the API but at the same time, I understand your point @stas00.
I don't think there is a way to satisfy everyone, so I'd err on the side of what makes life easier for beginners, which is to leave the warning. We can add an env variable to ignore one or the other integration (I think there is one for Wandb already) and test that env variable before raising the warning, so that the experienced user has a way of shutting up an integration they don't want.
How does that sound?
There was a problem hiding this comment.
FWIW, I know for sure that I haven't installed comet_pl and yet it's there. I have recently reset my conda env and currently don't have this package, so it might take time before I find which package forced it on me.
Normally one can find it with:
pip install pipdeptree
pipdeptree --reverse --packages comet_ml
but only after the package in question has been installed.
There was a problem hiding this comment.
I still don't understand what is the purpose of these warnings - if a user expects wandb/comet ml to work w/o the keys - it's not going to happen, so surely they can figure it out. Requiring users who don't use these modules endure warnings and forcing them to either succumb and register with those both services or seek out how to shut them down is not cool. The user who has an issue sorting out how to make them work, will face this once. The user who doesn't want to become wandb/comet_ml user will have this warning happening all the time. Please re-think this.
I'd add an env var to activate the warning as a debug measure for someone who needs it once - not to turn it off.
There was a problem hiding this comment.
Weird! I can't think of any standard package that has comet_ml as a dependency. Let us know if you find it :)
There was a problem hiding this comment.
Sitting on this some more, I am feeling I'm being difficult here, so I will let it go and trust that it'll sort itself out in a good time. Thank you for letting me share my thoughts.
There was a problem hiding this comment.
LGTM but I agree with @stas00: I wouldn't want to see warnings about cometml or wandb if I don't have the libraries installed.
Looking at the code, however, it seems weird that you're still getting warnings @stas00 as if it's not in your env it should raise an exception which wouldn't trigger the warnings.
This is not the case already. We were talking about the odd case where some other package installed one of these as its auto-dependencies. So the user now unwittingly needs to figure out why in the world she needs to get an API key for something she didn't ask for in first place. Unfortunately I am forced to reset my conda env a lot recently, so I lost the one where this exact scenario has happened, so at the moment I can't point the guilty finger at which package installed Otherwise all is good. |
|
Resolved merge conflicts. Should be ready to go. |
|
Thanks! |
What does this PR do?
The current version 3.4.0 is broken, and can not be used with comet_ml without a workaround. This PR fixes that.
Before submitting
Pull Request section?
to the it if that's the case.
documentation guidelines, and
here are tips on formatting docstrings.
Who can review?
Trainer: @sgugger