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

Add --scie-only & --scie-name-style. #2523

Merged
merged 9 commits into from
Sep 12, 2024
Merged

Conversation

jsirois
Copy link
Member

@jsirois jsirois commented Sep 10, 2024

These options allow the files output when using --scie {eager,lazy}
scies to be better controlled. The --scie-only option provides a way
to ensure no PEX file is output, just the resulting scie. The
--scie-name-style option allows control over how the scie's target
platform is incorporated in the scie file name.

These options allow the files output when using `--scie {eager,lazy}`
scies to be better controlled. The `--scie-only` option provides a way
to ensure no PEX file is output, just the resulting scie. The
`--scie-name-style` option allows control over how the scie's target
platform is incorporated in the scie file name.
These are likely better than the Pex PEX for most Pex binary use cases
since they include the management extra and run in `--venv` mode as well
as providing BusyBox support for equal footing of the `pex`, `pex-tools`
and `pex3` top-level commands.
@jsirois
Copy link
Member Author

jsirois commented Sep 10, 2024

@martina-oefelein I'm interested in your thoughts on the new --scie-only and --scie-name-style options. The bits you are likely most interested in ordered from most relevant to least:

I think this incorporates all your feedback from #2516 given the hard requirement I have of not breaking backwards compatibility; i.e: I could not change how things work by default and, unfortunately, I could only add options to get the desired results.

@jsirois
Copy link
Member Author

jsirois commented Sep 10, 2024

Pants reviewers: this is not as bad as the diffstat looks. The change in da66a85 was machine-generated and accounts for the lions share of the delta lines:

    Run `tox -e gen-scie-platform -- --all`.

 package/complete-platforms/linux-aarch64.json |  496 ++++++++++
 package/complete-platforms/linux-x86_64.json  |  874 +++++++++++++++++
 package/complete-platforms/macos-aarch64.json |  604 ++++++++++++
 package/complete-platforms/macos-x86_64.json  | 2629 ++++++++++++++++++++++++++++++++++++++++++++++++++
 package/pex-scie.lock                         |  138 +++
 5 files changed, 4741 insertions(+)

"--no-build",
"--no-compile",
"--no-use-system-time",
"--venv",
Copy link
Member Author

@jsirois jsirois Sep 10, 2024

Choose a reason for hiding this comment

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

One reason to switch from Pex PEX to a Pex scie:

:; hyperfine -w2 'dist/pex -V' 'dist/pex-linux-x86_64 pex -V'
Benchmark 1: dist/pex -V
  Time (mean ± σ):     472.5 ms ±   7.7 ms    [User: 445.7 ms, System: 35.4 ms]
  Range (min … max):   460.9 ms … 482.2 ms    10 runs

Benchmark 2: dist/pex-linux-x86_64 pex -V
  Time (mean ± σ):     171.1 ms ±   5.2 ms    [User: 163.4 ms, System: 10.8 ms]
  Range (min … max):   163.5 ms … 180.7 ms    17 runs

Summary
  dist/pex-linux-x86_64 pex -V ran
    2.76 ± 0.09 times faster than dist/pex -V

I could never do this with the Pex PEX since some host Pythons don't support -m venv out of the box (for example - Debian Pythons without an extra python-venv package installed). With a scie though, I know the PBS 3.12.5 CPython I've picked has -m venv.

The other reason is to easily gain useful pex3 cache purge output when the operation has to wait for in-flight PEX runs, but this may not be of much use depending on how Pants currently handles real-time console output from processes it launches.

Add a `--scie-busybox-pex-entrypoint-env-passthrough` option to trade a
small amount of perf for a Pex scie busybox accepting
PEX_{INTERPRETER,MODULE,SCRIPT} entry point control like its Pex PEX
sibling.
Comment on lines +127 to +129
"--scie-busybox",
"@pex",
"--scie-busybox-pex-entrypoint-env-passthrough",
Copy link
Member Author

@jsirois jsirois Sep 10, 2024

Choose a reason for hiding this comment

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

