(torchx/runner) standardize class and param naming around runopts and runcfg (#252)

Summary:
Pull Request resolved: #252

closes: #250

I've done a few things on this diff:

1. Renamed `torchx.specs.api.Runopt` to `torchx.specs.runopt` (for consistency with `runopts`)
2. Renamed variables (where I could) `runcfg` to `cfg` (to be consistent with the scheduler and runner apis)
3. Renamed the config section to `[$profile.$sched.cfg]`  instead of `[$profile.scheduler_args.$sched]`
4. Changed the torchx run cli's `-a` (short for `--scheduler_args`) to `-cfg` for consistency with the rest of the system.

Reviewed By: aivanou

Differential Revision: D31656766

fbshipit-source-id: 8c009852d5807010ac4cd33902b294cff4bd0ec1

Author: Kiuk Chung

torchx/cli/cmd_run.py

@@ -10,18 +10,18 @@
 import sys
 from dataclasses import asdict
 from pprint import pformat
-from typing import Dict, List, cast, Type, Optional
+from typing import Dict, List, Optional, Type, cast
 
 import torchx.specs as specs
 from pyre_extensions import none_throws
 from torchx.cli.cmd_base import SubCommand
 from torchx.runner import Runner, get_runner
-from torchx.schedulers import get_scheduler_factories, get_default_scheduler_name
+from torchx.schedulers import get_default_scheduler_name, get_scheduler_factories
 from torchx.specs.finder import (
+    ComponentNotFoundException,
+    ComponentValidationException,
     _Component,
     get_components,
-    ComponentValidationException,
-    ComponentNotFoundException,
 )
 from torchx.util.types import to_dict
 
@@ -41,13 +41,13 @@ def _convert_to_option_type(
         return option_type(value)
 
 
-def _parse_run_config(arg: str, scheduler_run_opts: specs.runopts) -> specs.RunConfig:
+def _parse_run_config(arg: str, scheduler_opts: specs.runopts) -> specs.RunConfig:
     conf = specs.RunConfig()
     if not arg:
         return conf
 
     for key, value in to_dict(arg).items():
-        option = scheduler_run_opts.get(key)
+        option = scheduler_opts.get(key)
         if option is None:
             raise ValueError(f"Unknown {key}, run `torchx runopts` for more info")
         option_type = option.opt_type
@@ -86,7 +86,7 @@ def add_arguments(self, subparser: argparse.ArgumentParser) -> None:
             default=get_default_scheduler_name(),
         )
         subparser.add_argument(
-            "-a",
+            "-cfg",
             "--scheduler_args",
             type=str,
             help="Arguments to pass to the scheduler (Ex:`cluster=foo,user=bar`)."

torchx/runner/api.py

@@ -263,7 +263,7 @@ def dryrun(
 
         cfg = cfg or RunConfig()
         # TODO enable profiles - https://github.com/pytorch/torchx/issues/248
-        config.apply(profile="default", scheduler=scheduler, runcfg=cfg)
+        config.apply(scheduler=scheduler, cfg=cfg, profile="default")
 
         sched = self._scheduler(scheduler)
         sched._validate(app, scheduler)

torchx/runner/config.py

@@ -12,6 +12,7 @@
 
 from torchx.schedulers import Scheduler, get_schedulers
 from torchx.specs import RunConfig, get_type_name
+from torchx.specs.api import runopt
 
 
 _NONE = "None"
@@ -47,22 +48,36 @@ def _get_scheduler(name: str) -> Scheduler:
     return sched
 
 
+def _fixme_placeholder(runopt: runopt, max_len: int = 60) -> str:
+    ph = f"#FIXME:({get_type_name(runopt.opt_type)}) {runopt.help}"
+    return ph if len(ph) <= max_len else f"{ph[:max_len]}..."
+
+
 def dump(
     f: TextIO, schedulers: Optional[List[str]] = None, required_only: bool = False
 ) -> None:
     """
-    Dumps a default INI-style config template containing the runopts for the
-    given scheduler names into ``f``. If no ``schedulers`` are specified
-    dumps all known registered schedulers.
+    Dumps a default INI-style config template containing the :py:class:torchx.specs.runopts for the
+    given scheduler names into the file-like object specified by ``f``.
+    If no ``schedulers`` are specified dumps all known registered schedulers.
 
     Optional runopts are pre-filled  with their default values.
-    Required runopts are set with a ``<FIXME_...>`` placeholder.
+    Required runopts are set with a ``FIXME: ...`` placeholder.
+    To only dump required runopts pass ``required_only=True``.
+
     Each scheduler's runopts are written in the section called
-    ``[default.scheduler_args.{scheduler_name}]`` (e.g. ``[default.scheduler_args.kubernetes]``)
+    ``[default.{scheduler_name}.cfg]``.
 
-    To only dump required runopts pass ``required_only=True``.
+    For example:
+
+    ::
 
-    Raises a ``ValueError`` if given a scheduler name that is not known
+     [default.kubernetes.cfg]
+     namespace = default
+     queue = #FIXME (str)Volcano queue to schedule job in
+
+    Raises:
+        ``ValueError`` - if given a scheduler name that is not known
     """
 
     if schedulers:
@@ -74,12 +89,12 @@ def dump(
     for sched_name in scheds:
         sched = _get_scheduler(sched_name)
 
-        section = f"default.scheduler_args.{sched_name}"
+        section = f"default.{sched_name}.cfg"
         config.add_section(section)
 
         for opt_name, opt in sched.run_opts():
             if opt.is_required:
-                val = f"<FIXME_WITH_A_{get_type_name(opt.opt_type)}_VALUE>"
+                val = _fixme_placeholder(opt)
             else:  # not required runopts MUST have a default
                 if required_only:
                     continue
@@ -96,11 +111,10 @@ def dump(
                     val = f"{opt.default}"
 
             config.set(section, opt_name, val)
-
     config.write(f, space_around_delimiters=True)
 
 
-def apply(profile: str, scheduler: str, runcfg: RunConfig) -> None:
+def apply(scheduler: str, cfg: RunConfig, profile: str = "default") -> None:
     """
     Loads .torchxconfig files from predefined locations according
     to a load hierarchy and applies the loaded configs into the
@@ -121,10 +135,10 @@ def apply(profile: str, scheduler: str, runcfg: RunConfig) -> None:
         if configfile.exists():
             log.info(f"loading configs from {configfile}")
             with open(str(configfile), "r") as f:
-                load(profile, scheduler, f, runcfg)
+                load(scheduler, f, cfg, profile)
 
 
-def load(profile: str, scheduler: str, f: TextIO, runcfg: RunConfig) -> None:
+def load(scheduler: str, f: TextIO, cfg: RunConfig, profile: str = "default") -> None:
     """
     loads the section ``[{profile}.scheduler_args.{scheduler}]`` from the given
     configfile ``f`` (in .INI format) into the provided ``runcfg``, only adding
@@ -137,17 +151,17 @@ def load(profile: str, scheduler: str, f: TextIO, runcfg: RunConfig) -> None:
 
     runopts = _get_scheduler(scheduler).run_opts()
 
-    section = f"{profile}.scheduler_args.{scheduler}"
+    section = f"{profile}.{scheduler}.cfg"
     if config.has_section(section):
         for name, value in config.items(section):
-            if name in runcfg.cfgs:
+            if name in cfg.cfgs:
                 # DO NOT OVERRIDE existing configs
                 continue
 
             if value == _NONE:
                 # should map to None (not str 'None')
                 # this also handles empty or None lists
-                runcfg.set(name, None)
+                cfg.set(name, None)
             else:
                 runopt = runopts.get(name)
 
@@ -161,9 +175,9 @@ def load(profile: str, scheduler: str, f: TextIO, runcfg: RunConfig) -> None:
                     if runopt.opt_type is bool:
                         # need to handle bool specially since str -> bool is based on
                         # str emptiness not value (e.g. bool("False") == True)
-                        runcfg.set(name, config.getboolean(section, name))
+                        cfg.set(name, config.getboolean(section, name))
                     elif runopt.opt_type is List[str]:
-                        runcfg.set(name, value.split(";"))
+                        cfg.set(name, value.split(";"))
                     else:
                         # pyre-ignore[29]
-                        runcfg.set(name, runopt.opt_type(value))
+                        cfg.set(name, runopt.opt_type(value))

torchx/runner/test/config_test.py

@@ -102,30 +102,30 @@ def run_opts(self) -> runopts:
         return opts
 
 
-_CONFIG = """[default.scheduler_args.local_cwd]
+_CONFIG = """[default.local_cwd.cfg]
 log_dir = /home/bob/logs
 prepend_cwd = True
 
-[test.scheduler_args.local_cwd]
+[test.local_cwd.cfg]
 log_dir = None
 prepend_cwd = False
 
-[alpha.scheduler_args.local_cwd]
+[alpha.local_cwd.cfg]
 log_dir = /tmp/logs
 """
 
-_CONFIG_INVALID = """[default.scheduler_args.test]
+_CONFIG_INVALID = """[default.test.cfg]
 a_run_opt_that = does_not_exist
 s = option_that_exists
 """
 
-_TEAM_CONFIG = """[default.scheduler_args.test]
+_TEAM_CONFIG = """[default.test.cfg]
 s = team_default
 i = 50
 f = 1.2
 """
 
-_MY_CONFIG = """[default.scheduler_args.test]
+_MY_CONFIG = """[default.test.cfg]
 s = my_default
 i = 100
 """
@@ -158,32 +158,30 @@ def _write(self, filename: str, content: str) -> Path:
         return f
 
     def test_load(self) -> None:
-        runcfg = RunConfig()
-        load(
-            profile="default", scheduler="local_cwd", f=StringIO(_CONFIG), runcfg=runcfg
-        )
-        self.assertEqual("/home/bob/logs", runcfg.get("log_dir"))
-        self.assertEqual(True, runcfg.get("prepend_cwd"))
+        cfg = RunConfig()
+        load(profile="default", scheduler="local_cwd", f=StringIO(_CONFIG), cfg=cfg)
+        self.assertEqual("/home/bob/logs", cfg.get("log_dir"))
+        self.assertEqual(True, cfg.get("prepend_cwd"))
 
-        runcfg = RunConfig()
-        load(profile="test", scheduler="local_cwd", f=StringIO(_CONFIG), runcfg=runcfg)
-        self.assertEqual(None, runcfg.get("log_dir"))
-        self.assertEqual(False, runcfg.get("prepend_cwd"))
+        cfg = RunConfig()
+        load(profile="test", scheduler="local_cwd", f=StringIO(_CONFIG), cfg=cfg)
+        self.assertEqual(None, cfg.get("log_dir"))
+        self.assertEqual(False, cfg.get("prepend_cwd"))
 
-        runcfg = RunConfig()
-        load(profile="alpha", scheduler="local_cwd", f=StringIO(_CONFIG), runcfg=runcfg)
-        self.assertEqual("/tmp/logs", runcfg.get("log_dir"))
-        self.assertEqual(None, runcfg.get("prepend_cwd"))
+        cfg = RunConfig()
+        load(profile="alpha", scheduler="local_cwd", f=StringIO(_CONFIG), cfg=cfg)
+        self.assertEqual("/tmp/logs", cfg.get("log_dir"))
+        self.assertEqual(None, cfg.get("prepend_cwd"))
 
     def test_no_override_load(self) -> None:
-        runcfg = RunConfig()
-        runcfg.set("log_dir", "/foo/bar")
-        runcfg.set("debug", 1)
+        cfg = RunConfig()
+        cfg.set("log_dir", "/foo/bar")
+        cfg.set("debug", 1)
 
-        load(profile="test", scheduler="local_cwd", f=StringIO(_CONFIG), runcfg=runcfg)
-        self.assertEqual("/foo/bar", runcfg.get("log_dir"))
-        self.assertEqual(1, runcfg.get("debug"))
-        self.assertEqual(False, runcfg.get("prepend_cwd"))
+        load(profile="test", scheduler="local_cwd", f=StringIO(_CONFIG), cfg=cfg)
+        self.assertEqual("/foo/bar", cfg.get("log_dir"))
+        self.assertEqual(1, cfg.get("debug"))
+        self.assertEqual(False, cfg.get("prepend_cwd"))
 
     @patch(
         TORCHX_GET_SCHEDULERS,
@@ -192,14 +190,14 @@ def test_no_override_load(self) -> None:
     def test_apply(self, _) -> None:
         with patch(PATH_CWD, return_value=Path(self.test_dir)):
             with patch(PATH_HOME, return_value=Path(self.test_dir) / "home" / "bob"):
-                runcfg = RunConfig()
-                runcfg.set("s", "runtime_value")
+                cfg = RunConfig()
+                cfg.set("s", "runtime_value")
 
-                apply(profile="default", scheduler="test", runcfg=runcfg)
+                apply(profile="default", scheduler="test", cfg=cfg)
 
-                self.assertEqual("runtime_value", runcfg.get("s"))
-                self.assertEqual(100, runcfg.get("i"))
-                self.assertEqual(1.2, runcfg.get("f"))
+                self.assertEqual("runtime_value", cfg.get("s"))
+                self.assertEqual(100, cfg.get("i"))
+                self.assertEqual(1.2, cfg.get("f"))
 
     def test_dump_invalid_scheduler(self) -> None:
         with self.assertRaises(ValueError):
@@ -215,50 +213,50 @@ def test_dump_only_required(self, _) -> None:
         # test scheduler has no required options hence expect empty string
         dump(f=sfile, required_only=True)
 
-        runcfg = RunConfig()
+        cfg = RunConfig()
         sfile.seek(0)
-        load(profile="default", scheduler="test", f=sfile, runcfg=runcfg)
+        load(profile="default", scheduler="test", f=sfile, cfg=cfg)
 
-        self.assertFalse(runcfg.cfgs)
+        self.assertFalse(cfg.cfgs)
 
     @patch(
         TORCHX_GET_SCHEDULERS,
         return_value={"test": TestScheduler()},
     )
     def test_load_invalid_runopt(self, _) -> None:
-        runcfg = RunConfig()
+        cfg = RunConfig()
         load(
             profile="default",
             scheduler="test",
             f=StringIO(_CONFIG_INVALID),
-            runcfg=runcfg,
+            cfg=cfg,
         )
         # options in the config file but not in runopts
         # should be ignored (we shouldn't throw an error since
         # this makes things super hard to guarantee BC - stale config file will fail
         # to run, we don't want that)
 
-        self.assertEquals("option_that_exists", runcfg.get("s"))
+        self.assertEquals("option_that_exists", cfg.get("s"))
 
     def test_load_no_section(self) -> None:
-        runcfg = RunConfig()
+        cfg = RunConfig()
         load(
             profile="default",
             scheduler="local_cwd",
             f=StringIO(),
-            runcfg=runcfg,
+            cfg=cfg,
         )
         # is empty
-        self.assertFalse(runcfg.cfgs)
+        self.assertFalse(cfg.cfgs)
 
         load(
             profile="default",
             scheduler="local_cwd",
             f=StringIO("[default.scheduler_args.local_cwd]\n"),
-            runcfg=runcfg,
+            cfg=cfg,
         )
         # still empty
-        self.assertFalse(runcfg.cfgs)
+        self.assertFalse(cfg.cfgs)
 
     @patch(
         TORCHX_GET_SCHEDULERS,
@@ -270,12 +268,12 @@ def test_dump_and_load_all_runopt_types(self, _) -> None:
 
         sfile.seek(0)
 
-        runcfg = RunConfig()
-        load(profile="default", scheduler="test", f=sfile, runcfg=runcfg)
+        cfg = RunConfig()
+        load(profile="default", scheduler="test", f=sfile, cfg=cfg)
 
         # all runopts in the TestScheduler have defaults, just check against those
         for opt_name, opt in TestScheduler().run_opts():
-            self.assertEqual(runcfg.get(opt_name), opt.default)
+            self.assertEqual(cfg.get(opt_name), opt.default)
 
     def test_dump_and_load_all_registered_schedulers(self) -> None:
         # dump all the runopts for all registered schedulers
@@ -284,11 +282,11 @@ def test_dump_and_load_all_registered_schedulers(self) -> None:
 
         sfile = StringIO()
         dump(sfile)
-
+        print(sfile.getvalue())
         for sched_name, sched in get_schedulers(session_name="_").items():
             sfile.seek(0)  # reset the file pos
-            runcfg = RunConfig()
-            load(profile="default", scheduler=sched_name, f=sfile, runcfg=runcfg)
+            cfg = RunConfig()
+            load(profile="default", scheduler=sched_name, f=sfile, cfg=cfg)
 
             for opt_name, _ in sched.run_opts():
-                self.assertTrue(opt_name in runcfg.cfgs)
+                self.assertTrue(opt_name in cfg.cfgs)