Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Move from pkg_resources to importlib.resources #12542

Closed

Conversation

vheuken
Copy link

@vheuken vheuken commented Apr 25, 2022

Resolves #12508

The issue also states that setuptools needs to be removed as a dev dependency. However, in tox.ini, there's comments explaining that it is required by another library. Is this comment outdated?

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
    • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
  • Pull request includes a sign off
  • Code style is correct
    (run the linters)

@vheuken vheuken requested a review from a team as a code owner April 25, 2022 17:42
Signed-off-by: Vincent Heuken <[email protected]>
@DMRobertson
Copy link
Contributor

The issue also states that setuptools needs to be removed as a dev dependency. However, in tox.ini, there's comments explaining that it is required by another library. Is this comment outdated?

That comment refers to types-setuptools, from here:

types-setuptools = ">=57.4.0"

But don't worry about that for now: the synapse team can handle that separately. As for tox, tox.ini is somewhat redundant after #11537. Again, not something to worry about for this PR.

@DMRobertson
Copy link
Contributor

Many thanks for taking this on @vheuken!

Some of the tests are failing on older versions of Python. This is because the porting instructions assume you're using a new version of importlib.resources which has the files() function. Unfortunately that was only added in Python 3.9. Sorry I hadn't anticipated this!

Fixing this requires a little more work. I think the easiest thing to do would be to try and use the functions in older versions of importlib.resources instead of files(). See here, and look for the ones that don't say "added in version 3.9".

Does that make sense? Do you think you'd be able to try that? Feel free to fire off any questions, either way.

@DMRobertson DMRobertson added the X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged label Apr 25, 2022
@DMRobertson DMRobertson self-assigned this Apr 25, 2022
Copy link
Contributor

@DMRobertson DMRobertson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Oops, I should have written that comment as a review, to take it out of Synapse's review queue.)

synapse/config/oembed.py Outdated Show resolved Hide resolved
@vheuken
Copy link
Author

vheuken commented Apr 26, 2022

@DMRobertson Thanks for that. I opted to use importlib.resources.path, which fixed the integration tests failing, but now it looks like it fails with versions of Python older than 3.10. I'll try to get 3.9 on my local machine and test against both versions before pushing again.

@vheuken vheuken marked this pull request as draft April 26, 2022 19:15
@DMRobertson
Copy link
Contributor

We discussed this in #synapse-dev and came to the conclusion that the change described in #12508 didn't seem to be working well in practice.

  • Synapse assumes that its resources are available in a directory on the filesystem.
  • importlib.resources emphasises that packages' resources may not be available as files on disk. E.g. they might be compressed in a zip, or fetched on-demand over the network somehow.
  • For that reason it's awkward to get our hands on the path to the templates directory in the way that Synapse currently does. It felt like we were fighting importlib.resources!
  • importlib.resources.files seemed to work okay, but isn't present in older pythons. We'd need to use the backport importlib_resources---but that doesn't seem to be worth the effort given the friction.

Given all that, I think it's best to close this PR and the original issue too.

@vheuken thanks again for your efforts here---you've uncovered something that we hadn't grasped at first!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
X-Awaiting-Changes A contributed PR which needs changes and re-review before it can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move from pkg_resources to importlib.resources
3 participants