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

Change find_module to find_spec for py37 compat #1563

Merged
merged 7 commits into from Mar 21, 2020
Merged

Conversation

alhirzel
Copy link
Contributor

@alhirzel alhirzel commented Oct 29, 2018

Summary of changes

Change the call of find_module to find_spec.

Attempts to fix Ron89/thesaurus_query.vim#33 and analogous issue tools-life/taskwiki#183, probably others brought about by vim/vim@79a494d5.

Pull Request Checklist

  • Changes have tests
  • News fragment added in changelog.d. See documentation for details
  • Review - PR author is not sure what this will break or how to write a test for it!

Copy link
Member

@pganssle pganssle left a comment

I have some problems with the implementation here, but I'm also not entirely sure I understand the problem. pkgutil.get_importer says that it returns a finder, and the abstract base class importlib.Finder has a find_module as an abstract method. Can you provide a minimal example of some code that should work but doesn't because of this issue?

@@ -2098,8 +2098,13 @@ def _handle_ns(packageName, path_item):
# capture warnings due to #1111
with warnings.catch_warnings():
Copy link
Member

@pganssle pganssle Oct 29, 2018

Choose a reason for hiding this comment

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

Do we still need this warning with importer.find_spec, or does find_spec not trigger the behavior from #1111?

Copy link
Contributor Author

@alhirzel alhirzel Oct 29, 2018

Choose a reason for hiding this comment

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

@jaraco could you comment? I was never affected by #1111 and have not reproduced it. My intent with this change was minimal impact to existing behavior.

Copy link
Contributor Author

@alhirzel alhirzel Nov 10, 2018

Choose a reason for hiding this comment

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

I tried disabling this warning and did not see any ill effects. I do not know if this warning-kill needs to be kept, but I narrowed its scope in the latest commit.

try:
loader = importer.find_spec(packageName).loader
except:
loader = None
Copy link
Member

@pganssle pganssle Oct 29, 2018

Choose a reason for hiding this comment

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

I don't like this try/except. Why is it done?

If it's because find_spec raises an error instead of returning None in the case that importer.find_module would normally return None, please narrow the exception catching to the specific exception raised in that instance. If it's just to avoid ever seeing an exception, drop this entirely - I'm not at all comfortable swallowing exceptions.

Copy link
Contributor Author

@alhirzel alhirzel Nov 10, 2018

Choose a reason for hiding this comment

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

I narrowed the exceptions to AttributeError catches in the latest commit. This dodges a "find_spec" in dir(...) check, which is an alternative if you'd prefer to avoid try/except entirely. Let me know if this addresses your concern.

@alhirzel
Copy link
Contributor Author

@alhirzel alhirzel commented Oct 29, 2018

The issue is most easily seen by stepping thru Ron89/thesaurus_query.vim#33, where :py3 import pkg_resources is run in a recent Vim vs IPython. I think the issue is because vim is not a real "module" in a base Python environment, but rather it is provided by the embedding environment that Vim creates for its plugins. I think Vim's change in vim/vim@79a494d caused this behavior to trigger (their change was non-backwards-compatible).

The spirit of my patch is to allow both of these cases to work. I agree the try/catch are rather loose, I will look later into how to narrow these checks. Thank you for the feedback.

@pganssle
Copy link
Member

@pganssle pganssle commented Oct 29, 2018

@alhirzel I don't know if this is something that needs to be fixed here or in vim. From a cursory reading, it seems like vim is failing to implement the Finder interface. This is why I asked for a minimal working example. For more information about why this is necessary and how to craft minimal working examples, see Matt Rocklin's blog post that I linked above, or Stack Overflow's guide to minimal, complete and verifiable examples (which Matt links to).

A simple minimal test case that should work but doesn't because of this issue, and contains no other distractions that may be causing the problem, is what we're looking for in terms of determining how to proceed.

@alhirzel
Copy link
Contributor Author

@alhirzel alhirzel commented Oct 29, 2018

I think around Python 3.4, find_module (i.e. PEP 302) became depreciated by PEP 451. I think on some level, Vim's implementation may be clean on 451? Curious where you think the change should be. I will do more digging, including producing a vim-less MWE if I am able.

