pants icon indicating copy to clipboard operation
pants copied to clipboard

Stop including type stubs in `pex_binary` targets

Open Eric-Arellano opened this issue 2 years ago • 6 comments

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": https://github.com/pantsbuild/pants/issues/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?

Eric-Arellano avatar May 13 '22 21:05 Eric-Arellano

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.

kaos avatar May 15 '22 07:05 kaos

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

Eric-Arellano avatar May 15 '22 12:05 Eric-Arellano

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): https://github.com/huonw/pants-awslambda-UndefinedEnvironmentName/compare/182145d2b3e742b455c0888cb0df74916589dc9a...94bdc6fe84a0dec263e81a6a13e4278352d77e34

huonw avatar Jul 15 '22 04:07 huonw

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?

Eric-Arellano avatar Aug 03 '22 20:08 Eric-Arellano

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?

thejcannon avatar Aug 03 '22 20:08 thejcannon

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.

Eric-Arellano avatar Aug 03 '22 21:08 Eric-Arellano

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.

sk0g avatar May 30 '23 11:05 sk0g

Not explicitly, although https://github.com/pantsbuild/pants/pull/19155 gets us closer than we've been to a generic solution

thejcannon avatar May 30 '23 12:05 thejcannon