Skip to content

Conversation

@FraPochetti
Copy link
Contributor

…the WandbCallback

This one addresses issue #490

@codecov
Copy link

codecov bot commented Nov 3, 2020

Codecov Report

Merging #524 into master will decrease coverage by 0.15%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #524      +/-   ##
==========================================
- Coverage   86.63%   86.47%   -0.16%     
==========================================
  Files          91       91              
  Lines        2184     2189       +5     
==========================================
+ Hits         1892     1893       +1     
- Misses        292      296       +4     
Flag Coverage Δ
unittests 86.47% <50.00%> (-0.16%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
icevision/models/rcnn/fastai/learner.py 82.60% <50.00%> (-17.40%) ⬇️
icevision/core/mask.py 86.82% <0.00%> (-0.30%) ⬇️
icevision/models/rcnn/mask_rcnn/model.py 100.00% <0.00%> (ø)
icevision/models/rcnn/faster_rcnn/model.py 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update be2c4ba...c266ea3. Read the comment docs.

Copy link
Collaborator

@lgvaz lgvaz left a comment

Choose a reason for hiding this comment

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

Looks good! Just a question before we merge this, if instead of patching we use log=None, is the bug solved?

@FraPochetti
Copy link
Contributor Author

if instead of patching we use log=None, is the bug solved?

Nope. That just doesn't log the gradients.

@lgvaz lgvaz linked an issue Nov 3, 2020 that may be closed by this pull request
@lgvaz
Copy link
Collaborator

lgvaz commented Nov 3, 2020

Okay, makes a lot of sense!!

So, the final thing before we merge, can you add a logger.warning if if len(is_wandb) == 1:?

It would be nice to open a new issue in icevision to show your findings, you can just copy and paste the messages you sent me on discord, explaining that torchvision rcnn models are having a problem with the wandb hooks and the best quick solution we found is to simply don't add the hooks

Then in the warning message you could say something like "Wandb quickfix implemented, for more info check issue #number_of_issue"

@FraPochetti
Copy link
Contributor Author

Sure will do!
Shall I just document my findings here or create a new issue from scratch?

@FraPochetti
Copy link
Contributor Author

I added a final comment to this issue describing what we did.
Is this a sufficient level of detail?

@lgvaz
Copy link
Collaborator

lgvaz commented Nov 3, 2020

The level of detail is perfect!

Open another issue because the original one contains useless information and will be closed by this PR. It's easier to have a direct and clear issue for further reference =)

@FraPochetti
Copy link
Contributor Author

Done.
Issue created and PR updated.

Copy link
Collaborator

@lgvaz lgvaz left a comment

Choose a reason for hiding this comment

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

Looks perfect! Thanks SO MUCH again Francesco, as I said before, the amount of line changes doesn't reflect at all the work put into this ❤️

@lgvaz lgvaz merged commit 91e86c9 into airctic:master Nov 3, 2020
@FraPochetti FraPochetti deleted the wandb_bug branch November 4, 2020 08:44
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.

Wandb + fastai + RCNN error

2 participants