Skip to content

PackageLoader doesn't depend on setuptools#1082

Merged
davidism merged 1 commit intomasterfrom
pkgutil-loader
Oct 17, 2019
Merged

PackageLoader doesn't depend on setuptools#1082
davidism merged 1 commit intomasterfrom
pkgutil-loader

Conversation

@davidism
Copy link
Copy Markdown
Member

@davidism davidism commented Oct 16, 2019

closes #970
closes #976

Based on pallets/werkzeug#1647. Uses pkgutil.get_loader instead of pkg_resources. pkg_resources required an implicit dependency on setuptools, and was is slow to import.

importlib.resources / importlib_resources makes some unhelpful decisions, such as requiring every directory to have a __init__.py file, and only loading one level deep. Python 3's loader.get_resource_reader() API suffers the same issue for the most part. Python's resource API in general is still too limited for us to support arbitrary types of installations, so only directories and zip files are supported, and it's mostly done manually rather than with the loader/reader API.

@davidism davidism added this to the 2.11.0 milestone Oct 16, 2019
@davidism
Copy link
Copy Markdown
Member Author

davidism commented Oct 16, 2019

PyPy's zipimporter doesn't have the _files attribute. 😞 I think I just won't return the template list in that case, rather than trying to traverse the zip file manually.

Implementation on Windows has some path issues, I'll have to address those once I can boot into Windows.

@gsemet
Copy link
Copy Markdown

gsemet commented Oct 16, 2019

I am not sure this method works with pyinstaller's way of packaging jinja, while mine does :)

@davidism
Copy link
Copy Markdown
Member Author

importlib.resources is a non-starter as long as it requires __init__.py in every directory and doesn't support directory traversal.

@davidism
Copy link
Copy Markdown
Member Author

davidism commented Oct 16, 2019

I'm assuming you mean pyinstaller's way of packaging package data, not packaging Jinja, since how Jinja is packaged shouldn't affect how other things are loaded. I'm not familiar with PyInstaller, if you have a simple example of a package with a templates directory, I can take a look.

@gsemet
Copy link
Copy Markdown

gsemet commented Oct 16, 2019

that's not something very problematic, you know, it is just a file to add. And I recommend letting people decide which method is the best for them. If you need to package jinja in pyinstaller like i do, I am pretty happy with this init.py (it is actually not the more weird thing to do). At least I have the option to choose this loader if I want to.
But it you find a better way to have jinja happy in pyinstaller or similar packaging, I'll be happy to switch

@gsemet
Copy link
Copy Markdown

gsemet commented Oct 16, 2019

I'm assuming you mean pyinstaller's way of packaging package data, not packaging Jinja, since how Jinja is packaged does not affect how other things are loaded.

Yes, packaging jinja and a template folder as data inside a pyinstaller executable, so that this executable can render the template. I use importlib resource and yes, it's tricky to do, but at least it works

@davidism
Copy link
Copy Markdown
Member Author

If you need a specific loader for your project, you can always add a loader, we don't need to cover every possible case in the built-in loaders, that's the whole point of it being an interface.

@davidism
Copy link
Copy Markdown
Member Author

davidism commented Oct 17, 2019

Just checked, PackageLoader with pkg_resources doesn't seem to support PyInstaller's FrozenImporter, so I'm not treating compatibility with PyInstaller as a supported feature.

However, this new implementation does support PyInstaller (unintentionally).

$ tree example/
example
├── __init__.py
└── templates
    └── test.txt
$ pyinstaller -F --name example --add-data example/templates:example/templates entry.py
$ ./dist/example
Hello

-   1
-   2
-   3

World

@gsemet
Copy link
Copy Markdown

gsemet commented Oct 17, 2019

Great, so it's even simpler that mine :) Will check on it once released !

@ghost
Copy link
Copy Markdown

ghost commented Oct 18, 2019

will this be part of the upcoming release?

jmbowman added a commit to openedx/openedx-platform that referenced this pull request Dec 30, 2019
Fix the issue that was preventing us from upgrading pytest.  pytest does some manipulation of test packages that prevents `pkg_resources` from loading resources from them, but used to contain a workaround for the problem.  That workaround was [removed](pytest-dev/pytest#5392) in 4.6.0 as a performance enhancement when pytest switched from `pkg_resources` to `importlib-metadata` for its own entrypoint handling.  This tripped up one of our test modules which defined classes that loaded templates from inside a test package.  Moving these resources to the parent package fixes the problem.

More and more, `pkg_resources` is being abandoned in favor of `importlib-metadata` and `importlib_resources` as they have a simpler design with much better performance.  However, `importlib_resources` doesn't support loading files from any directory which isn't itself a Python package (and doesn't allow direct use of paths including directories within the package).  Jinja2 chose a [different approach](pallets/jinja#1082) that we may want to emulate in our resource handling.

Also fixed usage of a removed `pytest.raises()` parameter and a bug in our configuration of the `common/lib` tests that became a problem after the upgrade.
@davidism
Copy link
Copy Markdown
Member Author

Reverting this in 2.11.2, it broke compatibility with Pytest. However, support will be present when 3.0 is released. See #1168, #1169

@github-actions github-actions Bot locked as resolved and limited conversation to collaborators Nov 13, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Replace pkg_resource by importlib_resources

2 participants