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

Disable wandb by default + complete integration tests #434

Merged
merged 12 commits into from
Aug 16, 2023

Conversation

s-maddrellmander
Copy link
Contributor

Changelogs

  • Making sure the default behaviour is sensible, i.e. users aren't having to bodge a lack of wandb accounts + login details / project access etc.
  • Adding tests for the new mode - ipu and cpu, complete integration tests for training + fine tuning

@codecov
Copy link

codecov bot commented Aug 14, 2023

Codecov Report

Merging #434 (737fe92) into main (71182f4) will increase coverage by 0.66%.
Report is 7 commits behind head on main.
The diff coverage is 20.00%.

@@            Coverage Diff             @@
##             main     #434      +/-   ##
==========================================
+ Coverage   64.74%   65.41%   +0.66%     
==========================================
  Files          89       89              
  Lines        8215     8218       +3     
==========================================
+ Hits         5319     5376      +57     
+ Misses       2896     2842      -54     
Flag Coverage Δ
unittests 65.41% <20.00%> (+0.66%) ⬆️

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

Components Coverage Δ
ipu 49.14% <ø> (ø)

tests/test_ipu_training.py Show resolved Hide resolved
graphium/cli/train_finetune.py Show resolved Hide resolved
tests/test_ipu_training.py Outdated Show resolved Hide resolved
@s-maddrellmander s-maddrellmander marked this pull request as ready for review August 15, 2023 13:58
Co-authored-by: Cas Wognum <caswognum@outlook.com>
Copy link
Collaborator

@DomInvivo DomInvivo 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! Minor changes only

Comment on lines 55 to 58
elif wandb_cfg is not None:
wandb.init(
entity=wandb_cfg["entity"],
project=wandb_cfg["project"],
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will allow to pass more than just the entity or the project.

Suggested change
elif wandb_cfg is not None:
wandb.init(
entity=wandb_cfg["entity"],
project=wandb_cfg["project"],
elif wandb_cfg is not None:
wandb.init(
config=cfg,
**wandb_cfg

# config is relative to a module
cfg = hydra.compose(
config_name="main",
overrides=[
Copy link
Collaborator

Choose a reason for hiding this comment

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

Also override

trainer.trainer.check_val_every_n_epoch = 1
trainer.trainer.precision = 32, # perhaps you can make this 32 for CPU and 16 for IPU

@s-maddrellmander s-maddrellmander merged commit d97f80c into main Aug 16, 2023
5 checks passed
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.

3 participants