key_rewrites in RewrittenYaml does not work

[ErikOrjehag]

key_rewrites for RewrittenYaml doesn't work in galactic with python 3.8.5.

I get this error:

  File "/opt/ros/galactic/lib/python3.8/site-packages/nav2_common/launch/rewritten_yaml.py", line 90, in perform
    self.substitute_keys(data, keys_rewrites)
  File "/opt/ros/galactic/lib/python3.8/site-packages/nav2_common/launch/rewritten_yaml.py", line 137, in substitute_keys
    for key, val in yaml.items():
RuntimeError: dictionary keys changed during iteration

Here is the function:
https://github.com/ros-planning/navigation2/blob/2de3f92c0f476de4bda21d1fc5268657b499b258/nav2_common/nav2_common/launch/rewritten_yaml.py#L135-L142

Another thing I noticed while looking at this is that we should recuse even if key is not in key_rewrites? And also substitute keys if the value is not a dict? Maybe this could work better:

    def substitute_keys(self, yaml, key_rewrites):
        if len(key_rewrites) != 0:
            for key in list(yaml.keys()):
                val = yaml[key]
                if key in key_rewrites:
                    new_key = key_rewrites[key]
                    yaml[new_key] = yaml[key]
                    del yaml[key]
                if isinstance(val, dict):
                    self.substitute_keys(val, key_rewrites)

[SteveMacenski]

I did some digging and found that this feature was never actually used but was added with the speculative idea that it might be helpful - so I'm not entirely surprised it wasn't updated with other changes.

If you're open to fixing the issue, I'd love to review a PR to get that back up to work! Alternatively, if you don't need it and since we don't use it, another option could be to remove that feature entirely so we don't have something broken and unused hanging around.

substitute keys if the value is not a dict

That does seem weird to me as well at first glance. Is it the case that this is always a dict practically speaking (e.g. when at leaf its also a dict of length 1? I'm actually not sure how it would work to replace a key at all as written otherwise) and this is just error checking the inputs rather than a functional check?

we should recuse even if key is not in key_rewrites

I don't quite understand this 😦

[ErikOrjehag]

This was a super useful feature for me so I would vote to not remove it, I copied RewrittenYaml and patched it to solve the issue in my project. I can make a PR and explain the solution a bit more in depth and if possible have unit tests for all the edge cases :)

we should recuse even if key is not in key_rewrites

I don't quite understand this

If the key is not in key_rewrites a child in the dictionary of that key can still be in key_rewrites deeper down in the structure.

substitute keys if the value is not a dict

That does seem weird to me as well at first glance.

Imagine this case:

a:
  b:
    c: True

with key_rewrites:

{
  "c": "d"
}

Then I think it makes sense to recurse all the way down to "c" and then replace c: True with d: True even though it is a leaf and its value is not a dict.

---

My use case for this feature is that I have two controller_managers with different names running at the same time but I want to reuse the exact same yaml config for both of them and therefore have to rename the root key so that the key matches the node names.

[SteveMacenski]

I can make a PR and explain the solution a bit more in depth

Sounds great, I look forward to it!

I'd have to refresh myself with the Rewritten Yaml code more, but the value rewrites I believe you need to specify the full path e.g. my_server.ros__params.a.b.c rather than just the leaf in case there are cs in multiple places meaning different things for different nodes. I believe that's the case b/c that's what I had to do with the system tests to rewrite some information. I don't have an issue with an option to override leaves to impact all, but I think that should not be the default behavior since otherwise you could override many things called that same name all over the file.

[SteveMacenski]

@ErikOrjehag any update?

[SteveMacenski]

@ErikOrjehag looking for an update after about ~3 months. It sounds like from your message you patched it already and just waiting on a PR to reintegrate the changes into Nav2!

[ErikOrjehag]

@SteveMacenski yes patched it locally. Having difficulty finding time to contribute atm... :/

[SteveMacenski]

@ErikOrjehag I’m happy to finish up whatever’s missing if you can push the branch somewhere I can start off of (or post code here)

i’m trying to get some of these small bugs / features resolved

[ErikOrjehag]

@SteveMacenski Something like this:

import typing as tp

YamlType = tp.TypeVar("YamlType", list, dict, str, bool, int, float)


def substitute_keys(yaml: YamlType, key_rewrites: tp.Dict[str, str]) -> YamlType:
    """Returns a new yaml where the keys are substituted."""

    if isinstance(yaml, dict):
        return {
            key_rewrites[key] if key in key_rewrites.keys() else key:
                substitute_keys(val, {
                    k[k.index(".")+1:]: v
                    for k, v in key_rewrites.items()
                    if "." in k and k[:k.index(".")] == key
                })
            for key, val in yaml.items()
        }

    if isinstance(yaml, list):
        return [
            substitute_keys(val, key_rewrites)
            for val in yaml
        ]

    return yaml

Tested with this:

if __name__ == "__main__":

    key_rewrites = {
        # substitute these
        "a": "a1",
        "a.c": "c1",
        "b": "b1",
        "b.c": "c2",
        "c": "c3",
        "c.d": "d1",
        "c.f": "f1",
        "d": "d1",
        "d.c": "c4"
        # and keep these unchanged
        # c.e
        # d.c.c
    }

    source = {
        "a": {
            "c": 1
        },
        "b": {
            "c": 2
        },
        "c": [
            {
                "d": 3,
                "e": False
            },
            {
                "f": "foo"
            }
        ],
        "d": {
            "c": {
                "c": True
            }
        }
    }

    result = substitute_keys(source, key_rewrites)

    import json
    print(json.dumps(result, indent=2))

Gives this output:

{
  "a1": {
    "c1": 1
  },
  "b1": {
    "c2": 2
  },
  "c3": [
    {
      "d1": 3,
      "e": false
    },
    {
      "f1": "foo"
    }
  ],
  "d1": {
    "c4": {
      "c": true
    }
  }
}

[SteveMacenski]

Great! Is there anything particularly wrong with it that I need to finish up or is it just putting it together into a PR and writing a few more tests?

I'll put this in my queue for next week.

[ErikOrjehag]

This one works a bit differently than the old one in that it returns a new dictionary instead of modifying the input argument. Other than that I don't think it's anything special :)

[SteveMacenski]

#3730 resolves