IPU Integration #7735
IPU Integration #7735
Conversation
# Conflicts: # pytorch_lightning/trainer/connectors/accelerator_connector.py
| return acc | ||
|
|
||
| def validation_epoch_end(self, outputs) -> None: | ||
| self.log('val_acc', torch.stack(outputs).mean(), prog_bar=True) |
There was a problem hiding this comment.
Need to clear up why this is here, not in the step itself (the step functions are jitted, and the outputs are collated from all devices, so mean averaging etc cannot happen within the functions)
Codecov Report
@@ Coverage Diff @@
## master #7735 +/- ##
=======================================
- Coverage 93% 87% -6%
=======================================
Files 202 205 +3
Lines 13121 13372 +251
=======================================
- Hits 12154 11623 -531
- Misses 967 1749 +782 |
| def on_reset_train_dataloader(self, dataloader) -> Union[Iterable, DataLoader]: | ||
| return self.process_dataloader(dataloader) | ||
|
|
||
| def on_reset_val_dataloader(self, dataloader) -> Union[Iterable, DataLoader]: | ||
| return self.process_dataloader(dataloader) | ||
|
|
||
| def on_reset_test_dataloader(self, dataloader) -> Union[Iterable, DataLoader]: | ||
| return self.process_dataloader(dataloader) | ||
|
|
||
| def on_reset_predict_dataloader(self, dataloader) -> Union[Iterable, DataLoader]: | ||
| return self.process_dataloader(dataloader) |
There was a problem hiding this comment.
Need to be pulled into the base plugin in a separate PR if we're comfortable with adding these hooks. Basically the reason these have been introduced is process_dataloader assumes that the dataset size doesn't change. If the size does change, the progress bar size is messed up.
These hooks happen early enough in the code that progress bar is correct. I also looked into moving process_dataloader but this is finnicky.
There was a problem hiding this comment.
instead of the multiple hooks, the method process_dataloader could simply be the one that goes into the base class.
There was a problem hiding this comment.
There was some confusion here; process_dataloader already exists in the TrainingTypePlugin
| from pytorch_lightning.plugins.precision.precision_plugin import PrecisionPlugin | ||
|
|
||
|
|
||
| class IPUPrecisionPlugin(PrecisionPlugin): |
There was a problem hiding this comment.
@awaelchli said before the next boilerplate precision plugin we should refactor to have the PrecisionPlugin inside of the TrainingTypePlugin, but maybe we can allow one more if this doesn't happen in time :P
| precision = self.lightning_module.trainer.accelerator.precision_plugin.precision | ||
| precision = 16 if self.half else precision | ||
|
|
||
| model = LightningIPUModule(self.lightning_module, precision) |
There was a problem hiding this comment.
We wrap the model in so many different places (for rerouting forward to the steps in DDP for example). Should we maybe do a wrapping like this all the time? So that we always reroute forward and are more consistent in our internals? (Not in this PR, just a general consideration)
# Conflicts: # .azure-pipelines/ipu-tests.yml # pytorch_lightning/plugins/training_type/training_type_plugin.py # pytorch_lightning/trainer/data_loading.py # pytorch_lightning/trainer/trainer.py # pytorch_lightning/trainer/training_loop.py
# Conflicts: # pytorch_lightning/accelerators/accelerator.py # pytorch_lightning/plugins/training_type/training_type_plugin.py
|
EDIT: closed and opened a new PR since a lot has changed :) |
What does this PR do?
Fixes #<issue_number>
Before submitting
PR review
Anyone in the community is free to review the PR once the tests have passed.
Before you start reviewing make sure you have read Review guidelines. In short, see the following bullet-list:
Did you have fun?
Make sure you had fun coding 🙃