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

Issue with tracking calls with referenced arguments #15

Open
puppis opened this issue Oct 26, 2023 · 1 comment
Open

Issue with tracking calls with referenced arguments #15

puppis opened this issue Oct 26, 2023 · 1 comment

Comments

@puppis
Copy link

puppis commented Oct 26, 2023

I came across this library and I must say I am really happy to have it.
However, there is a critical issue that blocks basically every real-case use of this. This is related to the fact that arguments of calls are tracked by reference, and so if some arguments are changed during or after the call (side-effect), then the rendering visualizes the updated values and not the original values used when the call was made. The problem is that one cannot implement a workaround in the tracked procedure without removing the side-effects on the arguments.

Example:

suppose there is a quicksort procedure that does not relies on comprehension, e.g.:

def quicksort(a, i, j):
if i < j - 1:
k = partition(a, i, j)
quicksort(a, i, k)
quicksort(a, k + 1, j)

When this procedure is tracked, e.g. with decorator @rcviz.viz, the rendered tree shows the same content for the array argument a, since this is stored as a reference.

I think a possible fix to this is to edit the init method in the file node_data.py, replacing

    self.args = _args
    self.kwargs = _kwargs
    self.fn_name = _fnname
    self.ret = _ret

with

    self.args = copy.deepcopy(_args)
    self.kwargs = copy.deepcopy(_kwargs)
    self.fn_name = _fnname
    self.ret = copy.deepcopy(_ret)

Of course one needs to import the python module copy
BTW. it seems something similar is already done for auxdata.update when adding user assigned track data.

PS. I have created a pull request with the suggested fix.

@carlsborg
Copy link
Owner

Thanks for the issue and the PR. I can see this being a problem if each invocation does not create the variable on the stack, and instead mutates it in place and passes it down. Please could you add a file in examples/ showing the failing case? Otherwise LGTM.

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

No branches or pull requests

2 participants