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

Include the Pants native client in released wheels #18957

Merged
merged 3 commits into from
May 10, 2023

Conversation

stuhood
Copy link
Member

@stuhood stuhood commented May 9, 2023

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.

@thejcannon
Copy link
Member

Just to get a sense of timelines and expectations. Should we do this once we bump to 2.18, which includes only publishing one wheel to PyPI (because we're assuming scie-pants)?

@stuhood
Copy link
Member Author

stuhood commented May 9, 2023

Just to get a sense of timelines and expectations. Should we do this once we bump to 2.18, which includes only publishing one wheel to PyPI (because we're assuming scie-pants)?

I don't think that we need to, no. I have been using the native client in the pants repo for a long time without issues, and actually experiencing the change will require updating scie-pants as well. And if it is broken for a user, they can use the PANTS_NO_NATIVE_CLIENT variable defined in pantsbuild/scie-pants#172 to disable it.

@thejcannon
Copy link
Member

Oh sorry I left out, I meant for size's sake. So we aren't adding even more to our wheel uploads?
Also knowing that 2.18 is right around the corner ;)

@stuhood
Copy link
Member Author

stuhood commented May 9, 2023

Oh sorry I left out, I meant for size's sake. So we aren't adding even more to our wheel uploads? Also knowing that 2.18 is right around the corner ;)

#18838 dropped Linux wheel sizes by 40% (~30MB per wheel), and this change adds ~5MB back to wheel sizes for Linux and ~1MB for macOS... I think that it's fine?

@thejcannon
Copy link
Member

Ha! Okie dokie

set +e
"${PANTS_NATIVE_EXE}" "$@"
"${NATIVE_CLIENT_BINARY}" "$@"
Copy link
Contributor

Choose a reason for hiding this comment

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

Did you intend to get some mileage on _PANTS_SERVER_EXE here? Seems good to do.

Copy link
Member Author

Choose a reason for hiding this comment

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

I didn't (yet): it's mentioned in the description as something that would be worth doing. But I think that doing it requires significant refactoring of this script to split it into pants vs pants_server scripts.

@stuhood stuhood enabled auto-merge (squash) May 9, 2023 23:13
@stuhood stuhood force-pushed the stuhood/native-client-in-wheel branch from 7784f9a to 178fa29 Compare May 10, 2023 20:27
@stuhood stuhood merged commit 558d843 into pantsbuild:main May 10, 2023
@stuhood stuhood deleted the stuhood/native-client-in-wheel branch May 10, 2023 21:35
stuhood added a commit to pantsbuild/scie-pants that referenced this pull request May 15, 2023
This change uses the `native-client` binary included in
`pantsbuild/pants` wheels by
pantsbuild/pants#18957 (and further adjusted in
pantsbuild/pants#19010) to launch `pants`. See
that change for the performance impact.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Port pantsd client to rust.
5 participants