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

Using __name__ on a non-package module results in "not a package" error #60

Closed
jaraco opened this issue Oct 21, 2020 · 13 comments
Closed
Labels
api wontfix This will not be worked on
Milestone

Comments

@jaraco
Copy link
Member

jaraco commented Oct 21, 2020

In GitLab by @jaraco on May 18, 2018, 11:36

Coming from pkg_resources, I'd previously used __name__ as the initial parameter:

>>> import pkg_resources
>>> import queso.validator.tests.test_options
>>> pkg_resources.resource_stream(queso.validator.tests.test_options.__name__, 'colliding questions.json')
<_io.BufferedReader name='/Users/jaraco/Dropbox/code/yg/G/queso/queso/validator/tests/colliding questions.json'>

But in importlib_resources, this approach doesn't work.

>>> importlib_resources.read_binary(queso.validator.tests.test_options.__name__, 'colliding questions.json')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/jaraco/Dropbox/code/yg/G/queso/.tox/py36/lib/python3.6/site-packages/importlib_resources/_py3.py", line 142, in read_binary
    package = _get_package(package)
  File "/Users/jaraco/Dropbox/code/yg/G/queso/.tox/py36/lib/python3.6/site-packages/importlib_resources/_py3.py", line 40, in _get_package
    raise TypeError('{!r} is not a package'.format(package))
TypeError: 'queso.validator.tests.test_options' is not a package

In other words, importlib_resources seems to be more stringent about the input, requiring the parameter to be a package and not simply a module.

Is this limitation intentional? If so, the migration docs should include a note about this incompatibility.

@jaraco jaraco added api wontfix This will not be worked on labels Oct 21, 2020
@jaraco jaraco added this to the post-1.0 milestone Oct 21, 2020
@jaraco
Copy link
Member Author

jaraco commented Oct 21, 2020

In GitLab by @jaraco on Jun 3, 2018, 13:53

I note that __package__ works on later Pythons, but not on Python 2.7.

@jaraco
Copy link
Member Author

jaraco commented Oct 21, 2020

In GitLab by @warsaw on Jun 4, 2018, 21:00

Is there something weird about queso.validator.tests.test_options? Can you reproduce this with something in the Python stdlib? This works for me:

