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

py.typed file is not present in built distributions on pypi #531

Closed
sirosen opened this issue Feb 24, 2023 · 9 comments
Closed

py.typed file is not present in built distributions on pypi #531

sirosen opened this issue Feb 24, 2023 · 9 comments

Comments

@sirosen
Copy link
Contributor

sirosen commented Feb 24, 2023

Issue

At $WORK, we found that mypy type checking didn't work with dramatiq, which surprised us because of the presence of the py.typed in the repo here.

Investigating, the setuptools config seems fine, and I'm not able to reproduce a build which omits this file using a modern version of setuptools or using pypa's build to run python -m build ..

What version of Dramatiq are you using?

1.14.0

What did you do?

Install dramatiq and mypy into a virtualenv, write a file which runs reveal_type(dramatiq.set_encoder), and run mypy on that file.

# foo.py
import dramatiq

reveal_type(dramatiq.set_encoder)

What did you expect would happen?

$ mypy foo.py
foo.py:3: note: Revealed type is "def (encoder: dramatiq.encoder.Encoder)"

What happened?

$ mypy foo.py
foo.py:3: note: Revealed type is "Any"

Fixing this

It's not clear how to submit a code change which could fix this because it's not clear what the publishing workflow for dramatiq is. I believe this can be handled with some codified workflow which uses python -m build. e.g. I've put this into tox in many of my projects as tox r -e build.

I'm happy to contribute a fix which adds some canonical build workflow, but don't know that it's necessary or desirable.

@sirosen
Copy link
Contributor Author

sirosen commented Feb 24, 2023

Oh, also, I downloaded the sdist and wheel from pypi for 1.14.0 and ran tar -tzf and unzip -l on them, and did not see py.typed in the file list.

@Bogdanp
Copy link
Owner

Bogdanp commented Feb 25, 2023

Thanks! I've just published 1.14.1, which should fix this problem.

Re. the workflow, currently it's a manual process where I

  1. run bumpversion with the relevant modifier (patch or minor)
  2. run python setup.py sdist bdist_wheel
  3. run twine upload -s dist/dramatiq-$version

I'd happily accept a PR to automate this. I think @amureki did a great job with django_dramatiq's workflow (see here) so maybe we could just port that over here.

@jenstroeger
Copy link
Contributor

I'd happily accept a PR to automate this. I think @amureki did a great job with django_dramatiq's workflow (see here) so maybe we could just port that over here.

@Bogdanp interested in using (some of) the template repo https://github.com/jenstroeger/python-package-template?

@Bogdanp
Copy link
Owner

Bogdanp commented Feb 25, 2023

@Bogdanp interested in using (some of) the template repo https://github.com/jenstroeger/python-package-template?

@jenstroeger that looks nice and I think I'd probably look at that for a new project, but for this I think all we really need is to copy the workflow I linked to. If someone wants to do that, that'd be great. If not, I'll get to it eventually.

@sirosen
Copy link
Contributor Author

sirosen commented Feb 25, 2023

Thanks for publishing the fixed version!

I'll try to make time to open a PR with the publishing workflow. It will need creds, of course, which I won't be able to do.

@jenstroeger
Copy link
Contributor

@sirosen if you don’t get around to it, I’m happy to lend a hand.

@jenstroeger
Copy link
Contributor

jenstroeger commented Feb 26, 2023

Adding the py.typed marker to the package (commit 30a1564) seems to be a breaking change.

I’ve got mypy configured with

[[tool.mypy.overrides]]
module = [
    "dramatiq.*",  # https://github.com/Bogdanp/dramatiq/issues/521
    # ...
]
ignore_missing_imports = true

because it doesn’t provide complete typing annotations, which was enough until and including v1.14.0.

As of v1.14.1, however, I now get a whole bunch of errors like e.g.

error: Call to untyped function "RabbitmqBroker" in typed context

which break local builds and CI. So I added explicit # type: ignore[no-untyped-call] to the code to manage these errors. However, these new type ignores are considered

error: Unused "type: ignore" comment

when I then step back to Dramatiq v1.14.0.

@Bogdanp might be worth a note in the Release Notes?

@sirosen
Copy link
Contributor Author

sirosen commented Feb 27, 2023

I wouldn't really call the change breaking (at least, in the semver sense of "breaking change"). It's fixing a bug.

Not to dismiss what you're saying and experiencing, but I just don't think it needs anything more than what's in the release notes already.

It should be an expectation that type annotations can't be written consistently to satisfy fixed/improved annotations in a library in one version, and mypy --warn-unused-ignores in versions prior to the improvement.

@Bogdanp
Copy link
Owner

Bogdanp commented Feb 28, 2023

I have not used mypy very much over the past few years, so I might not have the full picture here. But, insofar as I understand the problem, I think I agree with @sirosen. As long as typing works without any special configuration, I think I wouldn't consider this a breaking change, since what's breaking is configs dramatiq has no control over. That said, it's not great that a release has caused any breakage at all, but hopefully this is a one-time thing.

Mouhand-Kaddo pushed a commit to KalvadTech/dramatiq_postgres that referenced this issue Mar 1, 2023
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

No branches or pull requests

3 participants