use explicit Pex options for package "binary-ness" #20598

cburroughs · 2024-02-24T01:57:12Z

Previously to pass along only/no-binary options Pants would create a "requirements" file and use Pip's ability to put cli args in said file [1]. This worked fine to generate the artifacts in a lockfile from scratch, but meant that the lockfile itself did not contain sufficient information to carry its own configuration forward. That is if one did pex3 lock update on a Pants created lockfile that originally required everything to use wheels, that specification could not be preserved in the updated lockfile. Since v2.1.161 [2] Pex has provided --only-{wheel,build} options to support this directly.

[1] https://pip.pypa.io/en/stable/reference/requirements-file-format/#global-options

[2] https://github.com/pex-tool/pex/releases/tag/v2.1.161

ref #15704

Previously to pass along only/no-binary options Pants would create a "requirements" file and use Pip's ability to put cli args in said file [1]. This worked fine to generate the artifacts in a lockfile from scratch, but meant that the lockfile itself did not contain sufficient information to carry its own configuration forward. That is if one did `pex3 lock update` on a Pants created lockfile that originally required everything to use wheels, that specification could not be preserved in the updated lockfile. Since v2.1.161 [2] Pex has provided `--only-{wheel,build}` options to support this directly. [1] https://pip.pypa.io/en/stable/reference/requirements-file-format/#global-options [2] https://github.com/pex-tool/pex/releases/tag/v2.1.161 ref pantsbuild#15704

cburroughs · 2024-02-24T01:59:05Z

I don't think this needs to be "solved now", but one future question here is if we ought to do anything in the future when using lock update/sync is to make sure the lockfile was generated after this change.

cburroughs · 2024-02-24T02:00:01Z

src/python/pants/backend/python/util_rules/pex_requirements.py

+        # options.  Note that Pex's --wheel (for example) means "allow
+        # wheels", not "require wheels".
+        if self.only_binary and ":all:" in self.only_binary:
+            yield "--wheel"


There are several available synonyms available from Pex here.

cburroughs · 2024-02-24T02:00:40Z

src/python/pants/backend/python/util_rules/pex_requirements.py

+            yield "--no-build"
+        elif self.only_binary and ":none:" in self.only_binary:
+            yield "--no-wheel"
+            yield "--build"


I leaned into "explicit is better than implicit" for these "redundant" pairs.

huonw

Nice!

Is it worth some sort of a smoke test that actually passes the flags to Pex, to validate that, for instance, we've got the right names for them? I'm not sure.

huonw · 2024-02-27T02:34:48Z

src/python/pants/backend/python/util_rules/pex_requirements_test.py

@@ -347,3 +348,78 @@ def test_pex_lockfile_requirement_count() -> None:
        )
        == 3
    )
+
+
+class TestResolvePexConfigPexArgs:


Pants doesn't usually use these sort of classes to group tests, instead of just top-level test functions. What's the thinking with this approach here?

Agreed. This is remnant of a unittest style era, not all tests have been refactored to the pytest style yet, so those old ones that still exist shouldn't be used as role models :) c.f. #13208

I'm a little confused here. Python is an object oriented language, Pants does not otherwise eschew objects, and grouping tests with classes is a common idiom pytest has right in the getting started guide.

It's all dicts and functions somewhere down there to Python, you can totally do OOP-y stuff without "objects" as such (Hello C!), and the silicon doesn't care. So if Pants has a style guide that has some reason for not using objects -- or no reason! that's fine too! -- I'd know how to adjust. But in this case I did it this way because an idiomatic way to test the methods of a class is a test class with methods.

Ah sorry, I'm not sure it's explicitly covered by https://www.pantsbuild.org/2.20/docs/contributions/development/style-guide#tests, given it falls somewhere between the two examples there (not unittest, but also not covered by the "Good" example), but class-less seems to be the implicit style. I don't think it's worth blocking this PR, but @kaos is the old timer with more of the history here so his vote is stronger 😄

I'll wait until tomorrow and get it in if we don't hear (given how small it is).

Ah, right. yea the current style is class-less, but I see what you mean. (at first I failed to observe this was new tests altogether..)

I'm OK with using a bare class (i.e. not a TestCase derived one) to group related tests under. We should however update the style guide accordingly to cover this regardless of outcome.

Perhaps take a vote in #development slack?

I'm gonna merge this. It sounds like this is perfectly fine for pytest, and it's a bit silly to block a PR on the basis of something small like this.

