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

Changes to submodule are not reflected when reloading #165

Closed
klar-C opened this issue Jan 8, 2021 · 12 comments · Fixed by #195
Closed

Changes to submodule are not reflected when reloading #165

klar-C opened this issue Jan 8, 2021 · 12 comments · Fixed by #195

Comments

@klar-C
Copy link

klar-C commented Jan 8, 2021

Hi,

I'm going through the utils-submodule example from the current documentation.

When changing the seq.R file and then reloading with either reload() or just rerunning import, the changes to the submodule are not reflected.

Is that the expected behavior? If so, how can I reload it?

(I can provide an example but literally all I did was to add one function to seq.R.)

@klmr
Copy link
Owner

klmr commented Jan 8, 2021

See #39; unfortunately this is a long-standing bug that should hopefully be fixed at some point — unfortunately doing so is not trivial.

@klar-C
Copy link
Author

klar-C commented Jan 8, 2021

Okay thanks. My understanding is that having the super/submodule setup is the only way to have more than one .r file in the same module. Is that correct?

@klmr
Copy link
Owner

klmr commented Jan 8, 2021

In general that’s correct, though your __init__.r module file can forward submodule imports via export_submodule:

a/__init__.r:

export_submodule('./foo')

a/foo.r:

f = function () message('hi!')

Now when you import a via

a = modules::import('a')

Then you can access f directly via a$f.

@klar-C
Copy link
Author

klar-C commented Jan 8, 2021

Thank you.

@klar-C
Copy link
Author

klar-C commented Jan 8, 2021

Apologies - I wanted to add one more question:

What is your suggested workflow of developing 2 modules that work together?

  • I have one main module A which references module B.
  • Both have multiple R files.
  • (They are both separate R projects that in the end will become normal packages.)

If I work on A, and then need to fix something in B: Is the only way right now to reflect those changes in A to restart the R session?

@klmr
Copy link
Owner

klmr commented Jan 9, 2021

Ah, yes, I should probably have mentioned this above: You can manually flush the internal module cache. I wouldn’t generally recommend this, since it’s modifying implementation details; but it can serve as a workaround while the present bug exists.

Internally, the package keeps track of loaded modules in an un-exported environment, loaded_modules. The environment uses the module paths as keys. So for the session from the example above, ls(modules:::loaded_modules) would display something like this:

[1] "/full/path/to/a/__init__.r" "/full/path/to/a/foo.r"

You can manually delete the internal/nested module, and then reload the outer module:

rm(list = '/full/path/to/a/foo.r', envir = modules:::loaded_modules)
modules::reload(a)

Or you can choose the “nuclear” option and clear the entire list of loaded modules:

rm(list = ls(modules:::loaded_modules), envir = modules:::loaded_modules)
a = modules::import('a')

Beware that the latter option will break reload and require you to use import again. Other loaded modules should still continue to work regularly in the current session but I might be overlooking some corner case here.

@klar-C
Copy link
Author

klar-C commented Jan 9, 2021

Thank you!

@klmr klmr linked a pull request Apr 3, 2021 that will close this issue
@klar-C
Copy link
Author

klar-C commented May 21, 2021

Hi - I'm revisiting this package (it has come a very long way since we talked).

I wanted to ask whether reloading submodules is still an issue or whether I'm doing something wrong with the new syntax.

Like in your example in the documentation I have 3 files:
A(my env where I test stuff such as the global env)/B/C
A uses B and B uses C through the use statements.
Changing code in B works fine - any changes are reflected in A right away.
But changing code in C does not work - I need to restart R for B to be able to see the changes in C.
Am I doing something wrong?

(See code below.)

`
a.R:
box::use(b = ./b)
b$callme()

b.R
#' @export
callme <- function() {
box::use(c = ./c)
c$callme()
}

c.R
#' @export
callme <- function() {
print("hello")
}

`

@klmr
Copy link
Owner

klmr commented May 21, 2021

Apologies, this is indeed still in the works, because a fix isn’t at all trivial. If you’re keen, you could try out PR #195, which is the draft implementation; the following should work for an installation from source:

devtools::install_github('klmr/box@feature-reload-submodules')

This should work with many (most?) scenarios, though it isn’t well tested yet. The major limitation is that reloading modules with native code might cause crashes under specific circumstances (but then, native code is not explicitly supported anyway).

I should probably merge the PR quite soon so that people can experiment with it. Worst case, a subsequent release will roll it back.

@klar-C
Copy link
Author

klar-C commented May 21, 2021

Thanks!

I actually tried out your old suggestion with the nuclear option (and subsequent reload)
rm(list = ls(box:::loaded_mods), envir = box:::loaded_mods)
which seems to work fine (just the name of the env changed).

Are there any new side-effects of doing this besides what you already mentioned to me?

@klmr
Copy link
Owner

klmr commented May 21, 2021

There are two other side effects:

  • Perhaps obviously, it reloads all submodules
  • It doesn’t execute the .on_unload hooks. But their main use is also to unload native code, so there might be no added detrimental effect (though there are other uses for this hook, such as closing database connections).

klmr added a commit that referenced this issue May 21, 2021
Naïve implementation of dependency reloading (fixes #39, #165)
@klar-C
Copy link
Author

klar-C commented May 23, 2021

Okay thanks!

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.

2 participants