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 the module #70

Closed
jlperla opened this issue Dec 9, 2021 · 5 comments
Closed

Deepcopy of the module #70

jlperla opened this issue Dec 9, 2021 · 5 comments
Assignees

Comments

@jlperla
Copy link
Member

jlperla commented Dec 9, 2021

No description provided.

@jlperla
Copy link
Member Author

jlperla commented Dec 9, 2021

For now, add the following

import Base.deepcopy_internal
deepcopy_internal(x::Module,stackdict::IdDict) = x

Note that this basically makes Module's shallow copy. i.e., treats them as references. This is very unlikely to cause issues globally since right now it errors.

I tried to see if tehre was a way to "stop" the recursiion to copy a weak reference but it doesn't seem immediately possible. Later we can revisit this hack.

@jlperla
Copy link
Member Author

jlperla commented Dec 14, 2021

This is triggering a compilation warning, so we should revsit at some point:

[ Info: Precompiling DifferentiableStateSpaceModels [beacd9db-9e5e-4956-9b09-459a4b2028df]
WARNING: Method definition deepcopy_internal(Module, Base.IdDict{K, V} where V where K) in module Base at deepcopy.jl:33 overwritten in module DifferentiableStateSpaceModels at c:\Users\jesse\Documents\GitHub\DifferentiableStateSpaceModels.jl\src\types.jl:2.
  ** incremental compilation may be fatally broken for this module **

@jlperla jlperla reopened this Dec 14, 2021
@wupeifan
Copy link
Member

Yeah, essentially we overrode that line in deepcopy.jl

@cpfiffer
Copy link
Collaborator

Def. low priority for now, or at least it seems that way. One performance hit we could be exposing ourself to is that we're overwriting a fairly core Julia function, which could trigger a bunch of recompilations. Either way, the hit we're taking is at JIT spinup time and not runtime, so I'm not terribly worried for now.

@jlperla
Copy link
Member Author

jlperla commented Dec 14, 2021

Exactly. Yeah, this was intended as a hack to get us through, and we would need to revsit. What I didn't realize was that it was overwriting anything - rather than just adding in something that didn't already exist. But I don't think it will cause any immediate issues with the runtime.

The other things I tried (e.g. storing a Ref{Module} had the same problems since it didn't seem like there was any other convenient way to stop the recusion without a custom type.

So the next stage in the hack is to add in a wrapper DontDeepCopyMe{Module} which stops the deepcopy recusiion at the custom type so then piracy is no longer required. But this whole thing was just temporary anyways, so we shall see.

@wupeifan wupeifan removed their assignment Dec 21, 2021
@jlperla jlperla self-assigned this Feb 26, 2022
jlperla added a commit that referenced this issue Feb 27, 2022
jlperla added a commit that referenced this issue Feb 28, 2022
@jlperla jlperla closed this as completed Feb 28, 2022
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

3 participants