[wandb] restore WANDB_DISABLED=true to disable wandb

[stas00]

This PR

• extends ENV_VARS_TRUE_VALUES with "true"
• restores WANDB_DISABLED=true to disable wandb
• documents this exact setting
• syncs trainer_tf with the same solution.

Context: we are still dealing with #9623 where wandb fails no matter if you have it installed or not.

It looks like due to #9699 a few days ago this behavior was changed to be one of ENV_VARS_TRUE_VALUES = {"1", "ON", "YES"}. And it's not documented anywhere.

This PR tries to restore the original behavior where any value of WANDB_DISABLED should disable wandb.

And wandb integration is broken, that's why we need a way disable it - it's so annoying when trying to develop and wandb keeps on breaking things whether it's installed or not. See: #9623

Alternatively, instead of the proposed change in this PR, let's document this API that it has to be on of `{"1", "ON", "YES"}``, so that it doesn't change from day to day.

@sgugger

[sgugger]

If I understood correctly the user's issue, the problem was that any value was accepted. We can add "True" in ENV_VAR_TRUE_VALUES which seems to be missing, but if I set WAND_DISABLED=False for instance, I would expect wandb to not be disabled.

In any case those env variables are now deprecated (have to make a PR to issue a proper warning) since we have the report_to training argument that allows the user to set the reporting platform they want to use.

[stas00]

If I understood correctly the user's issue, the problem was that any value was accepted. We can add "True" in ENV_VAR_TRUE_VALUES which seems to be missing, but if I set WAND_DISABLED=False for instance, I would expect wandb to not be disabled.

That is how I implemented it originally for this PR, but then read user's issue that triggered the PR that broke the original setting, and the issue writer requested a plain - any WANDB_DISABLED value. I'm fine with either. Do you want me to recode it to add True?

Also it needs to be documented, so that this disabling is solid and doesn't get changed again and again. If it's documented with just Yes that is already supported that is good enough for me.

In any case those env variables are now deprecated (have to make a PR to issue a proper warning) since we have the report_to training argument that allows the user to set the reporting platform they want to use.

Well, except this new feature doesn't help in this particular case. As you can see from #9623 and problems as recent as yesterday wandb is still a problem, even if you don't purposefully activate it or even have it installed. I won't be trying to fix this if it worked.

Perhaps the default report_to should be None and have an option for All to ease up for those who want them all?

Whatever the outcome, please let's fix so that if one doesn't have wandb installed it shouldn't break things.

Thank you.

[sgugger]

Do you want me to recode it to add True?

Yes, just as I said, adding True to the ENV_VAR_TRUE_VALUES should be enough to have this work (it's an oversight that True is not in that constant).

Also it needs to be documented, so that this disabling is solid and doesn't get changed again and again. If it's documented with just Yes that is already supported that is good enough for me.

By all means, please add documentation in this PR. For now it's documented with the callback but I'm open to any suggestion to make this better.

Whatever the outcome, please let's fix so that if one doesn't have wandb installed it shouldn't break things

The bug in #9623 with wandb not installed is linked to something weird in your env as I haven't been able to reproduce it by following your steps. I can add stronger checks that wandb is a proper module by checking its version/authors (like is done for datasets but I have no idea if it will solve your bug or not (since I have no reproducer on my side).

If wandb is installed and you pass along --report_to [], you should not see either

wandb.errors.error.Error: You must call wandb.init() before wandb.log()

nor

AttributeError: module 'wandb' has no attribute 'ensure_configured'

as the callback is not passed to the Trainer.

Perhaps the default report_to should be None and have an option for All to ease up for those who want them all?

As I explained before, that switch will be done in v5, as it is a breaking change.

[stas00]

Thank you for the feedback, @sgugger - PR updated as requested, plus synced trainer_tf with the same solution.

[sgugger]

LGTM, thanks for adjusting!

[LysandreJik]

LGTM!