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

Needs docs on module initialization #167

Closed
timholy opened this issue Sep 26, 2024 · 4 comments · Fixed by #182
Closed

Needs docs on module initialization #167

timholy opened this issue Sep 26, 2024 · 4 comments · Fixed by #182

Comments

@timholy
Copy link

timholy commented Sep 26, 2024

The README doesn't describe how to handle ParallelStencil's initialization requirements. It should mention that each dependent package needs a __init__() function that calls @init_parallel_stencil, otherwise folks may try to call it from top-level in their module. xref timholy/Revise.jl#821

Alternatively (and better), you could switch to storing all the needed settings in the dependent module, e.g., have @init_parallel_stencil create a global const __parallel_stencil_initialized__ = true in the downstream package. It would also have to store any needed settings as well, of course. That would be precompile-friendly. The reason this is better is that (1) users don't need to know anything about __init__ functions, and (2) __init__ functions are awful black-boxes when it comes to the upcoming ability to compile small binaries (we will have to cache them regardless of whether they are needed). So it should be a win-win to move your storage into the dependent package.

@omlins
Copy link
Owner

omlins commented Oct 23, 2024

@timholy Thanks a lot for opening this issue. Yes, moving the storage into the dependent module is definitely the better way to ensure compatibility with to revise.

@timholy
Copy link
Author

timholy commented Oct 24, 2024

Not just Revise. It's really a package precompilation issue: currently, any "work" that needs to be done to initialize ParallelStencil must be done from within an __init__ function in the dependent package, otherwise that work is done ephemerally only when the package is precompiled. Since the "results" are not saved in ParallelStencil's precompilation cache, things will just break.

@omlins
Copy link
Owner

omlins commented Nov 6, 2024

@timholy: similar observations as you made with revise.jl can be made with pluto.jl, as a newly opened issue just showed: #178 . Naturally, I would like to do a fix that resolves both issues. Would you mind having a quick look at what was reported? Do you think storing all the needed settings in the dependent module should also solve the pluto issue?

Alternatively (and better), you could switch to storing all the needed settings in the dependent module, e.g., have @init_parallel_stencil create a global const parallel_stencil_initialized = true in the downstream package.

Thanks a lot for your suggestion. Instead of creating global constants for each setting in the caller module, I would prefer to create a single metadata submodule in the caller module bundling all the settings. Do you see any issues with this solution?

@omlins
Copy link
Owner

omlins commented Dec 11, 2024

Instead of creating global constants for each setting in the caller module, I would prefer to create a single metadata submodule in the caller module bundling all the settings.

@timholy This worked out without any issues and we have now full compatibility with Revise.jl. Thanks for the hint!
A side note: we did not have any problems is pre-compilation in general (we have unit tests to ensure that incremental precompilation as well as usage of extensions works).

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

Successfully merging a pull request may close this issue.

2 participants