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

sys.modules cannot be reassigned #62153

Closed
progval mannequin opened this issue May 11, 2013 · 12 comments
Closed

sys.modules cannot be reassigned #62153

progval mannequin opened this issue May 11, 2013 · 12 comments
Assignees
Labels
docs Documentation in the Doc dir easy

Comments

@progval
Copy link
Mannequin

progval mannequin commented May 11, 2013

BPO 17953
Nosy @brettcannon, @terryjreedy, @pitrou, @bitdancer, @progval, @ericsnowcurrently
Files
  • issue17953.patch: Adds statement "However, replacing it or deleting critical entries may cause Python to fail."
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/brettcannon'
    closed_at = <Date 2013-05-24.21:33:08.263>
    created_at = <Date 2013-05-11.08:23:33.811>
    labels = ['easy', 'docs']
    title = 'sys.modules cannot be reassigned'
    updated_at = <Date 2013-06-25.05:14:26.503>
    user = 'https://github.com/ProgVal'

    bugs.python.org fields:

    activity = <Date 2013-06-25.05:14:26.503>
    actor = 'eric.snow'
    assignee = 'brett.cannon'
    closed = True
    closed_date = <Date 2013-05-24.21:33:08.263>
    closer = 'brett.cannon'
    components = ['Documentation']
    creation = <Date 2013-05-11.08:23:33.811>
    creator = 'Valentin.Lorentz'
    dependencies = []
    files = ['30305']
    hgrepos = []
    issue_num = 17953
    keywords = ['patch', 'easy', '3.3regression']
    message_count = 12.0
    messages = ['188901', '188914', '188945', '189312', '189317', '189529', '189933', '189934', '189938', '189939', '189940', '191833']
    nosy_count = 8.0
    nosy_names = ['brett.cannon', 'terry.reedy', 'pitrou', 'r.david.murray', 'Valentin.Lorentz', 'python-dev', 'eric.snow', 'Yogesh.Chaudhari']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'needs patch'
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue17953'
    versions = ['Python 3.3', 'Python 3.4']

    @progval
    Copy link
    Mannequin Author

    progval mannequin commented May 11, 2013

    In Python 3.3 (I did not test with 3.4), sys.modules cannot be reassigned without breaking the import mechanism; while it works with Python <= 3.2.

    Here is how to reproduce the bug:

    progval@Andromede:/tmp$ mkdir test_imports
    progval@Andromede:/tmp$ echo "from . import module" > test_imports/__init__.py
    progval@Andromede:/tmp$ echo "print('foo')" > test_imports/module.py
    progval@Andromede:/tmp$ python3.3
    Python 3.3.1 (default, Apr  6 2013, 13:58:40) 
    [GCC 4.7.2] on linux
    Type "help", "copyright", "credits" or "license" for more information.
    >>> import sys
    >>> sys.modules = dict(sys.modules)
    >>> import test_imports
    Traceback (most recent call last):
      File "<stdin>", line 1, in <module>
      File "./test_imports/__init__.py", line 1, in <module>
        from . import module
    SystemError: Parent module 'test_imports' not loaded, cannot perform relative import
    >>> 
    progval@Andromede:/tmp$ python3.2
    Python 3.2.3 (default, May  6 2013, 01:46:35) 
    [GCC 4.7.2] on linux2
    Type "help", "copyright", "credits" or "license" for more information.
    >>> import sys
    >>> sys.modules = dict(sys.modules)
    >>> import test_imports
    foo
    >>>

    @progval progval mannequin added type-bug An unexpected behavior, bug, or error interpreter-core (Objects, Python, Grammar, and Parser dirs) labels May 11, 2013
    @pitrou
    Copy link
    Member

    pitrou commented May 11, 2013

    I wouldn't call it a bug personally. The modules dictionary is used in all kinds of places in the interpreter; you can change the dictionary's contents, but not swap it with another one.

    It's just a pity that we can't forbid reassignment altogether.

    @brettcannon
    Copy link
    Member

    There is no good way to solve this. At the C level there interpreter struct has two key fields, sysdict and modules. The former is sys.__dict__ and the latter is sys.modules. But when you re-assign sys.modules you then break the assumption that sys.modules is the same dict as that contained in interp->modules. And this all goes out the window as the C code is expected to use interp->modules while the Python code in importlib only has access to sys.modules. The reason this used to "work" is your new dictionary was completely ignored and so we basically a no-op from the perspective of import (done in Python 2.7 but same result in any version up to Python 3.3)::

      >>> import sys
      >>> original_modules = sys.modules
      >>> new_modules = sys.modules.copy()
      >>> sys.modules = new_modules
      >>> import pkg
      >>> 'pkg' in original_modules
      True
      >>> 'pkg' in new_modules
      False

    What really needs to happen is that sys.modules needs to be documented as something that cannot be replaced. If you really want to update it cleanly then do sys.modules.clear(); sys.modules.update(new_modules), but even that is tricky because removing certain modules will flat-out break Python.

    I have updated the issue to be a documentation one and added Python 3.4 to the affected versions.

    @brettcannon brettcannon added easy docs Documentation in the Doc dir and removed interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error labels May 11, 2013
    @terryjreedy
    Copy link
    Member

    "sys.modules
    This is a dictionary that maps module names to modules which have already been loaded. This can be manipulated to force reloading of modules and other tricks."

    How about adding at the end
    "However, replacing or clearing it may cause Python to fail."

    or to be slightly more explicit "However, replacing it or deleting critical entries may cause Python to fail."

    @brettcannon
    Copy link
    Member

    Sounds good to me.

    @YogeshChaudhari
    Copy link
    Mannequin

    YogeshChaudhari mannequin commented May 18, 2013

    Documentation added for sys.modules

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented May 24, 2013

    New changeset 4f8160e45cb7 by Brett Cannon in branch '3.3':
    Issue bpo-17953: document that sys.modules shouldn't be replaced (thanks
    http://hg.python.org/cpython/rev/4f8160e45cb7

    New changeset b60ae4ebf981 by Brett Cannon in branch 'default':
    merge fix for issue bpo-17953
    http://hg.python.org/cpython/rev/b60ae4ebf981

    @brettcannon
    Copy link
    Member

    Thanks for the patch, Yogesh, but I went with different wording that I came up with on the train the other day while offline. But you did remind me to update Misc/NEWS (which led to a discussion on python-committers), so thanks for that.

    @YogeshChaudhari
    Copy link
    Mannequin

    YogeshChaudhari mannequin commented May 25, 2013

    You are most welcome Brett. I am just starting to contribute with this doc fix. Didn't think a small detail in it will generate such a long debate in the python-committers mailing list.

    Is there a plan in action for 'fixing' Misc/NEWS. I now realize that this change is too small to warrant an entry there, however, as someone new to the list, I simply followed http://docs.python.org/devguide/patch.html. Doing a make patchcheck warns about no changes to Misc/NEWS.

    IMHO it would be great if this tool can look at the files changed (or maybe at-least just the directory, or even, simply number of lines modified) and decide if an entry is required in Misc/News. Of-course this is from my perspective. I would like to get your view on this and help about the solution if possible.

    @bitdancer
    Copy link
    Member

    I think that is a reasonable suggestion for an enhancement to patchcheck (though I think Brett had a good reason to add a news entry in this particular case). You could open a new issue with that suggestion. On the other hand, patchcheck always warns about other stuff that it can't so easily realize aren't really needed, so it may not be considered a worthwhile enhancement.

    @YogeshChaudhari
    Copy link
    Mannequin

    YogeshChaudhari mannequin commented May 25, 2013

    That sounds good. I will open up an issue (if not for anything else other than to get more eyes on this issue)

    @ericsnowcurrently
    Copy link
    Member

    For the record, bpo-12633 has some more discussion on this.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    docs Documentation in the Doc dir easy
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants