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

Better import for sometimes failing mypy test #4016

Merged
merged 2 commits into from
Apr 13, 2021

Conversation

mpharrigan
Copy link
Collaborator

Would get:

dev_tools/bash_scripts_test.py:33: error: Name
'_pytest.tmpdir.TempdirFactory' is not defined

This fixes it. Don't know why it sometimes works on
some configurations.

Would get:

dev_tools/bash_scripts_test.py:33: error: Name
'_pytest.tmpdir.TempdirFactory' is not defined

This fixes it. Don't know why it sometimes works on
some configurations.
@mpharrigan mpharrigan requested review from cduck, vtomole and a team as code owners April 12, 2021 22:08
@google-cla google-cla bot added the cla: yes Makes googlebot stop complaining. label Apr 12, 2021
@@ -18,7 +18,7 @@
from dev_tools import shell_tools

if TYPE_CHECKING:
import _pytest
import _pytest.tmpdir
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you have a sense for why this works? I don't really understand the root problem, even - my inuition is that import _pytest should give access to _pytest.tmpdir.TempdirFactory in the original code.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In python, import package does not give you availability to package.subpackage. But sometimes people like us put imports into the top-level __init__.py.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

maybe there are different versions of pytest (_pytest?) around.

Copy link
Contributor

@balopat balopat Apr 13, 2021

Choose a reason for hiding this comment

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

The _pytest package does not have tmpdir attribute and it never had (based on its history): https://github.com/pytest-dev/pytest/blob/main/src/_pytest/__init__.py

Thus, this should never work in mypy...but sometimes it does! :)

>>> import _pytest
>>> _pytest.tmpdir
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: module '_pytest' has no attribute 'tmpdir'
>>> import _pytest.tmpdir
>>> _pytest.tmpdir
<module '_pytest.tmpdir' from '/Users/balintp/.virtualenvs/cirq37/lib/python3.7/site-packages/_pytest/tmpdir.py'>

I can confirm that this fixes the mypy issue on my 3.7 mac env where I could consistently reproduce the error report.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

_pytest.* is listed in mypy.ini as follow_imports=silent and ignore_missing_imports=true. I don't want to actually do any sleuthing but it sounds relevant and could have been a corner case whose behavior has changed between versions.

Copy link
Contributor

@balopat balopat left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!!!

@balopat balopat added the automerge Tells CirqBot to sync and merge this PR. (If it's running.) label Apr 13, 2021
@CirqBot CirqBot added the front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. label Apr 13, 2021
@CirqBot CirqBot merged commit 76edeb1 into quantumlib:master Apr 13, 2021
@CirqBot CirqBot removed automerge Tells CirqBot to sync and merge this PR. (If it's running.) front_of_queue_automerge CirqBot uses this label to indicate (and remember) what's being merged next. labels Apr 13, 2021
@mpharrigan
Copy link
Collaborator Author

This change got reverted somehow

@95-martin-orion
Copy link
Collaborator

95-martin-orion commented Apr 21, 2021

This change got reverted somehow

EDIT: disregard this comment, I was looking at the wrong fork :p

Not only that, this file got reverted all the way to its version from December...

@balopat
Copy link
Contributor

balopat commented Apr 21, 2021

Hmmm...my "automated rebasing" strategy was error prone, this must have been overridden then. Let me fix that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes Makes googlebot stop complaining.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants