fix the overwrite_opts and handle the new_allowed properly

Summary:
D33301363 changes the signature of `update_cfg` from `update_cfg(cfg, *updaters)` to `update_cfg(cfg, updaters, new_allowed)`, while the call sites are not updated. Eg. https://www.internalfb.com/code/fbsource/[9e071979a62ba7fd3d7a71dee1f0809815cbaa43]/fbcode/fblearner/flow/projects/mobile_vision/detectron2go/core/workflow.py?lines=221-225, the `merge_from_list_updater(e2e_train.overwrite_opts),` is then not used.

For the fix:
- Since there're a lot of call sites for `update_cfg` it's better to keep the original signature.
- ~~~The `new_allowed` can actually be passed to each individual updater instead of the `update_cfg`, this also gives finer control.~~~
- Make override the `merge_from_list` to make it respect `new_allowed`.
- Preserve the `new_allowed` for all nodes (not only the root) in the FLOW Future calls.

Reviewed By: zhanghang1989

Differential Revision: D34840001

fbshipit-source-id: 14aff6bec75a8b53d4109e6cd73d2494f68863b4

d2go/config/config.py

@@ -18,6 +18,21 @@ logger = logging.getLogger(__name__)
 CONFIG_CUSTOM_PARSE_REGISTRY = Registry("CONFIG_CUSTOM_PARSE")
 
 
+def _opts_to_dict(opts: List[str]):
+    ret = {}
+    if opts is None:
+        return ret
+    for full_key, v in zip(opts[0::2], opts[1::2]):
+        keys = full_key.split(".")
+        cur = ret
+        for key in keys[:-1]:
+            if key not in cur:
+                cur[key] = {}
+            cur = cur[key]
+        cur[keys[-1]] = v
+    return ret
+
+
 class CfgNode(_CfgNode):
     @classmethod
     def cast_from_other_class(cls, other_cfg):
@@ -36,7 +51,10 @@ class CfgNode(_CfgNode):
             return res
 
     def merge_from_list(self, cfg_list: List[str]):
-        res = super().merge_from_list(cfg_list)
+        # NOTE: YACS's orignal merge_from_list could not handle non-existed keys even if
+        # new_allow is set, override the method for support this.
+        override_cfg = _opts_to_dict(cfg_list)
+        res = super().merge_from_other_cfg(CfgNode(override_cfg))
         self._run_custom_processing(is_dump=False)
         return res
 

tests/misc/test_config.py

@@ -7,6 +7,7 @@ import logging
 import os
 import unittest
 
+from d2go.config import CfgNode
 from d2go.config import auto_scale_world_size, reroute_config_path
 from d2go.config.utils import (
     config_dict_to_list_str,
@@ -92,6 +93,25 @@ class TestConfig(unittest.TestCase):
             ["QUANTIZATION.QAT.BACKEND", "fbgemm"],
         )
 
+    def test_merge_from_list_with_new_allowed(self):
+        """
+        YACS's merge_from_list doesn't take new_allowed into account, D2Go override its behavior, and this test covers it.
+        """
+        # new_allowed is not set
+        cfg = CfgNode()
+        cfg.A = CfgNode()
+        cfg.A.X = 1
+        self.assertRaises(Exception, cfg.merge_from_list, ["A.Y", "2"])
+
+        # new_allowed is set for sub key
+        cfg = CfgNode()
+        cfg.A = CfgNode(new_allowed=True)
+        cfg.A.X = 1
+        cfg.merge_from_list(["A.Y", "2"])
+        self.assertEqual(cfg.A.Y, 2)  # note that the string will be converted to number
+        # however new_allowed is not set for root key
+        self.assertRaises(Exception, cfg.merge_from_list, ["B", "3"])
+
 
 class TestConfigUtils(unittest.TestCase):
     """Test util functions in config/utils.py"""