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.

Sign up for GitHub

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

Jump to bottom

Refactor PyTorch for explicit Lighnting/Wandb hyperparameters #30

Merged
merged 34 commits into from
Oct 10, 2022
Merged

Refactor PyTorch for explicit Lighnting/Wandb hyperparameters #30

merged 34 commits into from
Oct 10, 2022

Conversation

mwalmsley
Copy link
Owner

@mwalmsley mwalmsley commented Oct 3, 2022 •
edited
Loading

Major changes

  • Refactor pytorch define_model API to pass all hparams as explicit simple variables (e.g. architecture_name) rather than convenient but implicit functions (e.g. loss_function, model). This is messier in that it requires a lot of args, but allows for tracking/sweeps by wandb and restoring from checkpoints by lightning.
  • Add stochastic_depth_probability as a hyperparameter. In short, this sometimes randomly skips whole blocks. Was previously set and on by default - should be no change to users, but can now be altered if desired.
  • Add efficientnetb2 and b4 as options. No performance improvement on my current tests.
  • Add predict_on_catalog.py for pytorch. Allows easy predictions on a catalog dataframe.

Minor (but potentially breaking) changes

  • Rename pytorch model_architecture to architecture_name. Clearer that it is not the architecture func. itself. (may break api)
  • Deprecate requirements.txt
  • Set explicit lightning version (may break a build)
  • Bump version ahead of pypi/pip

cc @patrikasvanagas, @camallen

@mwalmsley
add docs note
eab91aa
@mwalmsley
Begin adding pytorch prediction method
fd243ea
@mwalmsley
use predict options
681ed39
@mwalmsley
unpack batch better
cce0473
@mwalmsley
cast to numpy
5296234
@mwalmsley
debug shapes
a6b82e3
@mwalmsley
Remove old evaluation scripts for clarity
5c96aa4
@mwalmsley
try debug batch predictions
5dad673
@mwalmsley
debug
60c40f1
@mwalmsley
oops wrong file
10c174d
@mwalmsley
tweak
a9e5a13
@mwalmsley
Merge pull request #27 from mwalmsley/main
275eac1 
Include Cam's latest features on generic dev
@mwalmsley
Add b2 option, fix seed
89f0dc9
@mwalmsley
try 1408 rep for b2, 1.1 * 1280
9a89a0b
@mwalmsley
missed an el
1f933ff
@mwalmsley
try explicitly return best model
7994f45
@mwalmsley
try again
c090d5e
@mwalmsley
Try hparams via lightning
38b805c
@mwalmsley
disable fitting, temporarily
ee9a1d6
@mwalmsley
try with class itself
5377a1f
@mwalmsley
debug which checkpoint, last_ gives dir?!
a6e7142
@mwalmsley
hmm, maybe should be string names
a370ad3
@mwalmsley
refactor to inherit with hparams
0edf3a2
@mwalmsley
add output_dim
e0e90c6
@mwalmsley
debugging
1856ab1
@mwalmsley
typo
233edfa
@mwalmsley
okay, inits alright, does it load?
a6b7b6c
@mwalmsley
cleanup
f9ea51c
@mwalmsley
update reqs
25cf3f2
@mwalmsley
add drop_connect_rate, dropout as hparams
042d6e3
@mwalmsley mwalmsley self-assigned this Oct 3, 2022
camallen
camallen approved these changes Oct 5, 2022
View reviewed changes
Copy link
Collaborator

@camallen camallen left a comment

Choose a reason for hiding this comment

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

LGTM - fwiw i've been using this version of the code in the batch processing system

setup.py
@@ -26,7 +26,7 @@
'torch == 1.10.1',
'torchvision == 0.11.2',
'torchaudio == 0.10.1',
'pytorch-lightning',
'pytorch-lightning==1.6.5', # 1.7 requires protobuf version incompatible with tensorflow/tensorboard. Otherwise works.
Copy link
Collaborator

Choose a reason for hiding this comment

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

not blocking - in theory we could isolate the pytorch and tensorflow installs to their own python virtual envs, thus avoiding this conflict and allowing pytorch lightning to resolve as high as possible

Unless of course the tensorboard dependency i used in pytorch....then please ignore me.

Copy link
Owner Author

Choose a reason for hiding this comment

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

we could isolate the pytorch and tensorflow installs to their own python virtual envs

Good idea but I don't know how to do that.

In practice there is very little advantage to being on the absolute latest package and I suspect the tensorboard team will update their deps shortly.

zoobot/pytorch/training/train_with_pytorch_lightning.py
datamodule=datamodule,
ckpt_path='best' # can optionally point to a specific checkpoint here e.g. "/share/nas2/walml/repos/gz-decals-classifiers/results/early_stopping_1xgpu_greyscale/checkpoints/epoch=26-step=16847.ckpt"
)
# trainer.test(
Copy link
Collaborator

Choose a reason for hiding this comment

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

any reason to remove the test step of after fitting? Does this speed up the system but impact quality?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Testing routinely is bad practice as you may accidentally tune your hparams to overfit the model. I was being a bit lazy when adding here earlier. I have left it commented as example, with a warning note.

@mwalmsley mwalmsley merged commit c312a83 into main Oct 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@camallen camallen camallen approved these changes

Assignees

@mwalmsley mwalmsley

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mwalmsley @camallen