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

Let -2 argument dest match --python-version's #5619

Merged
merged 1 commit into from
Sep 18, 2018
Merged

Let -2 argument dest match --python-version's #5619

merged 1 commit into from
Sep 18, 2018

Conversation

sqwishy
Copy link
Contributor

@sqwishy sqwishy commented Sep 14, 2018

Hi. This addresses issue #5576. The pull request #5578 had stuff in it that broke things so this is a smaller change.

Currently, mypy -2 behaves differently from mypy --python-version 2.7 in that only the latter will look for typing information in the Python 2 site packages. When -2 only is passed it ends up setting a value on a different namespace. With this change, the value is set up the same namespace and attribute that --python-version sets. (See also #5576 (comment))

There is still this inconsistency when loading the python version from the ini file, this will have to be addressed in another issue and pull request since the desired behaviour is unclear.

Copy link
Collaborator

@ethanhs ethanhs left a comment

Choose a reason for hiding this comment

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

Thank you for splitting these changes out, LGTM.

Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

LGTM too.

@gvanrossum
Copy link
Member

There is still this inconsistency when loading the python version from the ini file, this will have to be addressed in another issue and pull request since the desired behaviour is unclear.

I'm not sure what's unclear about the desired behavior? I'd think that it should make no difference whether you pass -2 or --py2 or --python-version 2.7 or use the ini file to set python_version = 2.7. I don't recall ever desiring a difference nor do I think there ever was a documented difference. What am I missing?

@ethanhs
Copy link
Collaborator

ethanhs commented Sep 17, 2018

I'm not sure what's unclear about the desired behavior? I'd think that it should make no difference whether you pass -2 or --py2 or --python-version 2.7 or use the ini file to set python_version = 2.7. I don't recall ever desiring a difference nor do I think there ever was a documented difference. What am I missing?

I was about to say in #5620 essentially that the behavior of these probably should not be different.

@Michael0x2a
Copy link
Collaborator

@gvanrossum -- I might be misunderstanding, but I think the issue was that we were accidentally relying on the old, inconsistent behavior. For example, in CI, we install only Python 3 but type-check Python 2 code. If we were to make the config file behave in exactly the same way as --python-version = 2.7, that would make CI start looking for a Python 2 installation, which would then crash.

Or to put it another way, this inconsistency is apparently the only available mechanism for type-checking Python 2 code using Python 3 site-packages currently available.

One possible idea that's being floated around in #5620 is to relax some of our checks so that python_version and python_executable can refer to different Python versions. This would give us a alternative mechanism we could use so we no longer need to rely on the inconsistency to accomplish the above.

I guess the main open question here is whether or not setting python_version to Python 2.7 install and pointing python_executable at some Python 3 install could cause some sort of weirdness. Presumably, the answer is "no, it doesn't matter" since we were basically doing this all along -- but I'm not really sure.

@gvanrossum
Copy link
Member

in CI, we install only Python 3 but type-check Python 2 code. If we were to make the config file behave in exactly the same way as --python-version = 2.7, that would make CI start looking for a Python 2 installation, which would then crash.

Interesting. I had assumed that the way to skip looking for a Python 2 installation would be to set site_packages = False, but that flag doesn't exist for the config file -- it only exists for the command line (--no-site-packages). This seems to be an omission in the PEP 561 implementation.

I see it as a pretty fundamental contract that every flag or option can be specified either on the command line or in the config file with exactly the same semantics. (Apart from that the command line can override global config file flags, and the config file also supports per-module flags.)

@ethanhs Is there something I am missing here? Is there a deeper reason some of the PEP 561 behavior can only be done through the command line?

In the meantime I'll merge this tiny change.

@gvanrossum gvanrossum merged commit 5751f42 into python:master Sep 18, 2018
@ethanhs
Copy link
Collaborator

ethanhs commented Sep 18, 2018

@gvanrossum I believe that was merely an oversight on my part.

@gvanrossum
Copy link
Member

OK, I filed #5630 to track this.

@sqwishy
Copy link
Contributor Author

sqwishy commented Sep 18, 2018

@gvanrossum I had created issue #5620 for that earlier. So you may probably want to close it in favour of your new issue. Sorry for the confusion.

@ilevkivskyi
Copy link
Member

@gvanrossum

Interesting. I had assumed that the way to skip looking for a Python 2 installation would be to set site_packages = False, but that flag doesn't exist for the config file -- it only exists for the command line (--no-site-packages). This seems to be an omission in the PEP 561 implementation.

This will not solve all problems, because we actually do need to search Python 3 site packages, where PEP 561 packages are installed. So we can't set this flag. A solution proposed by @Michael0x2a is to allow python_version be different from python_executable.

@gvanrossum
Copy link
Member

This will not solve all problems, because we actually do need to search Python 3 site packages, where PEP 561 packages are installed. So we can't set this flag.

I guess I am not familiar with the constraints in the CI job we're talking about. Apparently:

  • we need to type-check Python 2 code
  • for this we need some stub packages installed (which?)
  • it's impossible to install Python 2
  • there are no significant differences between the Python 2 and 3 stubs in this case
  • so the only solution is to install those stub packages in the Python 3 site-packages folder and rely on the behavior where we initialize the search path for the stubs based on the defaults for the command line (i.e. 3), and then the Python version is set to 2 by mypy.ini?

This sounds really wrong. I'm not familiar with where in our (?) CI this is happening -- can someone show me the code where this is happening? (Not the code that searches for the stubs, but the part of the CI that relies on this behavior.)

A solution proposed by @Michael0x2a is to allow python_version be different from python_executable.

TBH I'm not against that if it's the only solution, but I'm not convinced it is yet.

@JukkaL
Copy link
Collaborator

JukkaL commented Sep 18, 2018

This sounds really wrong. I'm not familiar with where in our (?) CI this is happening -- can someone show me the code where this is happening?

I think that this happens with internal Dropbox mypy run scripts. We install SQLAlchemy stubs in the Python 3 virtualenv that we use to run mypy. We don't use a Python 2 virtualenv (neither locally nor in CI). We process the stubs in Python 2 mode even if they are installed in a Python 3 virtualenv, and there aren't separate stubs for Python 2 and 3. I'm not sure if this is a good way to set things up, but I believe that this is how things currently work.

@gvanrossum
Copy link
Member

I see. We should really fix this by also creating a Py2 venv and installing the stubs there, then pointing mypy to that Py2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants