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

un/registerJsModule fails various expectations #4029

Open
WebReflection opened this issue Aug 1, 2023 · 2 comments
Open

un/registerJsModule fails various expectations #4029

WebReflection opened this issue Aug 1, 2023 · 2 comments
Labels
bug Something isn't working

Comments

@WebReflection
Copy link
Contributor

WebReflection commented Aug 1, 2023

🐛 Bug

It is not clear nor transparent to users how both registerJsModule and unregisterJsModule work behind the scene and while I've found a workaround, I'd like to understand if the current behavior is actually acceptable to you ... please let me expand on that in the next section.

To Reproduce

// this throws errors if not previously defined which is OK
// pyodide.unregisterJsModule('test')

// this is fine
pyodide.registerJsModule('test', {a: 1});
// this is also fine *only if done before any import*
pyodide.registerJsModule('test', {a: 2});

// prints 2: correct!
pyodide.runPython('from test import a; print(a)');

// ...*BUT* ....
pyodide.registerJsModule('test', {a: 3});

// this *also prints 2* !!!
pyodide.runPython('from test import a; print(a)');

// so one might expect that doing this ...
pyodide.unregisterJsModule('test');

// would also drop it from the import cache so that this should throw
pyodide.runPython('from test import a; print(a)');
// instead of printing 2

// and also I could re-register it and be sure the value is the expected one
pyodide.registerJsModule('test', {a: 4});

// *nope* ... this will still print 2
pyodide.runPython('from test import a; print(a)');

// so that to actually really unregister a module one needs to:
pyodide.unregisterJsModule('test');
pyodide.runPython('import sys;del sys.modules["test"]');

// so now we talk !!!
pyodide.registerJsModule('test', {a: 5});
pyodide.runPython('from test import a; print(a)');
// 5 :partying_face: 

Expected behavior

  • it's unexpected that overwriting a module has expected results if never imported but unexpected results if previously imported
  • it's unexpected that, assuming the previous point is pythonic or a module cache expected behavior, once unregistered the module is still reachable as import in further code ... where has been unregistered if code can still use it and re-registring it won't ever change anything?
  • I hence would expect some error thrown when a module is unregistered, and imported after, or a proper cleanup of the module cache or I would expect an error thrown if I re-register a module that has already been imported, otherwise there are too many silent moving parts hard to debug or understand (imho)

Environment

  • Pyodide Version: 0.23.4
  • Browser version: any
  • Any other relevant information: @hoodmane seems to agree there might be room for improvements as this is a JS exposed API, not something pythonic that just behave like native Python would ... and I hope others agree something is weird with the current behavior.
@WebReflection WebReflection added the bug Something isn't working label Aug 1, 2023
@ryanking13
Copy link
Member

Thanks for opening the issue @WebReflection!

There was a similar discussion in pyodide/micropip#51, so the conclusion in this issue might also be applied to that one.

It's not very common to install modules at runtime, and even less common to uninstall them, so it's hard to say which way is better. It is also complicated by the fact that in Python, if an imported module is already in use, deleting it from sys.modules does not necessarily affect the module in use...

I am +1 with removing the module object from sys.modules when a module is re-registered / un-registered. This won't solve all of the problems, but it's somewhat close to "expected" behavior IMO.

@WebReflection
Copy link
Contributor Author

WebReflection commented Aug 3, 2023

I am +1 with removing the module object from sys.modules when a module is re-registered / un-registered. This won't solve all of the problems, but it's somewhat close to "expected" behavior IMO.

yeah, expectations is all I am after ... I understand if stuff was previously referenced and never let go the FinalizationRegistry won't get ever a chance to free those resources but I really believe once an explicit unregisterJsModule happens something must be different in further executions and, when a module is re-registered, something gotta happen ... either silently update the sys.modules cache or throw an error because already used in the wild ... I am all for replacing it with the new updated references that any further script can consume, thanks for considering the issue and proposing your suggested changes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants