-
-
Notifications
You must be signed in to change notification settings - Fork 636
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
Introduce a native pants client. #11922
Conversation
I've posted this PR as one big change. I'm happy to break into smaller changes if reviewers wish, but this may make more sense since the change is broken up naturally by modules, which are all ~200 lines or less and conceptually tightly constrained. To look at things in dependency order from none to some:
Outstanding bits in my mind:
|
Obligatory timings:
Note that USE_NATIVE_PANTS adds ~60ms of overhead just activating the pants venv and no-oping the rust build. The failure no-ops when the native client doesn't have a pantsd to talk to look like this:
And that last hyperfine warning is very nice to get. |
@benjyw added you in particular for options/parse.rs. I ended up going down the parser road after a bit of fiddling with Regex and reading your py list parsing code. |
} | ||
} | ||
|
||
fn format_parse_error( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This goes through some effort to present a useful error message. For example this shows both the native client error message and the python error message since any failure currently in the native client prior to nailgun session establishment has the pants script falling through to the python client:
$ USE_NATIVE_PANTS=1 PANTS_CONFIG_FILES="-['/etc/hosts'],
?(\"/dev/null\")
" ./pants -V
Problem parsing PANTS_CONFIG_FILES string list value:
1:-['/etc/hosts'],
2: ?("/dev/null")
---------^
3:
Expected an optional list edit action of '+' indicating `add` or '-' indicating `remove` at line 2 column 10
The value cannot be evaluated as a literal expression: IndentationError('unexpected indent', ('<string>', 2, 9, ' ?("/dev/null")\n'))
Given raw value:
1: ['/etc/hosts'],
2: ?("/dev/null")
Traceback (most recent call last):
File "/home/jsirois/dev/pantsbuild/jsirois-pants/src/python/pants/util/eval.py", line 47, in parse_expression
parsed_value = eval(val)
File "<string>", line 2
?("/dev/null")
^
IndentationError: unexpected indent
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or:
$ USE_NATIVE_PANTS=1 PANTS_CONFIG_FILES="-['/etc/hosts'],+(\"/dev/null\"]" ./pants -V
Problem parsing PANTS_CONFIG_FILES string list value:
1:-['/etc/hosts'],+("/dev/null"]
-----------------------------^
Expected "," or the end of a tuple indicated by ')' at line 1 column 30
The value cannot be evaluated as a literal expression: SyntaxError('unexpected EOF while parsing', ('<string>', 1, 13, '("/dev/null"]'))
Given raw value:
1: ("/dev/null"]
Traceback (most recent call last):
File "/home/jsirois/dev/pantsbuild/jsirois-pants/src/python/pants/util/eval.py", line 47, in parse_expression
parsed_value = eval(val)
File "<string>", line 1
("/dev/null"]
^
SyntaxError: unexpected EOF while parsing
This is not the benjy you're looking for :) |
Sorry @benjy. That's not the 1st and it won't be the last. It must be a bit of a curse having a common name handle on a big service. |
Sorry the rustfmt config is https://github.com/rust-lang/rustfmt/blob/master/Configurations.md#group_imports |
Ok, great @tdyas. That will be excellent, but also reformat the whole codebase. As such I'll defer that to its own disruptive change. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Huge. Great work John!
This is epic stuff! My rust knowledge is very entry-level, but the options-parsing part seems right to me, or at least it tracks with how the python options system behaves, esp wrt list-valued options. Ultimately the proof will be in the tests. |
Thanks for the feedback folks. I've addressed most or all of the missing feature / bug feedback. Still unit tests to fill out and a bit more feedback to address. I'll pick that all up on 4/21-4/22. |
8f61ba1
to
c994d0b
Compare
Alright, I believe all feedback is addressed and CI is now green. I just need to take one last pass at beefing up options tests. I'll be getting to that 4/26-4/27. |
Currently the client only handles talking to a pantsd brought up by other means. The Pants repo ./pants script is updated to optionally use the native client and prop up pantsd using the python client as needed. Work towards pantsbuild#11831 # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
Does this one have blockers? |
No external blockers. I just need time to finish tests to land it. |
…/11831 [ci skip-build-wheels]
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
…/11831 [ci skip-build-wheels]
…/11831 [ci skip-build-wheels]
So excited to see this land! |
…/11831 [ci skip-build-wheels]
Refactor launch_pantsd into a test utility to support this. # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
This found a few bugs, particularly handling escape sequences for implicit adds. # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
[ci skip-build-wheels]
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
[ci skip-build-wheels]
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
I'd still like to write tests for the toml option source and the OptionParser sandwich, but I'll be away for a while again; so I'm going to land this and circle back on that remaining test coverage. |
Internal changes omitted from the release notes: * [internal] `./pants lock` uses transitive closure as inputs ([pantsbuild#12312](pantsbuild#12312)) * [internal] Add experimental `./pants lock` to generate a lockfile with pip-compile ([pantsbuild#12300](pantsbuild#12300)) * upgrade tokio and futures crates ([pantsbuild#12308](pantsbuild#12308)) * upgrade tonic, tower, and hyper crates ([pantsbuild#12294](pantsbuild#12294)) * Set up pants to use the latest version of humbug ([pantsbuild#12302](pantsbuild#12302)) * Refactor BUILD file parsing. ([pantsbuild#12279](pantsbuild#12279)) * Use get_type_hints() to get typed type hints. ([pantsbuild#12287](pantsbuild#12287)) * [internal] Fix parameters of convert_source_to_sources_test ([pantsbuild#12274](pantsbuild#12274)) * Fix assert arguments in a test ([pantsbuild#12273](pantsbuild#12273)) * Introduce a native pants client. ([pantsbuild#11922](pantsbuild#11922)) * Don't run CI in forks ([pantsbuild#12267](pantsbuild#12267)) * Change how we embed docsite links in help text. ([pantsbuild#12261](pantsbuild#12261))
Internal changes omitted from the release notes: * [internal] `./pants lock` uses transitive closure as inputs ([pantsbuild#12312](pantsbuild#12312)) * [internal] Add experimental `./pants lock` to generate a lockfile with pip-compile ([pantsbuild#12300](pantsbuild#12300)) * upgrade tokio and futures crates ([pantsbuild#12308](pantsbuild#12308)) * upgrade tonic, tower, and hyper crates ([pantsbuild#12294](pantsbuild#12294)) * Set up pants to use the latest version of humbug ([pantsbuild#12302](pantsbuild#12302)) * Refactor BUILD file parsing. ([pantsbuild#12279](pantsbuild#12279)) * Use get_type_hints() to get typed type hints. ([pantsbuild#12287](pantsbuild#12287)) * [internal] Fix parameters of convert_source_to_sources_test ([pantsbuild#12274](pantsbuild#12274)) * Fix assert arguments in a test ([pantsbuild#12273](pantsbuild#12273)) * Introduce a native pants client. ([pantsbuild#11922](pantsbuild#11922)) * Don't run CI in forks ([pantsbuild#12267](pantsbuild#12267)) * Change how we embed docsite links in help text. ([pantsbuild#12261](pantsbuild#12261))
Internal changes omitted from the release notes: * [internal] `./pants lock` uses transitive closure as inputs ([pantsbuild#12312](pantsbuild#12312)) * [internal] Add experimental `./pants lock` to generate a lockfile with pip-compile ([pantsbuild#12300](pantsbuild#12300)) * upgrade tokio and futures crates ([pantsbuild#12308](pantsbuild#12308)) * upgrade tonic, tower, and hyper crates ([pantsbuild#12294](pantsbuild#12294)) * Set up pants to use the latest version of humbug ([pantsbuild#12302](pantsbuild#12302)) * Refactor BUILD file parsing. ([pantsbuild#12279](pantsbuild#12279)) * Use get_type_hints() to get typed type hints. ([pantsbuild#12287](pantsbuild#12287)) * [internal] Fix parameters of convert_source_to_sources_test ([pantsbuild#12274](pantsbuild#12274)) * Fix assert arguments in a test ([pantsbuild#12273](pantsbuild#12273)) * Introduce a native pants client. ([pantsbuild#11922](pantsbuild#11922)) * Don't run CI in forks ([pantsbuild#12267](pantsbuild#12267)) * Change how we embed docsite links in help text. ([pantsbuild#12261](pantsbuild#12261)) * Add JavacSubsystem and JavacBinary, allowing user-configurable JVM selection ([pantsbuild#12283](pantsbuild#12283)) * Revert "Deprecate unused `--process-execution-local-enable-nailgun` (pantsbuild#11600)" ([pantsbuild#12257](pantsbuild#12257))
Internal changes omitted from the release notes: * [internal] `./pants lock` uses transitive closure as inputs ([#12312](#12312)) * [internal] Add experimental `./pants lock` to generate a lockfile with pip-compile ([#12300](#12300)) * upgrade tokio and futures crates ([#12308](#12308)) * upgrade tonic, tower, and hyper crates ([#12294](#12294)) * Set up pants to use the latest version of humbug ([#12302](#12302)) * Refactor BUILD file parsing. ([#12279](#12279)) * Use get_type_hints() to get typed type hints. ([#12287](#12287)) * [internal] Fix parameters of convert_source_to_sources_test ([#12274](#12274)) * Fix assert arguments in a test ([#12273](#12273)) * Introduce a native pants client. ([#11922](#11922)) * Don't run CI in forks ([#12267](#12267)) * Change how we embed docsite links in help text. ([#12261](#12261)) * Add JavacSubsystem and JavacBinary, allowing user-configurable JVM selection ([#12283](#12283)) * Revert "Deprecate unused `--process-execution-local-enable-nailgun` (#11600)" ([#12257](#12257))
To prepare to make the address syntax more complicated as part of #13882, move to parsing addresses using a generated `peg` parser (using the same parser crate that @jsirois introduced in #11922). Additionally, simplify `Address.spec` and `Address.path_safe_spec` slightly by removing non-trivial early returns and reducing the total number of f-strings. [ci skip-build-wheels]
The Pants native client which was introduced in #11922 has so far only been usable in the Pants repo when the `USE_NATIVE_PANTS` environment variable was set. To make the native client available to end users, this change begins distributing the native-client binary in Pants wheels. A corresponding change in the `scie-pants` repo (pantsbuild/scie-pants#172) uses the native client to run `pants`. To reduce the surface area of `scie-pants` (rather than having it be responsible for handling the special `75` exit code similar to the `pants` script integration), this PR also moves to having the native-client execute its own fallback (via `execv`) to the Pants entrypoint. In future, the `pantsbuild/pants` `pants` script could also use that facility (e.g. by specifying a separate `pants_server` bash script as the entrypoint to use as the `_PANTS_SERVER_EXE`). ---- As originally demonstrated on #11831, the native client is still much faster than the legacy client. Using pantsbuild/scie-pants#172, the timings look like: ``` Benchmark #1: PANTS_NO_NATIVE_CLIENT=true PANTS_SHA=836cceb74e6af042e7c82677f3ceb4927efce20e scie-pants-macos-x86_64 help Time (mean ± σ): 1.161 s ± 0.067 s [User: 830.6 ms, System: 79.2 ms] Range (min … max): 1.054 s … 1.309 s 10 runs Benchmark #2: PANTS_SHA=836cceb74e6af042e7c82677f3ceb4927efce20e scie-pants-macos-x86_64 help Time (mean ± σ): 271.0 ms ± 30.6 ms [User: 8.9 ms, System: 6.9 ms] Range (min … max): 241.5 ms … 360.6 ms 12 runs Summary 'PANTS_SHA=836cceb74e6af042e7c82677f3ceb4927efce20e scie-pants-macos-x86_64 help' ran 4.29 ± 0.54 times faster than 'PANTS_NO_NATIVE_CLIENT=true PANTS_SHA=836cceb74e6af042e7c82677f3ceb4927efce20e scie-pants-macos-x86_64 help' ``` Fixes #11831.
Currently the client only handles talking to a pantsd brought up by
other means. The Pants repo ./pants script is updated to optionally use
the native client and prop up pantsd using the python client as needed.
Work towards #11831