Python 3.6.5+ (heads/3.6:b0ca398cab, Apr 24 2018, 18:02:36) 
[GCC 4.2.1 Compatible Apple LLVM 9.1.0 (clang-902.0.39.1)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import test.support
>>> test.support.__spec__.submodule_search_locations
['/usr/local/lib/python3.6/test/support']
>>> test.support.__name__
'test.support'
>>> from importlib_resources import *
>>> x = read_binary(test.support.__name__, '__init__.py')
>>> 

Is queso.validator.tests.test_options a non-package module? In that case, yes it's expected behavior:

>>> import importlib_resources._py3
>>> x = read_binary(importlib_resources._py3.__name__, '__init__.py')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/barry/projects/implib/resources/importlib_resources/_py3.py", line 142, in read_binary
    package = _get_package(package)
  File "/Users/barry/projects/implib/resources/importlib_resources/_py3.py", line 40, in _get_package
    raise TypeError('{!r} is not a package'.format(package))
TypeError: 'importlib_resources._py3' is not a package
>>> 

Is pkg_resources using the parent package containing the module? This would be the case regardless of whether you're passing in the module name via __name__ or the module object.

@jaraco
Copy link
Member Author

jaraco commented Oct 21, 2020

In GitLab by @warsaw on Jun 4, 2018, 21:01

assigned to @warsaw

@jaraco
Copy link
Member Author

jaraco commented Oct 21, 2020

In GitLab by @warsaw on Sep 7, 2018, 19:20

Getting back to this, I guess pkg_resources finds the parent package of the non-package module? Is this behavior we want to preserve? I'd rather not, but if this use case is pervasive then maybe we should. @brettcannon what do you think?

@jaraco
Copy link
Member Author

jaraco commented Oct 21, 2020

In GitLab by @jaraco on Sep 11, 2018, 09:03

Yes, queso.validator.tests.test_options is a non-package module (it's a module in a package). pkg_resources does resolve this (by way of dirname(__file__)).

I suggest that since __name__ can't be used in an arbitrary module (to find resources in the package containing that module), this project should instead recommend use of __package__, but should provide a compatibility shim for Python 2, such that one could write:

read_binary(importlib_resources.package(globals()), filename)

(or similar) to resolve the package of a module.

Curiously, when I test on Python 2 now, I see that __package__ is available for queso.validator.tests.test_options, so it seems that maybe recent versions of Python 2.7 added support for this property?

Indeed, just going back to Python 2.7.12, I see __package__ is defined, but None:

jaraco@dev-jaraco:~$ mkdir foo
jaraco@dev-jaraco:~$ touch foo/__init__.py
jaraco@dev-jaraco:~$ touch foo/bar.py
jaraco@dev-jaraco:~$ python2.7
Python 2.7.12 (default, Nov 20 2017, 18:23:56)
[GCC 5.4.0 20160609] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import foo.bar
>>> foo.bar.__package__
>>>

@jaraco
Copy link
Member Author

jaraco commented Oct 21, 2020

In GitLab by @brettcannon on Sep 11, 2018, 18:21

So technically, in Python 3 this is as simple as using __spec__.parent as that is the same as __spec__.name for packages and is set to the containing package for modules (it's "" for top-level modules).

As for API change to support this, I'm -0. It's a small nicety, but I would rather people be explicit with what they are anchoring off of rather than calculate it implicitly because someone happened to pass in a submodule. It also alleviates the very minor worry of someone who converted a package to a module and then gets bit by that shift.

@jaraco
Copy link
Member Author

jaraco commented Oct 21, 2020

In GitLab by @jaraco on Sep 11, 2018, 22:13

I'm struggling to devise a straightforward way to replace the pkg_resources code for this use-case.

So the recommendation is for a module that wishes to load resources from the package containing that module that they write:

importlib_resources.read_binary(__spec__.parent if sys.version_info[0] != 2 else __name__ if __import__('inspect').ismodule(__import__(__name__)) else __name__.rpartition('.')[0], filename)

That seems somehow less preferable to the code this intends to replace:

pkg_resources.resource_stream(__name__, filename).read()

Perhaps what you're suggesting is that loading a resource in one's package should not be allowed unless the developer explicitly type the package name. Obviously that's possible, but it does mean that only Python 3 users get the nice experience. If __spec__.parent works on Python 3, it sure would be nice if there was a helper to resolve a comparable value on Python 2.

I still don't see why a helper function couldn't provide a compatibility shim:

def package_for(globals):
    """
    Given a module's globals, return its package.
    """
    try:
        return globals['__spec__'].parent
    except KeyError:
        pass
    is_module = inspect.ismodule(__import__(globals['__name__']))
    return globals['__name__'] if is_module else globals['__name__'].rpartition('.')[0]

Or at least provide the Python 2 logic for resolving a package from a module, so one could at least write:

package_name = __spec__.parent if sys.version_info[0] != 2 else package_for(globals())
importlib_resources.read_binary(package_name, filename)

Note, I'm not suggesting changing the API for importlib_resources.read_binary (or its comparable functions), only to provide a means to resolve a package from a module.

It seems unnecessarily constraining and brittle to require a module to name its package by name (and then require that to be kept in sync). It seems comparable to only allowing absolute imports and never relative imports.

IMO, the resource API should provide for resources relative to a module and not leave it to each module to determine/declare in which package it is contained.

If you're not convinced, I'll drop it and focus on the documentation that clarifies this pitfall.

@jaraco
Copy link
Member Author

jaraco commented Oct 21, 2020

In GitLab by @warsaw on Sep 12, 2018, 13:02

Why wouldn't it just be importlib_resources.read_binary(module.__package__, filename) in both Python 2 and 3? E.g.

Python 2.7.15 (default, Jun 17 2018, 12:46:58) 
[GCC 4.2.1 Compatible Apple LLVM 9.1.0 (clang-902.0.39.2)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> from importlib_resources import read_text
>>> import email.utils
>>> source = read_text(email.utils.__package__, 'utils.py')
>>> len(source)
10026
ython 3.6.6+ (heads/3.6:b1707abd74, Jun 30 2018, 13:04:54) 
[GCC 4.2.1 Compatible Apple LLVM 9.1.0 (clang-902.0.39.2)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> from importlib_resources import read_text
>>> import email.utils
>>> source = read_text(email.utils.__package__, 'utils.py')
>>> len(source)
13897

@jaraco
Copy link
Member Author

jaraco commented Oct 21, 2020

In GitLab by @jaraco on Sep 12, 2018, 13:03

See above:

Indeed, just going back to Python 2.7.12, I see __package__ is defined, but None

@jaraco
Copy link
Member Author

jaraco commented Oct 21, 2020

In GitLab by @warsaw on Sep 12, 2018, 13:26

WTF? @brettcannon just tried it with a bespoke package layout and you're right, it doesn't work for 2.7.15 in that case. So why does it work for email.utils?

Aside from that, here's the philosophical question. If we add support for this as a nod to the mission of this library to replace pkg_resources (i.e. having this will make it easier to port away from the latter), then it seems more reasonable to support #58 as well, since that's a pain point for porters too. I've said over there that I'm conditionally in favor of that if we get a reasonable looking contribution.

So, Brett and I muse that if you were by chance to submit a PR for that and it looked okay, we'd be cool with accepting this one too. But in that case, I think allowing modules (objects and names) in the package argument would be better than a convenience function since it's a more direct compatibility compromise.

We'd need to update the documentation too, but I don't think we'd need to change the type signatures. This and #58 would require a major version bump IMO, and I would be disinclined to backport these changes to Python 3.7.

@jaraco
Copy link
Member Author

jaraco commented Oct 21, 2020

In GitLab by @jaraco on Oct 31, 2018, 15:02

As I think about this more, I'm leaning toward not proposing a change. I'm particularly reluctant to make the interface(s) less precise, especially since the expanded interface would encourage the anti-pattern (using __name__ instead of __package__). I think the alternative (requiring an explicit provision of the package name) is preferable for packages that need to support older Pythons (prior to 2.7.15).

@jaraco
Copy link
Member Author

jaraco commented Oct 21, 2020

In GitLab by @jaraco on Oct 31, 2018, 15:02

closed

@jaraco
Copy link
Member Author

jaraco commented Oct 21, 2020

In GitLab by @warsaw on Nov 1, 2018, 14:13

Thanks @jaraco

@jaraco jaraco closed this as completed Oct 21, 2020
jaraco added a commit that referenced this issue Jun 16, 2022
* Update .coveragerc

* Keep whitespace consistent.

Co-authored-by: Jason R. Coombs <jaraco@jaraco.com>
musicinmybrain added a commit to musicinmybrain/pint that referenced this issue Oct 30, 2023
Replace pkg_resources.resource_filename with
importlib.resources.as_file. This removes an implicit dependency on
setuptools (to which pkg_resources belongs); furthermore, the entire
pkg_resources API is deprecated.

Regarding the switch from __file__ to __package__, see:
python/importlib_resources#60
musicinmybrain added a commit to musicinmybrain/pint that referenced this issue Oct 31, 2023
Replace pkg_resources.resource_filename with importlib.resources.files.
This removes an implicit dependency on setuptools (to which
pkg_resources belongs); furthermore, the entire pkg_resources API is
deprecated.

Regarding the switch from __file__ to __package__, see:
python/importlib_resources#60
hgrecco pushed a commit to hgrecco/pint that referenced this issue Dec 3, 2023
Replace pkg_resources.resource_filename with importlib.resources.files.
This removes an implicit dependency on setuptools (to which
pkg_resources belongs); furthermore, the entire pkg_resources API is
deprecated.

Regarding the switch from __file__ to __package__, see:
python/importlib_resources#60
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

1 participant