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

Delete stubs for cryptography #9459

Merged
merged 5 commits into from
Jan 10, 2023
Merged

Conversation

AlexWaygood
Copy link
Member

@AlexWaygood AlexWaygood commented Jan 4, 2023

Closes #5768.
Closes #5618.
Closes #5847.

@github-actions

This comment has been minimized.

@AlexWaygood
Copy link
Member Author

Looks like the pytype test is still failing @Avasam :(

I'm taking a look at the stub_uploader error.

@Avasam
Copy link
Sponsor Collaborator

Avasam commented Jan 4, 2023

Looks like the same issue we saw before. Last time I couldn't find any config to let pytype use the typed library or even disable this specific error across the board.

(I'll let you tag a pytype maintainer if you want their opinion first)
Any idea if there's something we can do, are doing wrong and/or should raise as an issue in pytype's tracker?

@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Jan 4, 2023

Looks like a bug with typeshed-internal/stub_uploader#80 ordered is not guaranteed to only contain stubs distributions (which is e.g. why we have if dist in distributions)
edit: not exactly

@hauntsaninja
Copy link
Collaborator

I can make a PR!

@AlexWaygood
Copy link
Member Author

AlexWaygood commented Jan 4, 2023

I can make a PR!

Amazing, thank you! I got held up by the fact that a bunch of the stub_uploader tests don't pass on Windows (am working on a PR for that) :)

@Avasam
Copy link
Sponsor Collaborator

Avasam commented Jan 4, 2023

After further investigation, A lot of the pytype errors seem to reference types that mypy also flags as missing (ie: cryptography.hazmat.primitives.ciphers). I think we should fix mypy errors first and then take a new look at the pytype errors.

@AlexWaygood
Copy link
Member Author

After further investigation, A lot of the pytype errors seem to reference types that mypy also flags as missing (ie: cryptography.hazmat.primitives.ciphers). I think we should fix mypy errors first and then take a new look at the pytype errors.

All the mypy tests pass locally with #9408 applied ;)

@AlexWaygood AlexWaygood closed this Jan 4, 2023
@AlexWaygood AlexWaygood reopened this Jan 4, 2023
@AlexWaygood
Copy link
Member Author

That fixed the stub_uploader tests -- thanks @hauntsaninja!

@Avasam
Copy link
Sponsor Collaborator

Avasam commented Jan 4, 2023

All the mypy tests pass locally with #9408 applied ;)

So mypy doesn't pass now, because cryptography is not installed in mypy_test. But will with #9408 .
pytype test on CI does install cryptography, and it's not using a virtual environment, so pytype should see the modules/types.
I should double-check the same issue happens locally.

Unless I'm missing something here (and with the huge pytype errors, I might), seems like a pytype issue with py.typed modules.

@github-actions

This comment has been minimized.

@AlexWaygood
Copy link
Member Author

All the mypy tests pass locally with #9408 applied ;)

So mypy doesn't pass now, because cryptography is not installed in mypy_test. But will with #9408 . pytype test on CI does install cryptography, and it's not using a virtual environment, so pytype should see the modules/types. I should double-check the same issue happens locally.

Unless I'm missing something here (and with the huge pytype errors, I might), seems like a pytype issue with py.typed modules.

@rchen152, any idea what's going on here with the pytype errors? As @Avasam says, it looks like cryptography is being successfully installed in CI (https://github.com/python/typeshed/actions/runs/3841901232/jobs/6542651921) :/

@Avasam
Copy link
Sponsor Collaborator

Avasam commented Jan 4, 2023

Oh, wanna bet parse_pyi expects only .pyi files?

@Avasam
Copy link
Sponsor Collaborator

Avasam commented Jan 4, 2023

Depending on how soon you want to complete #5768 and get rid of cryptography, vs the complexity and time-to-implement a solution on pytype's side, supressing this exception is feasable, although not great 😕

from pytype.load_pytd import BadDependencyError  # type: ignore[import]
from utils import colored

# [...]

    stderr: str | None = None
    try:
        with pytype_config.verbosity_from(options):
            ast = loader.load_file(_get_module_name(filename), filename)
            loader.finish_and_verify_ast(ast)
    except BadDependencyError as error:
        error_str = str(error)
        # pytype errors out with non-types dependencies.
        # mypy and pyright already take care of ensuring external types are installed and do exist,
        # but this issue is problematic as it hides other potential errors in the same file.
        if error_str.startswith("Can't find pyi for"):
            print(
                colored(
                    f"{error_str}. This is likely an issue with pytype in typeshed, and not an issue with the stub.", "yellow"
                )
            )
        else:
            stderr = traceback.format_exc()
    except Exception:
        stderr = traceback.format_exc()

@AlexWaygood
Copy link
Member Author

Depending on how soon you want to complete #5768 and get rid of cryptography, vs the complexity and time-to-implement a solution on pytype's side, supressing this exception is feasable, although not great 😕

That's good to have as a fallback option, but definitely wouldn't be my preferred solution :)

@rchen152
Copy link
Collaborator

rchen152 commented Jan 4, 2023

Oh, I didn't realize that you wanted pytype_test to be able to process non-pyi dependencies. That seems a little challenging to set up. The simplest (well, definitely the quickest) thing to do is probably just to mark those dependencies as missing so that pytype treats them as having default stubs (def __getattr__(name) -> Any).

I'll try changing the missing_file argument to pytype.imports.typeshed.Typeshed to take a list of missing dependencies instead of a file, so that pytype_test can use tests/utils/read_dependencies to get a list of external dependencies (like what get_external_stub_requirements does) and pass the list to the Typeshed object.

@Avasam
Copy link
Sponsor Collaborator

Avasam commented Jan 5, 2023

Would closing #5768 also close #8312 since a decision seems to have been taken?

@github-actions

This comment has been minimized.

@@ -1,2 +1,3 @@
version = "23.0.*"
requires = ["types-cryptography"]
# Requires a version of cryptography with a `py.typed` file
requires = ["cryptography>=35.0.0"]
Copy link
Sponsor Collaborator

@Avasam Avasam Jan 5, 2023

Choose a reason for hiding this comment

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

Interesting. I feel like it would make sense to always enforce a minimal version for external dependencies of 3rd party stub.

Copy link
Member Author

@AlexWaygood AlexWaygood Jan 5, 2023

Choose a reason for hiding this comment

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

Interesting idea — definitely something we should explore in the future!

Copy link
Sponsor Collaborator

@Avasam Avasam Jan 5, 2023

Choose a reason for hiding this comment

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

Without having to enforce it through CI. Maybe a mention in documentation that external dependencies should be at least pinned to their first py.typed version? And optimally to the oldest version that has the types referenced by the stub.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll try to work on docs as soon as this PR makes it over the line — this is a very good point, thanks!

@AlexWaygood
Copy link
Member Author

Would closing #5768 also close #8312 since a decision seems to have been taken?

Let's leave it open for now; see comments on the issue :)

@github-actions

This comment has been minimized.

@rchen152
Copy link
Collaborator

Oh, I didn't realize that you wanted pytype_test to be able to process non-pyi dependencies. That seems a little challenging to set up. The simplest (well, definitely the quickest) thing to do is probably just to mark those dependencies as missing so that pytype treats them as having default stubs (def __getattr__(name) -> Any).

I'll try changing the missing_file argument to pytype.imports.typeshed.Typeshed to take a list of missing dependencies instead of a file, so that pytype_test can use tests/utils/read_dependencies to get a list of external dependencies (like what get_external_stub_requirements does) and pass the list to the Typeshed object.

It took a little longer than I expected, but I believe I have this working. It requires both a change to pytype (which I'll export and release tomorrow or Wednesday) and a change to pytype_test, which I've prepared a draft pull request for: #9486.

rchen152 added a commit to google/pytype that referenced this pull request Jan 10, 2023
This will allow typeshed's pytype_test to declare missing modules. See
python/typeshed#5768 and
python/typeshed#9459 for context.

PiperOrigin-RevId: 501058238
@AlexWaygood AlexWaygood marked this pull request as ready for review January 10, 2023 22:06
@github-actions
Copy link
Contributor

Diff from mypy_primer, showing the effect of this PR on open source code:

pyjwt (https://github.com/jpadilla/pyjwt)
- jwt/algorithms.py:506: error: "EllipticCurvePrivateKey" has no attribute "private_numbers"  [attr-defined]
- jwt/help.py:11: error: Module has no attribute "__version__"  [attr-defined]
+ jwt/algorithms.py:591: error: Argument "salt_length" to "PSS" has incompatible type "Callable[[HashAlgorithm], int]"; expected "Union[int, _MaxLength, _Auto, _DigestLength]"  [arg-type]
+ jwt/algorithms.py:603: error: Argument "salt_length" to "PSS" has incompatible type "Callable[[HashAlgorithm], int]"; expected "Union[int, _MaxLength, _Auto, _DigestLength]"  [arg-type]

urllib3 (https://github.com/urllib3/urllib3)
- src/urllib3/contrib/pyopenssl.py:47: error: Unused "type: ignore" comment
- src/urllib3/contrib/pyopenssl.py:174: error: Module "cryptography.x509.extensions" has no attribute "Extensions"; maybe "Extension"?  [attr-defined]

@AlexWaygood AlexWaygood merged commit e2d67bf into python:main Jan 10, 2023
@AlexWaygood AlexWaygood deleted the delete-cryptography branch January 10, 2023 22:16
hsheth2 added a commit to hsheth2/datahub that referenced this pull request Jan 11, 2023
In the latest releast of types-PyOpenSSL, they removed their dep on
types-cryptography (python/typeshed#9459). This
caused mypy to start using the type annotations provided by the
cryptography package, which caused an issue in our build. Example build
failure: https://github.com/datahub-project/datahub/actions/runs/3888835554/jobs/6636552750
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.

Allow non-types dependencies Remove cryptography (depends on #5952)
4 participants