Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

ListConfig.append() does not deep copy nodes #601

Closed
odelalleau opened this issue Mar 14, 2021 · 1 comment · Fixed by #738
Closed

ListConfig.append() does not deep copy nodes #601

odelalleau opened this issue Mar 14, 2021 · 1 comment · Fixed by #738
Labels
bug Something isn't working
Milestone

Comments

@odelalleau
Copy link
Collaborator

Describe the bug

If cfg is a ListConfig, cfg.append(node) does not deep copy the node (contrary to cfg[idx] = node).
This can cause several issues, including the following that raises a max recursion error:

c = OmegaConf.create({"x": []})
c.x.append(c)
c._get_flag("test")  # (RecursionError)

To Reproduce
See above

Expected behavior
Similar behavior to what happens with assigment:

c = OmegaConf.create({"x": [None]})
c.x[0] = c
c._get_flag("test")  # None (no error)

Additional context
See related discussion in #599

@odelalleau
Copy link
Collaborator Author

Additional remarks:

Other example

Another consequence is the failure of this test with a RecursionError. It would be good to see if this error goes away after fixing this issue:

def test_resolver_with_root_and_parent(restore_resolvers: Any) -> None:
    OmegaConf.register_new_resolver(
        "both", lambda _root_, _parent_: (_root_, _parent_), use_cache=False
    )

    cfg = OmegaConf.create(
        {
            "a": 10,
            "b": {
                "c": 20,
                "both": "${both:}",
            },
            "both": "${both:}",
        }
    )
    assert cfg.both[0] is cfg and cfg.both[1] is cfg
    assert cfg.b.both[0] is cfg and cfg.b.both[1] is cfg.b

(with a deecopy it would probably still fail because cfg would be deep-copied, but it wouldn't be a RecursionError)

Performance warning

Doing a deepcopy may have performance implications since append() is called when creating a new ListConfig. Verifying the impact on performance should be part of fixing this issue.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants