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

[WIP] Engines 3.0 #1230

Merged
merged 19 commits into from Jun 13, 2021
Merged

[WIP] Engines 3.0 #1230

merged 19 commits into from Jun 13, 2021

Conversation

Scitator
Copy link
Member

@Scitator Scitator commented Jun 6, 2021

Before submitting (checklist)

  • Was this discussed/approved via a Github issue? (no need for typos and docs improvements)
  • Did you read the contribution guide?
  • Did you check the code style? catalyst-make-codestyle && catalyst-check-codestyle (pip install -U catalyst-codestyle).
  • Did you make sure to update the docs? We use Google format for all the methods and classes.
  • Did you check the docs with make check-docs?
  • Did you write any new necessary tests?
  • Did you check that your code passes the unit tests pytest . ?
  • Did you add your new functionality to the docs?
  • Did you update the CHANGELOG?
  • Did you run colab minimal CI/CD with latest and minimal requirements?

Description

Related Issue

Type of Change

  • Examples / docs / tutorials / contributors update
  • Bug fix (non-breaking change which fixes an issue)
  • Improvement (non-breaking change which improves an existing feature)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

PR review

Anyone in the community is free to review the PR once the tests have passed.
If we didn't discuss your PR in Github issues there's a high chance it will not be merged.

FAQ

Please review the FAQ before submitting an issue:


if SETTINGS.fairscale_required:
from fairscale.nn import Pipe
from fairscale.nn.data_parallel import (
Copy link

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
N817 camelcase 'FullyShardedDataParallel' imported as acronym 'FSDP'

@@ -35,6 +35,14 @@ def _is_ddp_wrapped(model: nn.Module) -> bool:

parallel_wrappers = parallel_wrappers + (apex_DDP,)

if SETTINGS.fairscale_required:
from fairscale.nn.data_parallel import (
Copy link

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
N817 camelcase 'FullyShardedDataParallel' imported as acronym 'FSDP'

ShardedDataParallel as ShardedDDP,
)

parallel_wrappers = parallel_wrappers + (ShardedDDP, FSDP,)
Copy link

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
C819 trailing comma prohibited

from torch import nn, optim
from torch.utils.data import DataLoader

from catalyst import dl, utils
Copy link

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
F401 'catalyst.utils' imported but unused

from catalyst.data import ToTensor


class CustomRunner(dl.IRunner):
Copy link

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
D101 Missing docstring in public class

super().__init__()
self._logdir = logdir

def get_engine(self):
Copy link

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
D102 Missing docstring in public method

# return dl.SharedDataParallelFairScaleAMPEngine()
return dl.FullySharedDataParallelFairScaleEngine()

def get_loggers(self):
Copy link

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
D102 Missing docstring in public method

}

@property
def stages(self):
Copy link

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
D102 Missing docstring in public method

)
return model

def get_criterion(self, stage: str):
Copy link

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
D102 Missing docstring in public method

def get_criterion(self, stage: str):
return nn.CrossEntropyLoss()

def get_optimizer(self, stage: str, model):
Copy link

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
D102 Missing docstring in public method

def get_optimizer(self, stage: str, model):
return optim.Adam(model.parameters(), lr=1e-3)

def get_scheduler(self, stage: str, optimizer):
Copy link

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
D102 Missing docstring in public method

def get_scheduler(self, stage: str, optimizer):
return None

def get_callbacks(self, stage: str):
Copy link

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
D102 Missing docstring in public method

),
}

def handle_batch(self, batch):
Copy link

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
D102 Missing docstring in public method

@pep8speaks
Copy link

pep8speaks commented Jun 8, 2021

Hello @Scitator! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

Line 41:61: E231 missing whitespace after ','

Comment last updated at 2021-06-13 14:21:23 UTC

