importlib: set children as attribute of parent modules

[nicoddemus]

Now importlib mode will correctly set the imported modules as an attribute of their parent modules.

As helpfully posted on #12194, that's how the Python import module works so we should follow suit.

In addition, we also try to import the parent modules as part of the process of importing a child module, again mirroring how Python importing works.

Because of the latter, I think it is more prudent to not backport this and release this in the next minor release: I fear importing the parent module like we are doing here might trigger some side effect in test suites in the wild, which would count as a change in behavior.

Fix #12194

[nicoddemus]

cc @jaraco @flying-sheep

[jaraco]

Confirmed, this does fix the issue in keyring. Thanks!

[jaraco]

I tested this against a package with a namespace package (jaraco.collections) and it also works, so that's good.

Because of the latter, I think it is more prudent to not backport this and release this in the next minor release: I fear importing the parent module like we are doing here might trigger some side effect in test suites in the wild, which would count as a change in behavior.

Good to know. I'll probably then need to update my projects so they don't pick up 8.1.*. I've currently excluded only 8.1.1.

What do you think about having a selective bugfix for the 'setattr' behavior in 8.1.x and then releasing this change as 8.2?

[flying-sheep]

I tested #12169 as well as 8b393d7 (latest commit here at time of writing) with our package (scverse/scanpy@65f567e) and both work.

I think the issue we encountered is a bit different from @jaraco’s, and I’m going to describe it so you can judge if you think that case is properly fixed now, because I haven’t been able to make a minimal reproducer so far:

If you run pytest 8.1.1 on the above scanpy commit, and a directory named data exists in the repo root, there will be an ImportError: Cannot import <thing> from scanpy._testing.helpers.data.

That’s because pytest 8.1.1 misimports scanpy._testing.helpers.data (a regular submodule) and puts data (a directory not intended to be a module at all) into sys.modules['scanpy._testing.helpers.data'], i.e. it imports a toplevel package instead of a subpackage.

[nicoddemus]

Thanks folks for the feedback.

What do you think about having a selective bugfix for the 'setattr' behavior in 8.1.x and then releasing this change as 8.2?

Not sure, because it would be a different patch than the one here -- it currently depends on the parent being imported explicitly by the function.

I also think it would be cleaner to depend on 8.2.0+ for all the fixes, rather than depend on 8.1.2+.

I've created #12213 to start planning the next minor, in the hope we can have it sooner than later.

I’m going to describe it so you can judge if you think that case is properly fixed now.

TBH is hard to judge from the description alone... a reproducer indeed would be great to have, even if to add to our test suite as a regression in case it works with this PR already.

[flying-sheep]

Here we go! This test fails on 8.1.1 and succeeds now.

It closely matches what we observed in our package.

def test_import_submodule_not_namespace(pytester: Pytester) -> None:
    pytester.syspathinsert()
    # create actual package with a submodule
    pytester.path.joinpath("foo").mkdir()
    (foo_path := pytester.path.joinpath("foo/__init__.py")).touch()
    (bar_path := pytester.path.joinpath("foo/bar.py")).touch()
    # create directory in `sys.path` with the same name as that submodule
    pytester.path.joinpath("bar").mkdir()

    # Import `foo`, then `foo.bar` and ensure the correct module has been imported.
    import_path(
        foo_path,
        mode=ImportMode.importlib,
        root=pytester.path,
        consider_namespace_packages=False,
    )
    bar = import_path(
        bar_path,
        mode=ImportMode.importlib,
        root=pytester.path,
        consider_namespace_packages=False,
    )
    assert bar.__name__ == "foo.bar"
    # If the `bar` directory has been imported instead of the submodule, both assertions will fail
    assert type(bar.__loader__).__name__ != "NamespaceLoader"
    assert bar.__file__ == str(bar_path)

[nicoddemus]

Thanks @flying-sheep!

Seems similar to the other test we have, except for pytester.path.joinpath("bar").mkdir(). Is that an important part of the reproducer, or perhaps a left-over? In case of the former, I will add this to the test suite later. 👍

[flying-sheep]

That’s the vital part. if you run this test with 8.1.1, the second import_path call will import the bar directory instead of the foo.bar submodule, which will cause the test to fail.

I added some comments to the test

[nicoddemus]

Awesome, thanks for confirming.

Feel free to push that test yourself if you have the time, otherwise I will get to it later.