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

Stop including type stubs in pex_binary targets #15454

Open
Eric-Arellano opened this issue May 13, 2022 · 8 comments
Open

Stop including type stubs in pex_binary targets #15454

Eric-Arellano opened this issue May 13, 2022 · 8 comments
Assignees
Labels
backend: Python Python backend-related issues enhancement

Comments

@Eric-Arellano
Copy link
Contributor

We've heard from several users they only want type stubs like types-requests to be used with MyPy. It's a bummer to bundle w/ a PEX binary etc as it adds bloat.

This could be a use case for "dependency scopes": #12794

But I think we can implement this more easily. Given any python_requirement target, we have a way to know if it's a type stub or not:

  • If type_stub_modules is set, we can be 100% confident it is
  • if modules is set, we can be 100% confident it is
  • if it shows up in our default module mapping, we have a good idea it is
  • fallback to our heuristic to inspect the project_name if it has types- etc

If we decide it's a type stub, then leave it out of the PEX.

Unclear to me if we need a mechanism to force type stubs in the binary? Maybe a field on pex_binary called include_type_stubs: bool w/ default to false?

@kaos
Copy link
Member

kaos commented May 15, 2022

Maybe a field on pex_binary called include_type_stubs: bool w/ default to false?

I'd say yes, primarily in case a heuristic gets it wrong, it could be difficult to work around otherwise. But hmm, on second thought I disagree with my own argument here. Type stub classification ought to be directed at the source rather than at the dependent site (with a misleading option name, for that case as it wouldn't be a type stub to include and also may include more than desired).

However I'm still +1 for the option, in case type stubs are indeed desired to be included.

@Eric-Arellano
Copy link
Contributor Author

Type stub classification ought to be directed at the source rather than at the dependent site

Agreed. I think you'd do that by setting type_stub_modules field

@thejcannon thejcannon added the backend: Python Python backend-related issues label Jun 7, 2022
huonw added a commit to huonw/pants-awslambda-UndefinedEnvironmentName that referenced this issue Jul 15, 2022
huonw added a commit to huonw/pants-awslambda-UndefinedEnvironmentName that referenced this issue Jul 15, 2022
@huonw
Copy link
Contributor

huonw commented Jul 15, 2022

We're hitting this, and the worst part is that mypy itself seems to be being included if any of the stubs depends on it (e.g. if a package is both a set of stubs and a plugin, like https://github.com/dropbox/sqlalchemy-stubs), and mypy is a large dependency (~50MB). This is particularly annoying for AWS Lambdas, which have a strict 250MB (unzipped) limit for the a code archive.

Work around: it seems that setting dependencies=["!!:mypy", "!!:sqlalchemy-stubs", "!!:..."] on the python_awslambda target (or pex_binary presumably) will eliminate them from the package (NB. every stub/plugin package that depends on mypy needs to be excluded). This is despite the type stubs and mypy itself not seeming to be modelled as normal dependencies (don't appear in ./pants dependencies --transitive path/to:target).

Example (including work around): huonw/pants-awslambda-UndefinedEnvironmentName@182145d...94bdc6f

@Eric-Arellano
Copy link
Contributor Author

Sorry for the delay in replying @huonw, I was OOO. And pardon the issue! Thank you for sharing the workaround.

--

@thejcannon this ticket intersects a little bit with your proposals around dependencies, I think? What are your thoughts on my original proposed fix, vs. waiting for your proposals?

@thejcannon
Copy link
Member

I like forward progress but I also dislike churn.

If we can stomach it, I'll blast out my proposal later this week for RFC. The two "starting points" can be assets and this.

WDYT?

@Eric-Arellano
Copy link
Contributor Author

This ticket is the type of polish I'm hoping to have more time to implement now that my life is stabilizing after 6 months hah. But I'm also unlikely to get to it in one week, so sounds good to me. I'll assign this ticket to both of us so we don't let it sit for too long.

@sk0g
Copy link

sk0g commented May 30, 2023

Hey, just curious if there was progress made on this that wasn't recorded here?
MyPy and stubs, especially Boto ones, get quite large. CI time would probably be my main concern though.

@thejcannon
Copy link
Member

Not explicitly, although #19155 gets us closer than we've been to a generic solution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend: Python Python backend-related issues enhancement
Projects
None yet
Development

No branches or pull requests

5 participants