if SETTINGS.fairscale_required:
E2E.update(
{
"fs-pp": partial(dl.PipelineParallelFairScaleEngine, pipe_kwargs=dict(balance=[3, 4])),
Copy link

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
C408 Unnecessary dict call - rewrite as a literal.

# tested with `docker pull deepspeed/deepspeed:v031_torch17_cuda11`
if SETTINGS.deepspeed_required:
E2E.update(
{"ds-ddp": dl.DistributedDataParallelDeepSpeedEngine,}
Copy link

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
E231 missing whitespace after ','

# tested with `docker pull deepspeed/deepspeed:v031_torch17_cuda11`
if SETTINGS.deepspeed_required:
E2E.update(
{"ds-ddp": dl.DistributedDataParallelDeepSpeedEngine,}
Copy link

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
C819 trailing comma prohibited

)


def conv_block(in_channels, out_channels, pool=False):
Copy link

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
D103 Missing docstring in public function

return nn.Sequential(*layers)


def resnet9(in_channels: int, num_classes: int, size: int = 16):
Copy link

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
D103 Missing docstring in public function

+ " You can use download=True to download it"
)
with open(path, "rb") as infile:
data = pickle.load(infile, encoding="latin1")
Copy link

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
WPS442 Found outer scope names shadowing: data


return img, target

def __len__(self) -> int:
Copy link

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
D105 Missing docstring in magic method

return False
return True

def download(self) -> None:
Copy link

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
D102 Missing docstring in public method

return
download_and_extract_archive(self.url, self.root, filename=self.filename, md5=self.tgz_md5)

def extra_repr(self) -> str:
Copy link

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
D102 Missing docstring in public method

@@ -207,11 +219,21 @@ def all_gather(data: Any) -> List[Any]:
return data_list


def ddp_sync_run(function: Callable):
Copy link

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
D103 Missing docstring in public function

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

from catalyst.data import ToTensor


class CustomRunner(dl.IRunner):

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
D101 Missing docstring in public class

super().__init__()
self._logdir = logdir

def get_engine(self):

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
D102 Missing docstring in public method

def get_engine(self):
return dl.DistributedDataParallelEngine()

def get_loggers(self):

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
D102 Missing docstring in public method

}

@property
def stages(self):

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
D102 Missing docstring in public method

def stages(self):
return ["train_freezed", "train_unfreezed"]

def get_stage_len(self, stage: str) -> int:

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
D102 Missing docstring in public method

utils.set_requires_grad(model, True)
return model

def get_criterion(self, stage: str):

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
D102 Missing docstring in public method

def get_criterion(self, stage: str):
return nn.CrossEntropyLoss()

def get_optimizer(self, stage: str, model):

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
D102 Missing docstring in public method

else:
return optim.SGD(model.parameters(), lr=1e-1)

def get_callbacks(self, stage: str):

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
D102 Missing docstring in public method

"verbose": dl.TqdmCallback(),
}

def handle_batch(self, batch):

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
D102 Missing docstring in public method

scheduler = self.sync_device(scheduler)


return model, criterion, optimizer, scheduler

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
E303 too many blank lines (2)

def init_components(
self, model_fn=None, criterion_fn=None, optimizer_fn=None, scheduler_fn=None,
):
"""Inits the runs components."""

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
D202 No blank lines allowed after function docstring

return pipe_model, criterion, optimizer, scheduler

def deinit_components(self, runner):
"""Deinits the runs components.

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
DAR101 Missing parameter(s) in Docstring: - runner

self.targets = []

# now load the picked numpy arrays
for file_name, checksum in downloaded_list:

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
B007 Loop control variable 'checksum' not used within the loop body. If this is intended, start the name with an underscore.

+ " You can use download=True to download it"
)
with open(path, "rb") as infile:
data = pickle.load(infile, encoding="latin1")

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
WPS442 Found outer scope names shadowing: data


return img, target

def __len__(self) -> int:

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
D105 Missing docstring in magic method

return False
return True

def download(self) -> None:

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
D102 Missing docstring in public method

return
download_and_extract_archive(self.url, self.root, filename=self.filename, md5=self.tgz_md5)

def extra_repr(self) -> str:

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
D102 Missing docstring in public method


if SETTINGS.fairscale_required:
from fairscale.nn import Pipe
from fairscale.nn.data_parallel import (

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
N817 camelcase 'FullyShardedDataParallel' imported as acronym 'FSDP'

def init_components(
self, model_fn=None, criterion_fn=None, optimizer_fn=None, scheduler_fn=None,
):
"""Inits the runs components."""

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
D202 No blank lines allowed after function docstring

return pipe_model, criterion, optimizer, scheduler

def deinit_components(self, runner):
"""Deinits the runs components.

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
DAR101 Missing parameter(s) in Docstring: - runner

if SETTINGS.fairscale_required:
from fairscale.nn.data_parallel import FullyShardedDataParallel, ShardedDataParallel

parallel_wrappers = parallel_wrappers + (ShardedDataParallel, FullyShardedDataParallel,)

Choose a reason for hiding this comment

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

[pep8] reported by reviewdog 🐶
C819 trailing comma prohibited

@Scitator Scitator merged commit ed363dd into master Jun 13, 2021
@mergify mergify bot deleted the feature/engines-3 branch June 13, 2021 14:39
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.

None yet

2 participants