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

Deepcopy of state in _copy_state copies the parent sdfg #1443

Closed
luigifusco opened this issue Nov 23, 2023 · 3 comments · Fixed by #1446
Closed

Deepcopy of state in _copy_state copies the parent sdfg #1443

luigifusco opened this issue Nov 23, 2023 · 3 comments · Fixed by #1446

Comments

@luigifusco
Copy link
Contributor

luigifusco commented Nov 23, 2023

state_copy = copy.deepcopy(state)
state_copy._label += '_copy'
sdfg.add_node(state_copy)

The state is deepcopied and inserted into the sdfg. This results in an expensive copy of the state._parent sdfg, and in potential failures of the state validation (that checks that all states in an sdfg have that sdfg instance as parent).

In general, using deepcopies instead of dedicated methods sounds dangerous.

@alexnick83
Copy link
Contributor

Can you verify that it is indeed deep-copying the parent SDFG? In general, when making a clone of an object of some DaCe class, we do deep-copy, but we also describe what deep-copy should do in the class's __deepcopy__ method. It makes sense to me that deep-copying an SDFGState should not copy the parent SDFG but rather make a reference to the original parent SDFG. Therefore, the bug should be in SDFGState.__deepcopy__. See also here.

@luigifusco
Copy link
Contributor Author

luigifusco commented Nov 23, 2023

@dace.program
def double_loop(arr: dace.float32[N]):
    for i in range(N):
        arr[i] *= 2
    for i in range(N):
        arr[i] *= 2

sdfg = double_loop.to_sdfg()
from dace.transformation.helpers import find_sdfg_control_flow
from dace.sdfg.validation import validate_sdfg
find_sdfg_control_flow(sdfg)
validate_sdfg(sdfg)

is enough to trigger the error. find_sdfg_control_flow modifies in place the sdfg to perform the analysis more easily. The _parent of teh copy and the sdfg are two different instances on comparison, while the original uncopied state is valid

@tbennun
Copy link
Collaborator

tbennun commented Nov 25, 2023

The bug was indeed in SDFGState.__deepcopy__. For safety (and in the same way that we implement SDFG's deepcopy), I now set the parent to None if you copy an orphaned state in #1446. This ensures we don't make any mistakes.

tbennun added a commit that referenced this issue Nov 27, 2023
This PR fixes #1439 and #1443 by adapting fields and the deepcopy
operation for states:
1. Skips derived field `parent` being set if a state is deepcopied on
its own
2. Does not add a new field to AST nodes during preprocessing. That
parent-pointing field outlives preprocessing and ends up copying the
entire original AST for short codeblocks.
3. Does not add a new field to states during state propagation.
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 a pull request may close this issue.

3 participants