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

Prepare to enable pantsd by default #9826

Merged

Conversation

stuhood
Copy link
Sponsor Member

@stuhood stuhood commented May 19, 2020

Problem

Pants' runtime overhead is influenced by multiple factors:

  1. packaging overhead in virtualenvs and pexes
  2. import time for python code
  3. time to walk the filesystem and (re)fingerprint inputs
  4. time to run @rules with unchanged inputs

Pantsd helps to address all of these issues, and has just (re-)reached maturity.

Solution

Prepare to enable pantsd by default by deprecating not setting the enable_pantsd flag. Also, set the flag for pantsbuild/pants.

Result

noop time when running in a virtualenv (such as in github.com/pantsbuild/example-python) drops from ~2.4s to ~1.6s. Followup work (moving fingerprinting to the server, porting the client to rust) is expected to push this below 500ms.

There are a collection of known issues that might impact users tracked in https://github.com/pantsbuild/pants/projects/5, some of which we will address via dogfooding. Others are matters of expectation: "I wouldn't expect that to work".

One of the most significant issues though is #7654: we should consider making a push before 1.29.0 to remove use of global mutable state to allow for concurrent pants runs with pantsd under v2.

Fixes #4438.

@stuhood stuhood force-pushed the stuhood/pantsd-by-default-questionmark branch 2 times, most recently from e4c3d85 to 39a4e6e Compare May 21, 2020 16:58
# Delete this line to force CI to run Clippy and the Rust tests.
[ci skip-rust-tests]
# Delete this line to force CI to run the JVM tests.
[ci skip-jvm-tests]
@stuhood stuhood force-pushed the stuhood/pantsd-by-default-questionmark branch 2 times, most recently from 2bcc5f4 to 875e4f7 Compare May 22, 2020 05:46
[ci skip-rust-tests]
[ci skip-jvm-tests]
…hem being gitignored.

# Delete this line to force CI to run the JVM tests.
[ci skip-jvm-tests]
# Delete this line to force CI to run Clippy and the Rust tests.
[ci skip-rust-tests]
@stuhood stuhood force-pushed the stuhood/pantsd-by-default-questionmark branch from 875e4f7 to da95bb2 Compare May 22, 2020 17:42
@stuhood stuhood changed the title Explore enabling pantsd by default. Enable pantsd by default May 22, 2020
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Awesome. Great work! Not yet approving because of the deprecation policy, but otherwise looks great.

@@ -134,11 +134,18 @@ REQUIREMENTS_3RDPARTY_FILES=(
# internal backend packages and their dependencies which it doesn't have,
# and it'll fail. To solve that problem, we load the internal backend package
# dependencies into the pantsbuild.pants venv.
#
# TODO: Starting and stopping pantsd repeatedly here works fine, but because the
# created venvs are located within the buildroot, pantsd will fingerprint them on
Copy link
Contributor

Choose a reason for hiding this comment

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

We do encourage users to create venvs within the build root: https://pants.readme.io/docs/python-third-party-dependencies#tip-set-up-a-virtual-environment-optional

Presumably, those will be in their .gitignore already, so it would be in pants_ignore. Should this be okay for end users?

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

This is referring to a virtualenv that contains pants' own code, rather than to end user virtualenvs. pantsd fingerprints its own sys.path and source plugins in order to restart if they are edited.

src/python/pants/option/global_options.py Outdated Show resolved Hide resolved
src/python/pants/testutil/pants_run_integration_test.py Outdated Show resolved Hide resolved
src/python/pants/testutil/pants_run_integration_test.py Outdated Show resolved Hide resolved
@stuhood stuhood requested review from benjyw and blorente May 22, 2020 18:23
@stuhood stuhood marked this pull request as ready for review May 22, 2020 18:24
):
"""Temporarily create the given literal directory, which must not exist."""
path = os.path.realpath(path)
assert path.startswith(
Copy link
Contributor

Choose a reason for hiding this comment

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

This assert is surprising given the doc. Seems worth documenting or else moving the caring about this to the call site that has this restriction.


parent = os.path.dirname(path)
parent_ctx = (
suppress() if os.path.isdir(parent) else self.temporary_directory_literal(parent)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can use os.makedirs to get rid of the recursion and existence checks: https://docs.python.org/3/library/os.html#os.makedirs

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

The recursion is to try to leave things as we found them: we recurse upward to create the parents temporarily, rather than permanently as with makedirs. Expanded the method docs.

…links, and is no longer strictly necessary for isolation when run with v2.

[ci skip-rust-tests]
[ci skip-jvm-tests]
…tead, 2) enable only for the pantsbuild/pants repo, 3) test helper cleanup.

[ci skip-rust-tests]
[ci skip-jvm-tests]
@stuhood stuhood force-pushed the stuhood/pantsd-by-default-questionmark branch from 298682a to 307af2e Compare May 22, 2020 23:18
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Thanks! Looks great from my perspective. The title should be updated though to something like "Deprecate pantsd being disabled by default"

@stuhood stuhood changed the title Enable pantsd by default Prepare to enable pantsd by default May 22, 2020
@stuhood stuhood merged commit 93a66ca into pantsbuild:master May 23, 2020
@stuhood stuhood deleted the stuhood/pantsd-by-default-questionmark branch May 23, 2020 00:50
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.

3 participants