N.B.: This means the released Pex scies, unlike the Pex PEX, support all 3 top-level Pex commands 1st class if you leave the file name as-is. For example:

:; dist/pex-linux-x86_64
Error: Could not determine which command to run.

Please select from the following boot commands:

pex
pex-tools
pex3

You can select a boot command by setting the SCIE_BOOT environment variable or else by passing it as the 1st argument.

So you can:

# Run `pex`:
:; dist/pex-linux-x86_64 pex ...

# Run `pex3`:
:; dist/pex-linux-x86_64 pex3 ...

# Run `pex-tools`:
:; dist/pex-linux-x86_64 pex-tools ...

Alternatively you can do something like:

# Install the binaries with top-level names (the `--symlink` is optional):
:; SCIE=install dist/pex-linux-x86_64 --symlink bin

# Run `pex`:
:; bin/pex ...

# Run `pex3`:
:; bin/pex3 ...

# Run `pex-tools`:
:; bin/pex-tools ...

Finally, if you re-name the downloaded Pex scie to pex, it behaves ~just like the Pex PEX and you must use env vars to access the alternate entry points; e.g: PEX_SCRIPT=pex3 pex ....

Comment on lines +108 to +113
- name: Append Hashes to Changelog
run: |
changelog_tmp="$(mktemp)"
cat "${{ steps.prepare-changelog.outputs.changelog-file }}" <(echo '***') dist/hashes.md \
> "${changelog_tmp}"
mv "${changelog_tmp}" "${{ steps.prepare-changelog.outputs.changelog-file }}"
Copy link
Member Author

Choose a reason for hiding this comment

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

The upshot of this is the Releases will get this style content between the change-log and the files list below:


file sha256 size
pex b5fb8b2469dd8f208a7805c7ec51850e81f4a4952e4808827b8e07589bc2496f 4306002
pex-linux-aarch64 1861382d2c273105a4b7013054e7acbab02f9ce78de7baa7d5e744c398eb4204 24854091
pex-linux-x86_64 e42f3231f605147ccb93118c2bb1386ca95dfac624b19cb405e91db9886cd7ad 28404512
pex-macos-aarch64 b1f18587833c9af437449dddf53dc778b1705eaace750b0f02ecee3bba5d2103 22320777
pex-macos-x86_64 fb8e9c580a9dc7ed0aae9f5b4ecad44287d4db585980400c2c3893687e545e74 22816650

Copy link
Collaborator

@benjyw benjyw left a comment

Choose a reason for hiding this comment

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

Thanks for the explanations throughout, will be helpful when integrating the PEX scies.

@@ -1375,6 +1375,8 @@ def do_main(
),
V=options.verbosity,
)
if scie_configuration.options.scie_only:
os.unlink(pex_file)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I assume this means that in certain cases of early failure the .pex file will be left behind as cruft. I imagine that is fine, but @martina-oefelein may have thoughts.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, exactly. The existing scie creation code also goes to lengths to preserve the temporary lift.toml it generates when there is a scie building failure; so this intent to leave behind the inputs to the failure for inspection is consistent. In the lift.toml case the fact that it is being plopped out is actually printed to the console.

Copy link
Member Author

Choose a reason for hiding this comment

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

@martina-oefelein I'll likely submit this tomorrow morning ~9am U.S. Pacific unless I hear negative feedback from you. One thing I know is missing is docs and I'll eventually circle back to that. My main thrust here was to enable your use case (and Pex's, which matches yours) and to get a release out that Pants can use to handle overly huge Pex cache sizes there with the new pex3 cache ... commands that are only really nice to use with the Pex scie.

Choose a reason for hiding this comment

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

Sorry, I was offline. Yes, I think leaving the .pex file behind on failure is fine.

@jsirois jsirois merged commit d8a60db into pex-tool:main Sep 12, 2024
26 checks passed
@jsirois jsirois deleted the release/pex-scies branch September 12, 2024 18:00
@jsirois
Copy link
Member Author

jsirois commented Sep 12, 2024

Alrighty, now available here: https://github.com/pex-tool/pex/releases/tag/v2.18.0

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