Skip to content

Conversation

@tamarl08
Copy link
Contributor

Add option to log to W&B from rsmtool.py.

Note: I didn't add this to unit tests since it only allows a very shallow testing. Instead I added a tutorial config that logs to wandb and verified that everything works.
See the results of the most recent run here or try it yourself!

"use_scaled_predictions": false,
"use_thumbnails": false,
"use_truncation_thresholds": false
"use_truncation_thresholds": false,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Aha!

@codecov
Copy link

codecov bot commented Jul 17, 2023

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (1e48664) 95.87% compared to head (0b3760e) 95.87%.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #617   +/-   ##
=======================================
  Coverage   95.87%   95.87%           
=======================================
  Files          59       59           
  Lines        9300     9339   +39     
=======================================
+ Hits         8916     8954   +38     
- Misses        384      385    +1     
Impacted Files Coverage Δ
rsmtool/configuration_parser.py 88.44% <100.00%> (+0.11%) ⬆️
rsmtool/rsmtool.py 99.23% <100.00%> (+0.10%) ⬆️
rsmtool/utils/constants.py 100.00% <100.00%> (ø)
rsmtool/writer.py 96.36% <100.00%> (+0.36%) ⬆️
tests/test_configuration_parser.py 99.80% <100.00%> (+<0.01%) ⬆️
tests/test_experiment_rsmtool_4.py 97.18% <100.00%> (+0.51%) ⬆️

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@desilinguist
Copy link
Collaborator

I think we will need to add tests, @tamarl08. That's not a minor change in coverage.

@tamarl08
Copy link
Contributor Author

Thanks for the review @desilinguist @mulhod! I addressed all your comments, leaving the dependency as it is for now.

@desilinguist
Copy link
Collaborator

Okay, just tested this. Couple of things that we should add to the documentation:

  1. I hadn't set up my W&B API key and so I just got this weird menu asking me to choose one of three options (create account, use existing account etc.). We should add a link to Step 2 and tell folks that they should have completed this step before they start using W&B.

  2. If you enable W&B logging, the rsmtool run will take significantly longer due to the network traffic being sent to W&B. We should mention this.

Both of these can go under an "Important" note in the use_wandb section of the documentation.

Copy link
Collaborator

@desilinguist desilinguist left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!! 🎉

@tamarl08 tamarl08 merged commit 5929f4c into main Jul 19, 2023
@delete-merged-branch delete-merged-branch bot deleted the 597-add-wandb-rsmtool branch July 19, 2023 15:07
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.

4 participants