@kaos I don't feel strongly enough about this to spend my energy defining the style here. I assume you will do that if you want to? But, if you do so and the result is that we need to come back and change this, I'm happy to do it (since I'm the one doing the merge).

perhaps not very clear, but I was in favour of allowing this style for tests, and agree we need not hold this PR for the sake of it. I can run a little poll to formalize the style docs.

huonw · 2024-02-27T02:35:59Z

src/python/pants/backend/python/util_rules/pex_requirements_test.py

+        args = self.simple_config_args(only_binary=[":all:"])
+        assert "--wheel" in args
+        assert "--no-build" in args
+        assert "--only-binary" not in " ".join(self.simple_config_args(only_binary=[":all:"]))


Would you consider this:

Suggested change

assert "--only-binary" not in " ".join(self.simple_config_args(only_binary=[":all:"]))

assert "--only-binary" not in " ".join(args)

And similarly pulling out the common values in test_only_binary_none & test_no_binary_none?

cburroughs · 2024-02-27T17:48:54Z

Nice!

Thanks!

Is it worth some sort of a smoke test that actually passes the flags to Pex, to validate that, for instance, we've got the right names for them? I'm not sure.

I lean in this instance towards probably not, Pex isn't going to change the names out on a whim.

More generally as I've been making changes relative to lockfiles, it would be really nice if we already had built up test infrastructure to have a local "frozen" index of PyPi (perhaps reminiscent of pypa/pip#12275) so we could have stable end-to-end tests of lockfile generation. I have little sense at this time where that falls between "commit a json file" and "totally intractable".

huonw · 2024-02-28T09:58:35Z

I lean in this instance towards probably not, Pex isn't going to change the names out on a whim.

Ah, yeah, I guess I'm more thinking about Pants making a mistake, whether that's a future refactoring introducing a typo in the arg strings or making them strung together incorrectly, rather than doing a Pex upgrade that breaks it (although Pants' test suite has caught accidental regressions in Pex before, so it's not impossible).

I agree it's unlikely though. Happy enough without, now that we've had the discussion.

More generally as I've been making changes relative to lockfiles, it would be really nice if we already had built up test infrastructure to have a local "frozen" index of PyPi (perhaps reminiscent of pypa/pip#12275) so we could have stable end-to-end tests of lockfile generation. I have little sense at this time where that falls between "commit a json file" and "totally intractable".

Interesting idea. I imagine one might be able to do something with Pex's --no-pypi and --find-links / --index args.

Changelog: https://github.com/pex-tool/pex/releases/tag/v2.3.0 - `sync` of interest for pantsbuild#15704 - error message clarification regarding pantsbuild#15062 - fix for explicit flags as implemented pantsbuild#20598 ``` Lockfile diff: 3rdparty/python/user_reqs.lock [python-default] == Upgraded dependencies == asgiref 3.7.2 --> 3.8.1 cryptography 42.0.3 --> 42.0.5 pex 2.2.1 --> 2.3.0 pyparsing 3.1.1 --> 3.1.2 python-dateutil 2.8.2 --> 2.9.0.post0 sniffio 1.3.0 --> 1.3.1 ```

Changelog: https://github.com/pex-tool/pex/releases/tag/v2.3.0 - `sync` of interest for #15704 - error message clarification regarding #15062 - fix for explicit flags as implemented #20598 ``` Lockfile diff: 3rdparty/python/user_reqs.lock [python-default] == Upgraded dependencies == asgiref 3.7.2 --> 3.8.1 cryptography 42.0.3 --> 42.0.5 pex 2.2.1 --> 2.3.0 pyparsing 3.1.1 --> 3.1.2 python-dateutil 2.8.2 --> 2.9.0.post0 sniffio 1.3.0 --> 1.3.1 ```

cburroughs self-assigned this Feb 24, 2024

cburroughs added category:new feature backend: Python Python backend-related issues labels Feb 24, 2024

cburroughs commented Feb 24, 2024

View reviewed changes

cburroughs requested a review from huonw February 24, 2024 02:00

huonw approved these changes Feb 27, 2024

View reviewed changes

dry

2e5837e

huonw merged commit f66f92f into pantsbuild:main Feb 28, 2024
24 checks passed

huonw mentioned this pull request Mar 7, 2024

upgrade Pex to 2.2.1 (Cherry-pick of #20587, etc.) #20653

Merged

cburroughs mentioned this pull request Mar 29, 2024

upgrade Pex to 2.3.0 #20734

Merged

This was referenced Jun 21, 2024

"Resolving wheels disallowed" error when using --lock and --only-binary pex-tool/pex#2432

Closed

Using resolves_to_only_binary causes "Resolving wheels was disallowed..." error in 2.21 #21077

Closed

purajit mentioned this pull request Jun 26, 2024

Bump default terraform version to 1.9.0, add all known terraform versions inbetween #21110

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use explicit Pex options for package "binary-ness" #20598

use explicit Pex options for package "binary-ness" #20598

cburroughs commented Feb 24, 2024

cburroughs commented Feb 24, 2024

cburroughs Feb 24, 2024

cburroughs Feb 24, 2024

huonw left a comment

huonw Feb 27, 2024

kaos Feb 27, 2024

cburroughs Feb 27, 2024

huonw Feb 28, 2024 •

edited

Loading

kaos Feb 28, 2024

huonw Feb 28, 2024

kaos Feb 29, 2024

huonw Feb 27, 2024

cburroughs commented Feb 27, 2024

huonw commented Feb 28, 2024 •

edited

Loading

	assert "--only-binary" not in " ".join(self.simple_config_args(only_binary=[":all:"]))
	assert "--only-binary" not in " ".join(args)

use explicit Pex options for package "binary-ness" #20598

use explicit Pex options for package "binary-ness" #20598

Conversation

cburroughs commented Feb 24, 2024

cburroughs commented Feb 24, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huonw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huonw Feb 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cburroughs commented Feb 27, 2024

huonw commented Feb 28, 2024 • edited Loading

huonw Feb 28, 2024 •

edited

Loading

huonw commented Feb 28, 2024 •

edited

Loading