@pganssle
Copy link
Member

@pganssle pganssle commented Oct 29, 2018

@alhirzel I do not see anything in PEP 451 about find_module being deprecated, in fact the backwards compatibility section specifically mentions find_module several times.

In any case, if we do want to migrate off of using find_module, it should work the other way around, where find_spec (or, apparently, find_loader) is called first, and pkg_resources falls back to find_module. That said, I am not amazingly familiar with this particular code path, so I can't tell you what the consequences of this could be.

In any case, we'll need a minimal working example here anyway in order to write a failing test. I'm inclined to say that in this case, vim failed to implement the Finder interface, which is a bug in vim, not in pkg_resources, but I imagine we can look at PEP 451's discussion of the deficiencies of find_module to come up with a minimal test case that cannot be solved with the find_module approach.

@alhirzel
Copy link
Contributor Author

@alhirzel alhirzel commented Oct 29, 2018

@pganssle agreed on all points. @brammool thoughts?

@brammool
Copy link

@brammool brammool commented Oct 29, 2018

Eh, why are you asking me?

@pganssle
Copy link
Member

@pganssle pganssle commented Oct 29, 2018

@brammool I think because you made this commit on vim, which caused the issue that @alhirzel is trying to fix.

That said, I cant be sure it's a bug in vim because I don't understand vim's codebase. It may be worth moving the discussion onto the vim issue tracker?

@alhirzel
Copy link
Contributor Author

@alhirzel alhirzel commented Oct 31, 2018

My MNWE right now is vim -c ":py3 import pkg_resources". After realizing vim's presence in sys.path confused me, i.e.

['/usr/lib/python37.zip', '/usr/lib/python3.7', '/usr/lib/python3.7/lib-dynload',
 '/usr/lib/python3.7/site-packages', '_vim_path_']

I dug into Vim's source and found if_py_both.h:vim_special_path (decl, usage) and the documentation on how the loader works.

My open questions right now:

  • Why is the magic string "_vim_path_" present in sys.path when pkg_resources gets imported? It seems like an import dance, wondering if this old-ish code is part of a band-aid tower? @brammool could you shed some light? I am not an expert in this ecosystem and could use your help. If you committed someone else's work, I wasn't able to see that on the git blame.
  • In the sense of spec, is the bug in pkg_resources, vim or somewhere else, or no bug? I don't want to spam setuptools if this isn't their issue, but I also want to unbreak things.
  • How can I condense the Python pseudocode into an MNWE against setuptools, if appropriate?

@alhirzel
Copy link
Contributor Author

@alhirzel alhirzel commented Nov 9, 2018

No longer reproducible on new install of Arch Linux with same software versions. Something spooky must have been happening. closing PR

@alhirzel alhirzel closed this Nov 9, 2018
@fanovpn
Copy link

@fanovpn fanovpn commented Nov 9, 2018

@alhirzel Do you have any packages installed with 'namespace_packages.txt' files in your new install, to trigger the code path that calls find_module? If I install vim + python3 + python-setuptools in an 'archlinux/base' docker container, I see no problem with the command line vim --clean -c ':py3 import pkg_resources', however if I install e.g. python-matplotlib or python-protobuf the attempt to call vim.find_module then occurs and fails when 'import pkg_resources' is attempted by vim.

@alhirzel
Copy link
Contributor Author

@alhirzel alhirzel commented Nov 9, 2018

The old install contained two packages in py3 with namespace_packages.txt included:

/usr/lib/python3.7/site-packages/matplotlib-3.0.1-py3.7.egg-info/namespace_packages.txt
/usr/lib/python3.7/site-packages/zope.interface-4.6.0-py3.7.egg-info/namespace_packages.txt

The new install had none. I installed matplotlib and the issue returned. Thank you @fanovpn.

@alhirzel
Copy link
Contributor Author

@alhirzel alhirzel commented Nov 10, 2018

latest commit addresses @pganssle comments

@alhirzel alhirzel reopened this Nov 10, 2018
@pganssle
Copy link
Member

@pganssle pganssle commented Nov 10, 2018

@alhirzel Thank you for updating your PR. Please note that I am still not entirely sure if this is fixing a real problem, though I'm willing to be convinced that it is. I think the best way to get this accepted is to create a minimal example of how pkg_resources fails on one of the edge cases that PEP 451 was created to solve.

