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

Getting rid of "module." heritage #1184

Merged
merged 25 commits into from
Jun 28, 2023
Merged

Getting rid of "module." heritage #1184

merged 25 commits into from
Jun 28, 2023

Conversation

BloodAxe
Copy link
Contributor

@BloodAxe BloodAxe commented Jun 16, 2023

What this PR does

We do not store state_dict of the model with "module." prefix anymore

Why this is important

  • When training with DDP/DP mode, DistributedDataParallel/DataParallel wrappers adds this "module." prefix unintentionally to state_dict. So there goes discrepancy when the model is trained with or without DDP. Previously SG was solving this with a hack of adding artificial "module." prefix to all saved models in single GPU mode. However, this is a broken design

  • Some of our checkpoints are saved with and some are saved without "module." which I believe the main cause we have this ugly NO_KEY_MATCHING checkpoint loading hack. The actual reasoning is lost in time, but I'm, pretty sure this is because of this discrepancy of saving models with and without DDP wrapper.

Solution

  1. This PR introduce get_real_model method to return the "real" model if it is wrapped with DP/DDP. Whenever there is a need to save state_dict one should use this method to unwrap the model and save state dict of the real one.
  2. Similarly, when loading a checkpoint same logic should be performed: get_real_model(net).load_state_dict(...)
  3. Compatibility with existing checkpoints that may or may not has "module." prefix is secured buy checking whether all state_dict keys are starting with "module.". If so - we drop them and load checkpoint as usual. This handles the case when checkpoint was saved with DDP wrapper.

Testing

Not properly tested yet
Components requiring special attention

  • Regular trainer
  • KDTrainer
  • EMA
  • Pretrained models
  • Unit Tests
  • Integration Tests
  • Shortened Recipe Tests

Risks of introducing breaking changes

No risk assessment has been done yet

Related issues

#1163
#1153

Copy link
Contributor

@Louis-Dupont Louis-Dupont left a comment

Choose a reason for hiding this comment

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

Looks great, I am just not sure about the name get_real_model, I think it is pretty confusing.
Maybe unwrap_model? It's also not that good, but maybe slightly more explicit? Any better idea?

@BloodAxe
Copy link
Contributor Author

unwrap_model is good, I like this name)

@BloodAxe BloodAxe marked this pull request as ready for review June 27, 2023 08:25
Louis-Dupont
Louis-Dupont previously approved these changes Jun 27, 2023
Copy link
Contributor

@Louis-Dupont Louis-Dupont left a comment

Choose a reason for hiding this comment

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

LGTM

ofrimasad
ofrimasad previously approved these changes Jun 28, 2023
Copy link
Collaborator

@ofrimasad ofrimasad left a comment

Choose a reason for hiding this comment

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

LGTM
not sure if the Makefile was changed by mistake

Makefile Show resolved Hide resolved
BloodAxe added 2 commits June 28, 2023 11:01
# Conflicts:
#	src/super_gradients/training/sg_trainer/sg_trainer.py
@BloodAxe BloodAxe dismissed stale reviews from ofrimasad and Louis-Dupont via c340da8 June 28, 2023 08:04
ofrimasad
ofrimasad previously approved these changes Jun 28, 2023
Copy link
Collaborator

@ofrimasad ofrimasad left a comment

Choose a reason for hiding this comment

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

LGTM

# Conflicts:
#	src/super_gradients/training/sg_trainer/sg_trainer.py
Copy link
Contributor

@Louis-Dupont Louis-Dupont left a comment

Choose a reason for hiding this comment

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

LGTM

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