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

HydraConfig support #1022

Merged
merged 14 commits into from Dec 20, 2020
Merged

HydraConfig support #1022

merged 14 commits into from Dec 20, 2020

Conversation

otherman16
Copy link
Contributor

Hydra support

  1. MNIST classification test pipeline was added
  2. catalyst/experiments/hydra_config.py: HydraConfigExperiment was added
  3. catalyst/runners/hydra_supervised.py: HydraSupervisedExperiment was added
  4. catalyst/dl/script/hydra_run.py: @hydra.main() was added as entry point for hydra configuration experiment
  5. catalyst/dl/script/run.py: optional argument '--hydra' was added to change entrypoint to hydra config experiment one
  6. catalyst/settings.py: optional 'hydra' package import was added
  7. catalyst/utils/hydra_config.py: hydra config utils was added
  8. catalyst/utils/hydra_scripts.py: hydra scripts utils was added
  9. catalyst/utils/hydra_sys.py: hydra sys utils was added
  10. requirements/requirements-hydra.txt: optional 'hydra-core' package was added

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 check that your code passes the unit tests pytest . ?
  • Did you add your new functionality to the docs?
  • Did you update the CHANGELOG?

Description

Hydra support for catalyst-dl run

Previous PR: OmegaConf support for "catalyst-dl run" #943

Related Issue

Hydra/OmegaConf configuration structure #912

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.

PS

  • I know, that I could join slack for pull request discussion.

…IST classification test pipeline was added

2. catalyst/experiments/hydra_config.py: HydraConfigExperiment was added
3. catalyst/runners/hydra_supervised.py: HydraSupervisedExperiment was added
4. catalyst/dl/script/hydra_run.py: @hydra.main() was added as entry point for hydra configuration experiment
5. catalyst/dl/script/run.py: optional argument '--hydra' was added to change entrypoint to hydra config experiment one
6. catalyst/settings.py: optional 'hydra' package import was added
7. catalyst/utils/hydra_config.py: hydra config utils was added
8. catalyst/utils/hydra_scripts.py: hydra scripts utils was added
9. catalyst/utils/hydra_sys.py: hydra sys utils was added
10. requirements/requirements-hydra.txt: optional 'hydra-core' package was added
@otherman16
Copy link
Contributor Author

I don't know what's wrong with this Error

Traceback (most recent call last):
  File "catalyst/dl/scripts/run.py", line 9, in <module>
    from catalyst.settings import IS_HYDRA_AVAILABLE
  File "/home/runner/work/catalyst/catalyst/catalyst/settings.py", line 1, in <module>
    from typing import Any, Dict, List, Optional, Tuple
  File "/home/runner/work/catalyst/catalyst/catalyst/typing.py", line 4, in <module>
    from typing import Dict, Union
ImportError: cannot import name 'Dict' from partially initialized module 'typing' (most likely due to a circular import) (/home/runner/work/catalyst/catalyst/catalyst/typing.py)

Think I need some review help guys.

@Scitator Scitator changed the title 1. tests/_tests_cv_classification_hydra: 'catalyst-dl run --hydra' MN… HydraConfig support Dec 10, 2020
@Scitator
Copy link
Member

Dear @otherman16 ,
should we close #943 then?

bin/tests/check_dl_cv.sh Outdated Show resolved Hide resolved
Copy link
Member

@Scitator Scitator left a comment

Choose a reason for hiding this comment

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

thrilled by this PR, amazing work!

catalyst/experiments/hydra_config.py Show resolved Hide resolved
catalyst/runners/hydra_supervised.py Outdated Show resolved Hide resolved
@otherman16
Copy link
Contributor Author

Yes we can close #943

Comment on lines +22 to +29
cfg.setdefault("args", DictConfig({}))
cfg.args.setdefault("expdir", ".")
cfg.args.setdefault("resume", None)
cfg.args.setdefault("autoresume", None)
cfg.args.setdefault("seed", 42)
cfg.args.setdefault("distributed", os.getenv("USE_DDP", "0") == "1")
cfg.args.setdefault("apex", os.getenv("USE_APEX", "0") == "1")
cfg.args.setdefault("amp", os.getenv("USE_AMP", "0") == "1")
Copy link

Choose a reason for hiding this comment

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

Thanks for working on this!
While I understand you have backward compatibility requirements that might be in conflict with the suggestions here, I wanted to provide them anyway (at least as food for thought) :
I'd recommend utilizing config composition to get this instead of manually editing the config.
for example, the defaults list of the primary config can look like:

defaults:
  - catalyst/config  # mandatory catalyst config (provided by catalyst)
  - foo: bar # user config groups

catalyst/config can contain everything needed by catalyst (what you are adding above).

A few extra points:

  1. I am guessing this is designed to be backward compatible with the standard command line.
    If possible, break away from that and utilize hierarchy to give catalyst a space of it's own where it wouldn't conflict with other frameworks/user config.
    for example:
    catalst/config.yaml:
catalyst:
  resume: null
  seed: 42
  ...
  1. Consider using Structured Configs to get config type safety, the above yaml file can be replaced with something like:
@dataclass
class CatalystConf:
  resume: Optional[bool] = None
  seed: int = 42

ConfigStore.instance().store(group="catalst", name="config", node=CatalystConf)

(See the Structured Configs tutorial for more info).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you. Thinking about it.

catalyst/experiments/hydra_config.py Outdated Show resolved Hide resolved
catalyst/experiments/hydra_config.py Outdated Show resolved Hide resolved
catalyst/experiments/hydra_config.py Show resolved Hide resolved
catalyst/experiments/hydra_config.py Show resolved Hide resolved
catalyst/experiments/hydra_config.py Show resolved Hide resolved
catalyst/runners/hydra_supervised.py Outdated Show resolved Hide resolved
catalyst/runners/__init__.py Outdated Show resolved Hide resolved
catalyst/utils/hydra_scripts.py Outdated Show resolved Hide resolved
catalyst/utils/hydra_sys.py Outdated Show resolved Hide resolved
Hydra cv classification test was fixed
All dublicated code was replaced to catalyst/experiments/functional.py
Hydra cv classification test was fixed
…and changed catalyst/utils/sys.py:

Functions 'catalyst/utils/scripts.py:distributed_cmd_run()' and 'catalyst/utils/sys.py:dump_environment()' are used for both HydraConfigExperiment and ConfigExperiment
…i_supervised.py

MultiSupervisedRunner was added for multiple model support
@otherman16
Copy link
Contributor Author

I do not know what is wrong with import catalyst.experiments.functional as F in catalyst/experiments/config.py in VMs with python3.6 inside.
Can you help me please?

@Scitator
Copy link
Member

I do not know what is wrong with import catalyst.experiments.functional as F in catalyst/experiments/config.py in VMs with python3.6 inside.
Can you help me please?

could you try something like import catalyst.experiments.functional import my_required_function?

@Scitator Scitator merged commit 6d453a2 into catalyst-team:master Dec 20, 2020
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

3 participants