It also needs tests, but if the minimal example takes the form of a failing test, that's two birds with one stone.

@blastmaster
Copy link

@blastmaster blastmaster commented Feb 6, 2019

Hi, what is the state of this issue? I also encounter the problem when using vim together with python-3.7 and I can confirm what @fanovpn says. My question is, is this a bug in setuptools? Or is it a vim bug?
First it looks like a vim bug for me.
But as @fanovpn says, it will just be triggered if at least one python module in site-packages has a namespace_packages.txt file in it's egg-info.
Can someone help answering the question?

@pganssle
Copy link
Member

@pganssle pganssle commented Feb 6, 2019

@blastmaster The current state is this:

I think the best way to get this accepted is to create a minimal example of how pkg_resources fails on one of the edge cases that PEP 451 was created to solve.

If you can do this, it will make the PR much easier to review.

@joelfrederico
Copy link

@joelfrederico joelfrederico commented Feb 14, 2019

It looks like two people were super-close. @alhirzel said:

I think around Python 3.4, find_module (i.e. PEP 302) became depreciated by PEP 451. I think on some level, Vim's implementation may be clean on 451? Curious where you think the change should be. I will do more digging, including producing a vim-less MWE if I am able.

This isn't true. Vim moved a bit too fast and instead of deprecating find_module, it removed its support. If you look at vim, find_module is conditional on Python version. So they're not meeting PEP 451.

@pganssle said:

@alhirzel I do not see anything in PEP 451 about find_module being deprecated, in fact the backwards compatibility section specifically mentions find_module several times.

If you look in PEP 451, it is actually deprecated:

Deprecations

  • importlib.abc.MetaPathFinder.find_module()
  • importlib.abc.PathEntryFinder.find_module()

So this is both Vim and setuptool's fault: setuptools is using a deprecated API (the lesser of the faults) and Vim removed a deprecated API (the greater of the faults).

I think I can fix Vim's regression really easily: just move the bit about find_module so that it's not conditional on the python version.

It looks like this commit prefers find_spec over find_module, so at the worst it's forward-looking.

I may issue a PR on Vim in a bit, I need dinner...

@pganssle
Copy link
Member

@pganssle pganssle commented Feb 14, 2019

@joelfrederico Thanks for digging into that. I agree that if it's deprecated we should probably change it.

This PR still definitely needs tests, we've been bitten before by seemingly simple changes to this kind of thing. Ideally we'd still have what I asked for above - some examples of edge cases that the new version solves - so that we can write tests around them to prevent regressions.

I'm also worried that the semantics of this new mechanism seem different, so at least I'll need to understand how it works a lot better before merging any change. Separate PRs with tests (possibly extracted from real-life uses of register_namespace_handler) would be more than welcome.

@joelfrederico
Copy link

@joelfrederico joelfrederico commented Feb 14, 2019

I have a workaround! Before importing other modules, you can monkey-patch the vim module:

import vim
from importlib.machinery import PathFinder as _PathFinder
vim.find_module = _PathFinder.find_module

It's not ideal, but it works for simple cases. It probably messes up importing the vim module in some circumstances, and still needs to be fixed at any rate.

@joelfrederico
Copy link

@joelfrederico joelfrederico commented Feb 14, 2019

@pganssle Thanks!

Yeah, it's a completely new mechanism. Probably needs a completely new set of unit tests, not just one or two. I don't really know how setuptools is using the old mechanism. This is why things are deprecated IMHO: to give others (like setuptools) time to figure these problems out. Hopefully it can be done before find_module transitions from deprecated to removed!

@alhirzel
Copy link
Contributor Author

@alhirzel alhirzel commented Feb 26, 2019

I don't understand enough of the Python package ecosystem to issue a more insightful patch or write appropriate tests. It would be best if someone else took the wheel on this.

diraol added a commit to diraol/python-mode that referenced this issue May 11, 2019
diraol added a commit to diraol/python-mode that referenced this issue May 11, 2019
@jaraco jaraco merged commit 17dde19 into pypa:master Mar 21, 2020
19 of 20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants