Speed up `run` to no longer rebuild a Pex on source file changes #10410

Eric-Arellano · 2020-07-21T00:11:16Z

Whereas binary must include source files in the PEX, run does not need to. We get less cache invalidation and generally faster performance by instead having the chroot simply be populated with the source files, similar to how we implement Pytest.

We still use a Pex to handle the entry_point field and to resolve all 3rd party requirements.

Before, with a whitespace change:

▶ /usr/bin/time ./pants run build-support/bin/generate_travis_yml.py > .travis.yml
        2.72 real         0.73 user         0.21 sys

After, with a whitespace change:

▶ /usr/bin/time ./pants run build-support/bin/generate_travis_yml.py > .travis.yml
        1.87 real         0.73 user         0.21 sys

Implements half of #10406.

[ci skip-rust-tests]

coveralls · 2020-07-21T01:00:17Z

Coverage remained the same at 0.0% when pulling 2542706 on Eric-Arellano:faster-run into 26c73d8 on pantsbuild:master.

stuhood · 2020-07-21T01:07:30Z

src/python/pants/core/goals/run.py

+class RunRequest:
+    digest: Digest
+    binary_name: str
+    extra_args: Tuple[str, ...]


Nit: "extra" to me tends to imply that things are added afterward. Maybe prefix_args?

stuhood · 2020-07-21T01:12:20Z

src/python/pants/backend/python/rules/run_python_binary.py

+async def run_python_binary(
+    field_set: PythonBinaryFieldSet, python_binary_defaults: PythonBinaryDefaults
+) -> RunRequest:


It feels like rather than being binary (or repl) specific, this is connected to TwoStepPex... where rather than creating two pexes (one for requirements, the other for sources, iirc) we instead could use a requirements pex, and a Digest of loose sources.

Repl already uses TwoStepPex, so perhaps starting there would make sense?

I don't think so. We use TwoStepPex for binary and awslambda, actually. TwoStepPex results in one single Pex file. I think it's important that it continues to behave the same way.

I plan to change repl tomorrow to behave the same way as run. If it helps, I can bundle it into this PR. Only wanted to get this one up as a checkpoint and to make it in time for the release.

I don't think so. We use TwoStepPex for binary and awslambda, actually. TwoStepPex results in one single Pex file. I think it's important that it continues to behave the same way.

Got it: my mistake.

I plan to change repl tomorrow to behave the same way as run. If it helps, I can bundle it into this PR. Only wanted to get this one up as a checkpoint and to make it in time for the release.

No need to bundle. Thanks!

No mistake - it was a good question! I was wondering around the same time you wrote the original question "Hm, can we delete TwoStepPex now?"

benjyw · 2020-07-21T16:53:06Z

I would maybe edit the title slightly - it's not that we no longer invalidate on source file changes (that would be broken), it's that we no longer rebuild a pex...

stuhood

Thanks!

stuhood · 2020-07-21T17:08:07Z

src/python/pants/backend/python/rules/run_python_binary.py

+
+
+@rule
+async def run_python_binary(


Nit: this doesn't actually run the binary: just produces it.

…0418) Similar to #10410. Closes #10406. [ci skip-rust-tests]

benjyw · 2020-07-24T00:15:17Z

FYI that according to git bisect this broke /pants run build-support/bin/generate_docs.py, because it can no longer see its resources.

benjyw · 2020-07-24T00:22:36Z

The resources are in the chroot, so maybe some PYTHONPATH issue.

benjyw · 2020-07-24T00:48:54Z

OK, the problem is that loading relative to __name__ no longer works. Even though AFAICT __name__ is __main__ in both cases.

benjyw · 2020-07-24T00:50:54Z

I think this is a quirk of the loader system in python, possibly not something we should address here.

benjyw · 2020-07-24T00:51:19Z

Workaround is to use pkgutil.get_data("generate_docs", f"docs_templates/{name}")instead of pkgutil.get_data(__name__, f"docs_templates/{name}")

Eric-Arellano · 2020-07-24T01:46:17Z

Hm, does that workaround work still if you run ./pants binary and then run the binary? If not, that's concerning.

benjyw · 2020-07-24T21:41:11Z

It does work in that case.

stuhood · 2020-07-28T22:26:48Z

It looks like one other visible effect of this change is that there won't be a PEX-INFO file created for the actual binary. We have some code internally that expects it.

Eric-Arellano · 2020-07-28T22:42:51Z

That expects PEX-INFO during run? binary should behave the same as before.

stuhood · 2020-07-28T23:46:10Z

That expects PEX-INFO during run? binary should behave the same as before.

It's less that they "expect it during run", and more that they expected the python_binary target to always involve the creation of a pex. I expect that the performance difference between the two implementations makes it worthwhile for them to be different, but it's a consideration.

Eric-Arellano · 2020-07-28T23:48:17Z

Hm, okay. Yeah, I'm thinking about a new user who has never used Pants or Pex. I think they would expect this current implementation. You don't normally need a Pex to run a script, so why would you need it here?

Whereas with binary, we are explicitly saying that the point of it is to create a Pex file.

Eric-Arellano added 2 commits July 20, 2020 13:22

Rename python_create_binary.py to create_python_binary.py

d02b2e3

[ci skip-rust-tests]

Speed up run to no longer invalidate on source file changes

2542706

[ci skip-rust-tests]

Eric-Arellano requested review from stuhood, benjyw and gshuflin July 21, 2020 00:11

stuhood reviewed Jul 21, 2020

View reviewed changes

Eric-Arellano changed the title ~~Speed up run to no longer invalidate on source file changes~~ Speed up run to no longer rebuild a Pex on source file changes Jul 21, 2020

stuhood approved these changes Jul 21, 2020

View reviewed changes

stuhood merged commit 64bfcbb into pantsbuild:master Jul 21, 2020

Eric-Arellano mentioned this pull request Jul 21, 2020

Speed up repl to no longer rebuild a Pex on source file changes #10418

Merged

Eric-Arellano deleted the faster-run branch July 21, 2020 20:48

Eric-Arellano added a commit that referenced this pull request Jul 21, 2020

Speed up repl to no longer rebuild a Pex on source file changes (#1…

eca3ba7

…0418) Similar to #10410. Closes #10406. [ci skip-rust-tests]

stuhood mentioned this pull request Jul 30, 2020

[v2] running binary goal (for a python_binary) doesn't create usable caches for a subsequent run goal on the same target. #10513

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Speed up `run` to no longer rebuild a Pex on source file changes #10410

Speed up `run` to no longer rebuild a Pex on source file changes #10410

Eric-Arellano commented Jul 21, 2020

coveralls commented Jul 21, 2020

stuhood Jul 21, 2020

stuhood Jul 21, 2020

Eric-Arellano Jul 21, 2020

stuhood Jul 21, 2020

Eric-Arellano Jul 21, 2020

benjyw commented Jul 21, 2020

stuhood left a comment

stuhood Jul 21, 2020

benjyw commented Jul 24, 2020

benjyw commented Jul 24, 2020

benjyw commented Jul 24, 2020

benjyw commented Jul 24, 2020

benjyw commented Jul 24, 2020

Eric-Arellano commented Jul 24, 2020

benjyw commented Jul 24, 2020

stuhood commented Jul 28, 2020

Eric-Arellano commented Jul 28, 2020

stuhood commented Jul 28, 2020

Eric-Arellano commented Jul 28, 2020



		@rule
		async def run_python_binary(

Speed up run to no longer rebuild a Pex on source file changes #10410

Speed up run to no longer rebuild a Pex on source file changes #10410

Conversation

Eric-Arellano commented Jul 21, 2020

coveralls commented Jul 21, 2020

stuhood Jul 21, 2020

Choose a reason for hiding this comment

stuhood Jul 21, 2020

Choose a reason for hiding this comment

Eric-Arellano Jul 21, 2020

Choose a reason for hiding this comment

stuhood Jul 21, 2020

Choose a reason for hiding this comment

Eric-Arellano Jul 21, 2020

Choose a reason for hiding this comment

benjyw commented Jul 21, 2020

stuhood left a comment

Choose a reason for hiding this comment

stuhood Jul 21, 2020

Choose a reason for hiding this comment

benjyw commented Jul 24, 2020

benjyw commented Jul 24, 2020

benjyw commented Jul 24, 2020

benjyw commented Jul 24, 2020

benjyw commented Jul 24, 2020

Eric-Arellano commented Jul 24, 2020

benjyw commented Jul 24, 2020

stuhood commented Jul 28, 2020

Eric-Arellano commented Jul 28, 2020

stuhood commented Jul 28, 2020

Eric-Arellano commented Jul 28, 2020

Speed up `run` to no longer rebuild a Pex on source file changes #10410

Speed up `run` to no longer rebuild a Pex on source file changes #10410