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

state to runner #823

Merged
merged 6 commits into from May 28, 2020
Merged

state to runner #823

merged 6 commits into from May 28, 2020

Conversation

Scitator
Copy link
Member

@Scitator Scitator commented May 25, 2020 •
edited

Before submitting

  • 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 add your new functionality to the docs?
  • Did you update the CHANGELOG?
  • You can use 'Login as guest' to see Teamcity build logs.

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.

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

Review Jupyter notebook visual diffs & provide feedback on notebooks.

Powered by ReviewNB

@pep8speaks
Copy link

pep8speaks commented May 25, 2020 •
edited

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

Line 143:17: W503 line break before binary operator

Line 571:13: W503 line break before binary operator
Line 595:13: W503 line break before binary operator

Line 859:17: W503 line break before binary operator

Line 103:17: W503 line break before binary operator

Comment last updated at 2020-05-28 06:31:04 UTC

github-actions[bot]
github-actions bot reviewed May 25, 2020
View reviewed changes
catalyst/core/runner.py Outdated
device (Device): Torch device
"""
self._model: Model = model
self._device: Device = device
self._prepare_inner_state(device=device, model=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 🐶
E241 multiple spaces after ','

@Scitator Scitator force-pushed the core/state-to-runner-merge branch 2 times, most recently from 85a903b to 9f945d7 Compare May 27, 2020 06:24
@Scitator Scitator force-pushed the core/state-to-runner-merge branch from 9f945d7 to b7fc5b2 Compare May 27, 2020 06:59
github-actions[bot]
github-actions bot reviewed May 27, 2020
View reviewed changes
catalyst/core/runner.py Outdated


Runner section

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 🐶
W293 blank line contains whitespace

catalyst/core/runner.py Outdated


Experiment section

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 🐶
W293 blank line contains whitespace

catalyst/core/runner.py Outdated


Dataflow section

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 🐶
W293 blank line contains whitespace

catalyst/core/runner.py Outdated


Metrics section

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 🐶
W293 blank line contains whitespace

catalyst/core/runner.py Outdated


Validation metrics section

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 🐶
W293 blank line contains whitespace

catalyst/core/runner.py Outdated


Epoch info section

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 🐶
W293 blank line contains whitespace

catalyst/core/runner.py Outdated


Loader info section

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 🐶
W293 blank line contains whitespace

catalyst/core/runner.py Outdated


Batch info section

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 🐶
W293 blank line contains whitespace

catalyst/core/runner.py Outdated
**runner.batch_size** - int, length of the current batch

Logging section

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 🐶
W293 blank line contains whitespace

catalyst/core/runner.py Outdated
with all extra data for experiment tracking

Extra section

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 🐶
W293 blank line contains whitespace

@Scitator Scitator force-pushed the core/state-to-runner-merge branch from 73f80a6 to 1002d13 Compare May 27, 2020 18:06
@Scitator
Copy link
Member Author

Scitator commented May 27, 2020 •
edited

todo:

  • state_kwargs -> stage_kwargs
  • freeze runner during expriment
  • check tutorials

github-actions[bot]
github-actions bot reviewed May 27, 2020
View reviewed changes
examples/cifar_stages/experiment.py
@@ -31,16 +31,16 @@ def __getitem__(self, index: int):
class Experiment(ConfigExperiment):
"""``ConfigExperiment`` with CIFAR10 dataset."""

def _postprocess_model_for_stage(self, stage: str, model: nn.Module):
model_ = model
def get_model(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

catalyst/dl/experiment/config.py Outdated
self._config["stages"]["state_params"] = utils.merge_dicts(
deepcopy(self._config["stages"].get("state_params", {})),
self._config["stages"]["stage_params"] = utils.merge_dicts(
deepcopy(self._config["stages"].get("state_params", {})), # saved for backward compatibility
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 🐶
E501 line too long (105 > 79 characters)

@Scitator Scitator force-pushed the core/state-to-runner-merge branch from 0e5770e to e6b3e05 Compare May 27, 2020 19:10
@Scitator Scitator force-pushed the core/state-to-runner-merge branch from e6b3e05 to 01deb15 Compare May 27, 2020 19:16
@Scitator Scitator changed the base branch from master to develop May 28, 2020 06:59
@Scitator Scitator merged commit 78667e9 into develop May 28, 2020
@Scitator Scitator deleted the core/state-to-runner-merge branch May 31, 2020 13:22
Scitator added a commit that referenced this pull request Jun 4, 2020
@Scitator
Release 20.06 (#824)
0c08738 
* state to runner (#823)

* state to runner

* import & codestyle fix

* unfreeze

* remaning & docs

* config and freeze

* last renaming

* froze update

* merged with master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions github-actions[bot] github-actions left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@Scitator @pep8speaks