-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
semantics for GLOBAL_RNG
and ENV
#42899
Comments
Great example of the inherent problems with deepcopy :) One can easily see how deepcopy can copy "too much", but it can also copy too little! In either case the problem is that deepcopy cannot obey the abstractions that objects present; it can only traverse the object graph and duplicate the representations of objects. Note that adding an overload for deepcopy might seem like it could fix the problem, but it can't, because you might have
When deepcopy tries to recursively copy the field, it would get something of a different type (due to the overload), resulting in a type error or an invalid object. Of course, |
This would not be expected to be true, since an rng has mutable state. |
IMO it's questionable whether that type existing can be relied upon at all - I wouldn't consider the |
RNG might be an even weirder example, since for a PRNG, we'd expect a copy to change the object identity, but for a real RNG (such as RandomDevice), we'd expect it not to change the identity. |
Under the hood, |
Fair point, but whether it is public doesn't really matter. The key feature is that you can have a field with that type, and since |
For comparison, what would you expect to happen when serializing and deserializing |
I'd expect it to give a |
Well, from my POV
I'd expect to get the same stream of random numbers from the deserialized version as I would get from the non-serialized version, while not affecting the original, when passed as the first argument to a
|
In other words (even though I know internally it's not/only sort-of implemented like that): We can picture a |
Unifying this with the above - I think a |
It would be nice to get rid of the |
Right - the problem is the interaction with the task machinery. One way out of this would be to have |
When an object has a reference to Having @cscherrer Can you say more about how this case arose? Did you need to As many of us have long said, |
Hi Jeff, The problem I was working on was to be able to build a reproducible infinite stream of values. Ideally, this would use a purely functional RNG, but all the infrastructure seems to assume mutability, so I wasn't sure I could get that to work for arbitrary samplers. So as a hack, I made a I guess a more rigorous mutable approach would have something like Rust's borrow checker, so you can guarantee that only one other object can hold the reference to the RNG. But you know, baby steps :) Anyway, that why I needed to copy RNGs. And I always just thought |
Thanks, makes sense. In this case the solution is certainly to |
Thanks, I guess the case that's not clear to me is if you have something like a vector of tuples of arrays. I could imagine |
Our Arrays don't own their elements, so |
tl;dr don't use |
This was very surprising to me, and took me a very long time to figure out:
Even more surprising is
There's similar behavior with
ENV
in place ofGLOBAL_RNG
.There are a few issues here:
deepcopy
onGLOBAL_RNG
is very different than for other RNGs. It's not clear that the former should be "special" in this way.deepcopy
to be a stronger version ofcopy
, in the sense thatcopy
may still result in shared mutable sub-structures, but thatdeepcopy
would replicate these, so the result ofdeepcopy
would have no mutable substructures in common with the original.I had some discussion about this on Zulip with @Seelengrab and @fredrikekre here. Fredrik pointed out that
If this is really the intent, it seems it ought to be well-documented, with guidance on when to use which one.
In any case, I can't imagine a use case for the current arrangement where
deepcopy
can result in strictly more sharing thancopy
.The text was updated successfully, but these errors were encountered: