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

Do not use wandb if not specified in the config file #1253

Merged
merged 9 commits into from Jan 17, 2023

Conversation

jcohenadad
Copy link
Member

@jcohenadad jcohenadad commented Jan 2, 2023

Checklist

GitHub

  • I've given this PR a concise, self-descriptive, and meaningful title
  • I've linked relevant issues in the PR body
  • I've applied the relevant labels to this PR
  • I've assigned a reviewer

PR contents

Description

#1252, even if the config file does NOT include any wandb field, training with ivadomed can use wandb in case wandb logging was previously done, indicates that deliberate omission of the value to wandb_api_key while logging in and the anonymous logging feature introduced in #1193 could sometimes be misleading and undesired. Hence, this PR undoes and simplifies it by bypassing wandb.login call if the key is not defined or is empty.

Linked issues

Resolves #1252

@coveralls
Copy link

coveralls commented Jan 2, 2023

Pull Request Test Coverage Report for Build 3933753469

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 74.114%

Totals Coverage Status
Change from base Build 3915337488: 0%
Covered Lines: 4621
Relevant Lines: 6235

💛 - Coveralls

@mariehbourget mariehbourget modified the milestone: new release Jan 4, 2023
@jcohenadad jcohenadad requested review from kanishk16 and removed request for dyt811 January 9, 2023 23:33
@jcohenadad
Copy link
Member Author

@kanishk16 do you mind reviewing this? if you cannot please let me know

@kanishk16
Copy link
Contributor

I'm sorry to stall the progress... On it right away... weirdly all the notifications were sent to my spam

Copy link
Contributor

@kanishk16 kanishk16 left a comment

Choose a reason for hiding this comment

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

#1252 indicates that deliberate omission of the value in wandb_api_key while logging in and the anonymous logging feature introduced in #1193 could sometimes be misleading and undesired. Hence, this undoes and simplifies it.

We'd also like to remove this from the documentation here. Specifically, the following should be removed from the docs:

From then on, the value in this key-value pair in the config file could be omitted, like 'wandb_api_key': ''

ivadomed/utils.py Outdated Show resolved Hide resolved
@jcohenadad
Copy link
Member Author

We'd also like to remove this from the documentation here. Specifically, the following should be removed from the docs:

Yes! excellent catch

@kanishk16 kanishk16 self-requested a review January 16, 2023 17:44
Copy link
Contributor

@kanishk16 kanishk16 left a comment

Choose a reason for hiding this comment

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

LGTM 🚀 Will merge once tests pass!

@kanishk16 kanishk16 merged commit 8b70c30 into master Jan 17, 2023
@kanishk16 kanishk16 deleted the jca/1252-wandb-force branch January 17, 2023 18:47
@joshuacwnewton joshuacwnewton added the enhancement category: improves performance/results of an existing feature label Dec 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement category: improves performance/results of an existing feature wandb
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Do not use wandb if not specified in the config file
5 participants