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

deep copy not copying data #153

Closed
berceanu opened this issue Feb 27, 2019 · 4 comments
Closed

deep copy not copying data #153

berceanu opened this issue Feb 27, 2019 · 4 comments
Assignees
Labels
bug Something isn't working
Milestone

Comments

@berceanu
Copy link

statepoint = copy.deepcopy(job.sp)
del statepoint['a']

results in changes to the workspace folder, i.e. the removing of parameter 'a' from signac_statepoint.json.

@csadorf csadorf added the bug Something isn't working label Feb 27, 2019
@csadorf csadorf added this to the v1.0.0 milestone Feb 27, 2019
@csadorf
Copy link
Contributor

csadorf commented Feb 27, 2019

Thank you for reporting! Which Python version are you running?

@csadorf
Copy link
Contributor

csadorf commented Feb 27, 2019

@berceanu I'm having trouble reproducing the bug. Can you verify that this unit test should trigger the described behavior?

@csadorf csadorf removed this from the v1.0.0 milestone Feb 27, 2019
@bdice
Copy link
Member

bdice commented Feb 27, 2019

This triggers the confusing behavior:

import copy
import os
import signac
pr = signac.init_project('issue_153')
job = pr.open_job({'a': 1})
job.init()
print(job._id)  # 42b7b4f2921788ea14dac5566e6f06d0
copy_sp_bug = copy.deepcopy(job.sp)
copy_sp_bug['a'] = '🐛'
print(job.sp)  # {'a': 1}
print(copy_sp_bug)  # {'a': '🐛'}
print(list(iter(project)))  # [signac.contrib.job.Job(project=signac.contrib.project.Project({'project': 'issue_153', 'project_dir': '/home/bdice/tmp_signac_153', 'workspace_dir': '/home/bdice/tmp_signac_153/workspace'}), statepoint={'a': '🐛'})]
assert job in project  # fails! We have a job handle that doesn't exist in the project anymore...?

@csadorf csadorf added this to the v1.1.0 milestone Mar 4, 2019
@csadorf csadorf modified the milestones: v1.1.0, v1.2.0, v1.1.1 May 19, 2019
@vyasr vyasr self-assigned this May 28, 2019
@csadorf csadorf modified the milestones: v1.1.1, v1.1.2 Jul 7, 2019
@csadorf csadorf modified the milestones: v1.1.2, v1.2.0 Jul 17, 2019
@csadorf
Copy link
Contributor

csadorf commented Jul 19, 2019

After looking more closely at this issue, we have come to the conclusion that it actually makes most sense that a copy or a deepcopy will still modify the underlying state point and thus job albeit operating on a different in-memory reference. That is because a state point and a job for that matter are directly tied to a specific location on the file system and that doesn't change just because we have a different reference.

What users actually intend when they create a deepcopy of a state point is to generate a dict that is no longer tied to the job and thus the file system. For this, users are advised to use Job.sp() or Job.sp._as_dict().

We have fixed the deepcopy behavior of Job and added unit tests to ensure that we actually implement the behavior described above in pull request #207 .

To help avoid user confusion, we will need to improve the documentation on how to convert state points to dicts (glotzerlab/signac-docs#39).

@csadorf csadorf closed this as completed Jul 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

4 participants