Skip to content

replace DummyMod with a more robust ModuleType that supports a custom user_ns #14754

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.

Sign up for GitHub

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

Jump to bottom
Merged
Carreau merged 3 commits into from
Feb 23, 2025
Merged

replace DummyMod with a more robust ModuleType that supports a custom user_ns #14754

Carreau merged 3 commits into from
Feb 23, 2025

Conversation

@smacke
Copy link
Contributor

@smacke smacke commented Feb 20, 2025

What changes are proposed in this pull request?

Some IPython apps, such as the one used in Databricks, need to seed the shell with a custom user namespace before it is actually created. This means the main module used by ipython can’t be a normal ModuleType (which doesn’t support overwriting dict) but instead a custom “DummyMod”. DummyMod isn’t considered an instance of ModuleType, which can mess up tools like ipytest + pytest -- see, for example, pytest-dev/pytest#12965.

To fix, in this PR we create a helper function that generates a ModuleType subclass on-the-fly and that uses the user namespace provided a priori as its __dict__. Note that ModuleType.__dict__ is read-only, which is probably why IPython originally created DummyMod. To get around that, we create a __dict__ property and use custom getters / setters to delegate to the user_ns we pass in.

Searching around, it seems like DummyMod / FakeModule may have caused some pain points in the past, e.g. around pickling (#384, #112) -- still trying to determine if this change can help with that.

How is this tested?

Manual + pre-merge

@smacke
Copy link
Contributor Author

smacke commented Feb 20, 2025

FYI @jasongrout

@smacke
Copy link
Contributor Author

smacke commented Feb 20, 2025

Looks like there are some test issues around pickling that need to be fixed; will get to those in a bit

@smacke
Copy link
Contributor Author

smacke commented Feb 20, 2025

Looks like a bunch of the pickling issues in downstream tests are failing for unrelated reasons, and then there's another test that expects the string DummyMod to be present. Could potentially fix by renaming IPythonMainModule back to DummyMod.

@smacke
Copy link
Contributor Author

smacke commented Feb 20, 2025

The failure in test_embed.py seems to be unrelated; I see it happening on another PR as well: https://github.com/ipython/ipython/actions/runs/13446531221/job/37572996725?pr=14753

@Carreau
Copy link
Member

Carreau commented Feb 21, 2025 

FAILED tests/test_ipkernel_direct.py::test_do_apply - ModuleNotFoundError: No module named 'IPython.utils.coloransi'
FAILED tests/test_pickleutil.py::test_no_closure - TypeError: cannot pickle code objects
FAILED tests/test_pickleutil.py::test_generator_closure - TypeError: cannot pickle code objects
FAILED tests/test_pickleutil.py::test_nested_closure - TypeError: cannot pickle code objects
FAILED tests/test_pickleutil.py::test_closure - TypeError: cannot pickle code objects

All those are unrelated, and will be fixed with a new ipyparallel release. Don't worry about those.

I think we can update ipykernel 6.x and 7 (not release), for test to allow for IPythonMainModule name instead of DummyModule

@Carreau Carreau mentioned this pull request Feb 23, 2025
Merged
@Carreau
Copy link
Member

Carreau commented Feb 23, 2025

Let's try.

@Carreau Carreau merged commit f68a791 into ipython:main Feb 23, 2025
16 checks passed
@Carreau Carreau added this to the 9.0 milestone Feb 23, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

9.0

Development

Successfully merging this pull request may close these issues.

2 participants

@smacke @Carreau