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

reload() does not reload submodules #39

Closed
mschubert opened this issue Sep 4, 2014 · 8 comments · Fixed by #195
Closed

reload() does not reload submodules #39

mschubert opened this issue Sep 4, 2014 · 8 comments · Fixed by #195

Comments

@mschubert
Copy link
Collaborator

Consider the following module:

a
|- __init__.r
|- file.r

Editing and reloading does not reload a submodule's changes:

a = import('amodule')
# change file.r
reload(a) # this does not load the changes in file.r

Having reload() default to shallow reloading that only reloads the __init__ file doesn't make any sense. Either make shallow-whole-module reloading default or full deep reloading.

@klmr
Copy link
Owner

klmr commented Jul 30, 2015

This needs more reflection than I thought:

Reloading super-modules

What’s clear is that reload(a) should reload a and all its (module) dependencies, which may also have changed. However, what about super-modules? In other words, assume the following structure:

a
|- __init__.r
|- file.r

Now, does the following reload a, or does it only reload a/file?

file = import('a/file') # implicitly loads 'a'
reload(file)

Unloading dependent modules

What is unload(a) supposed to do? At the moment it doesn’t touch dependencies, since these may also have been loaded from elsewhere. However, reload(a) is currently specified such that it does the same as unload(a) followed by a = import(a), using the exact same import parameters that were used to import a previously.

After the changes discussed in this issue, the semantics of reload(a) would no longer match this behaviour. Is this intended? It isn’t necessarily a problem … I can’t decide which behaviour makes more sense.

@mschubert
Copy link
Collaborator Author

This might be a good time to revisit if import('a/file') should execute the __init__.r file by default (which, if I remember correctly, and you state above, it does). Do we expect a to be loaded when we import file? - Python does not show this behaviour either.

My first intuition is, that if I expect that a reload only reloads the file associated with the object I call it on. A potential issue starts, however, if I:

a = import('a')
file = import('a/file')
reload(file)

then can you easily reload file and a$file, without touching the rest of a? What about module b that references either a or a/file?

I think the most important consideration is to never leave modules in a broken state, i.e. never allow that two different objects to hold two different versions of the same module - this would be a nightmare to debug (and each module should be loaded only once and then referenced multiple times anyway).

Can you achieve this with either approach?

@klmr
Copy link
Owner

klmr commented Jul 30, 2015

Do we expect a to be loaded when we import file? - Python does not show this behaviour either.

It absolutely does:

⟩⟩⟩ tree .
.
└── foo
    ├── __init__.py
    └── bar.py
⟩⟩⟩ more foo/__init__.py
print ("foo")
⟩⟩⟩ more foo/bar.py
print ("foo/bar")
import foo.bar
foo
foo/bar

I guess this behaviour makes sense: submodules may expect that parent modules are set up correctly. This should only matter if the initialisers of these modules have side-effects, of course.

I can’t think of very strong arguments for and against this behaviour actually. The ones I can think of at the moment are rather mundane:

  • FOR: Consistency with Python; currently implemented, i.e. no change required
  • AGAINST: Removing it would simplify the code, and would make the implementation of this feature request much easier.

@klmr
Copy link
Owner

klmr commented Jul 31, 2015

For completeness’ sake, Python does not reload super-modules. So we should probably stay consistent with that.

@klmr
Copy link
Owner

klmr commented Jul 31, 2015

To answer your other concern:

never allow that two different objects to hold two different versions of the same module

Actually I think the opposite is true: references to modules are truly independent of each other, once they’re loaded.

a = import('a')
b = import('a')
reload(a)

This will not touch b, which continues to refer to the old version of a. This is actually important because b may be somewhere deep inside another (loaded) module that relies on the old semantics to work. So no, reloading a module should not touch other references to that module (incidentally, this is also documented correctly).

Can you achieve this with either approach?

Not easily, and as I’ve said above I actually don’t want to achieve this.

However, this behaviour is inconsistent with Python.

@gaborcsardi
Copy link

To answer your other concern:

never allow that two different objects to hold two different versions of the same module

Actually I think the opposite is true: references to modules are truly independent of each other, once they’re loaded.

Yes, I think this is definitely the way to go!

It is somewhat more complicated to achieve this with

  • modules with compiled source,
  • tricky packages loaded as modules, e.g. packages that call system.file(), and alike, and
  • modules with S4 classes/object,

For the first problem, you need to make sure that the shared libs you load have different paths in the file system, e.g. linking them to a temporary dir is enough. Maybe even symlinking.

For the second, you probably don't need to do much, maybe having a shim for system.file() is worthwhile. For S4, I have no idea.

@klmr
Copy link
Owner

klmr commented Jul 31, 2015

Yikes. I hadn’t even considered shared libraries, to be honest. The proposed solution only works as long as the shared library hasn’t itself got shared library dependencies (that might be recompiled), and it’s probably impractical to copy the whole dependency graph of shared libraries anyway. In that sense I might make it a conscious decision (properly documented, of course) to offer only limited support for shared libraries in reload.

S4 is currently unsupported by modules anyway and, if I don’t hear compelling testimony, never will be. I’d be more interested in getting R6 running but I don’t have the necessary knowledge for that.

@gaborcsardi
Copy link

I don't think the shared libraries are too bad, but cannot say for sure until we try.

R6 should be easy, it does not introduce any new namespaces like S4. R6 objects are just environments, so as long as modules can contain environments, you are fine. I am fairly sure that everything will just work, including cross module inheritance.

@klmr klmr modified the milestone: 1.0 Jul 15, 2016
@klmr klmr linked a pull request Apr 3, 2021 that will close this issue
@klmr klmr closed this as completed in #195 May 21, 2021
klmr added a commit that referenced this issue May 21, 2021
Naïve implementation of dependency reloading (fixes #39, #165)
radbasa pushed a commit to Appsilon/box that referenced this issue Jul 1, 2024
Naïve implementation of dependency reloading (fixes klmr#39, klmr#165)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants