fix[next]: unique sys.modules key per compiled program variant - #2740
fix[next]: unique sys.modules key per compiled program variant#2740tehrengruber wants to merge 2 commits into
Conversation
00c8b78 to
a93f3af
Compare
a93f3af to
ba333cf
Compare
egparedes
left a comment
egparedes
left a comment
There was a problem hiding this comment.
I'm not sure about using the holding variable as a cache, so proposed an alternative.
| if keep_reference and (cached := _loaded_modules.get(key)) is not None: | ||
| return cached |
There was a problem hiding this comment.
I'm not sure if using the _loaded_modules variable as a cache is a good idea, I think it can add hidden bugs in weird corner cases which might be very hard to track. Are you aware of any scenario where this would matter for performance? Otherwise, I think I'd prefer to define it something similar to:
_loaded_modules: list[tuple[str, Module]]
...
_loaded_modules.append((key, loaded_module))
…llisions GridTools#2431 kept a nanobind module alive by registering it in `sys.modules`, but the key was just the `entry_point_name`. Static variants of the same program share that name (and the `.so` base name), so they collided and evicted one another: # variant_a / variant_b: same program, different static args func_a = variant_a.load() # sys.modules["...copy"] = module_a func_b = variant_b.load() # overwrites the key; module_a now only # reachable via func_a del variant_a # on nanobind >=2.10 module_a can be GC'd (nb_module_clear) while func_a # is still in use -> segfault when func_a is called Replace the `sys.modules` registration with an opt-in `keep_reference` flag on `import_from_path` that appends the module to a private, process-wide list. The compiler's `load()` (which returns a function to be called later) passes it, so every loaded variant stays alive; callers that only introspect the module leave the default and don't pollute the process.
ba333cf to
8ea829a
Compare
| module = importer.import_from_path( | ||
| file, add_to_sys_modules=True, sys_modules_prefix="_temp_test_prefix_" | ||
| ) | ||
| # Without keeping a reference, the module is loaded but not retained. |
There was a problem hiding this comment.
Not sure if these tests still make sense. I'm leaning to remove them.
There was a problem hiding this comment.
Those are testing if the keep_reference option works as intended, right? I think they still make sense, but feel free to remove them since in the current trivial this is trivially true.
| module = importer.import_from_path( | ||
| file, add_to_sys_modules=True, sys_modules_prefix="_temp_test_prefix_" | ||
| ) | ||
| # Without keeping a reference, the module is loaded but not retained. |
There was a problem hiding this comment.
Those are testing if the keep_reference option works as intended, right? I think they still make sense, but feel free to remove them since in the current trivial this is trivially true.
2 participants
#2431 keeps a nanobind module alive by registering it in
sys.modules, but the key was just theentry_point_name. Static variants of the same program share that name (and the.sobase name), so they collided and evicted one another:Solution here is to not rely on
sys.modules, but just keep a reference to the module in a global variable.Text and code generated with the help of LLMs and then verified by me.