Allow passing arbitrary args to PEX invocation when building FaaS artifacts #20237

huonw · 2023-11-23T22:54:23Z

This adds an pex3_venv_create_extra_args field to all FaaS targets (python_aws_lambda_function, python_aws_lambda_layer, python_google_cloud_function). This allows adding arbitrary extra arguments that are passed to the pex3 venv create --layout=flat-zipped ... invocation that is used to create the final zip file.

Most acutely, this is driven by making it possible to pass the --collisions-ok flag. This allows work around dependencies that are packaged with files outside a namespaced directories, e.g. commonly a LICENCE or README file, or tests/ directory, since they'll have different content. A command like pip install ... will happily install them and have one of the files "win" arbitrarily, while PEX is more correct and flags that it doesn't know what to do in that circumstance.

Fixes #20224

This is marked for cherry picking back to 2.18 because it can block adoption of the new layout, and the old (lambdex) layout is deprecated and using it is noisy in 2.18.

benjyw

I'm wondering if we want an option under [pex] to set this globally. And if we have that, do we need to be able to set this at the target level as well?

benjyw · 2023-11-25T23:34:55Z

src/python/pants/backend/awslambda/python/rules_test.py

@@ -342,3 +342,75 @@ def test_layer_must_have_dependencies(rule_runner: PythonRuleRunner) -> None:
            expected_extra_log_lines=("    Runtime: python3.7",),
            layer=True,
        )
+
+
+def test_collisions_ok(rule_runner: PythonRuleRunner) -> None:


Do we need these tests given the generic one in pex_venv_test.py ?

I was assuming we want to validate we've hooked up the field appropriately.

I guess technically this could be a normal test that checks the pex_venv.py rule is called with the expected args, rather than needing to be a full integration test...

Do you think it's worth switching over?

We have so many large lumbering tests, our CI is already so heavyweight, that yeah, I think it's worth keeping this light.

I've switched them all to be fully mocked, basically just testing that PexCliProcess gets the expected args.

huonw · 2023-11-26T00:53:46Z

I'm wondering if we want an option under [pex] to set this globally. And if we have that, do we need to be able to set this at the target level as well?

Hm, not a bad idea, to simplify the code.

However, personally, I'd prefer to be able to set this only on targets where it matters (i.e. this particular lambda happens to use conflicting dependencies), rather than having a blanket allow that might let an unexpected/undesirable collision slip through in a different artifact. If someone has a lot of conflicts, __defaults__ allows avoiding the repetition.

What do you think?

benjyw · 2023-11-26T19:03:57Z

I'm wondering if we want an option under [pex] to set this globally. And if we have that, do we need to be able to set this at the target level as well?

Hm, not a bad idea, to simplify the code.

However, personally, I'd prefer to be able to set this only on targets where it matters (i.e. this particular lambda happens to use conflicting dependencies), rather than having a blanket allow that might let an unexpected/undesirable collision slip through in a different artifact. If someone has a lot of conflicts, __defaults__ allows avoiding the repetition.

What do you think?

I'm not convinced we need this level of granularity yet. Note that pip, the de facto standard, silently allows collisions, so users clearly accept it as normal, and in some cases distributions expect it to work... The fact that pex is far stricter is nice-to-have, but I think a global - allow/warn/error setting might be fine to start with.

huonw · 2023-11-26T21:30:10Z

Okay, I'll add a setting like [pex] venv_create_collisions = "allow" / "error", and try to make the docs reference it so that users can find it when they have problems with their otherwise-unrelated python_aws_lambda_function targets.

huonw · 2023-11-26T23:59:05Z

Hm, alternative: what if we just allow passing extra_pex_venv_create_args=["..."] as referenced in the description, and we're likely to do for #20227?

This feels like less of a proliferation of narrow-use-case arguments, and is more clearly attached to the relevant targets.

benjyw · 2023-11-27T06:14:58Z

Hm, alternative: what if we just allow passing extra_pex_venv_create_args=["..."] as referenced in the description, and we're likely to do for #20227?

This feels like less of a proliferation of narrow-use-case arguments, and is more clearly attached to the relevant targets.

That works for me.

cedric-fauth · 2023-12-07T13:06:00Z

Hi, do you have any updates on this? Would be great if this feature will be out soon for pants 2.18. Thanks for working on it!

huonw · 2023-12-08T02:26:42Z

Sorry, had a bit of work crunch over the last weeks, so this has had to pause; maybe over the next day/week.

huonw · 2023-12-11T03:43:55Z

Okay, this now exists as the pex3_venv_create_extra_args field, including a call-out in the long-form docs about this specific failure mode because it seems to be common enough to be worth it. I've tested I can reproduce the issue from #20224 and that adding pex3_venv_create_extra_args=["--collisions-ok"], gets it working again 🎉

…ifacts (#20237) This adds an `pex3_venv_create_extra_args` field to all FaaS targets (`python_aws_lambda_function`, `python_aws_lambda_layer`, `python_google_cloud_function`). This allows adding arbitrary extra arguments that are passed to the `pex3 venv create --layout=flat-zipped ...` invocation that is used to create the final zip file. Most acutely, this is driven by making it possible to pass the `--collisions-ok` flag. This allows work around dependencies that are packaged with files outside a namespaced directories, e.g. commonly a LICENCE or README file, or `tests/` directory, since they'll have different content. A command like `pip install ...` will happily install them and have one of the files "win" arbitrarily, while PEX is more correct and flags that it doesn't know what to do in that circumstance. Fixes #20224 This is marked for cherry picking back to 2.18 because it can block adoption of the new layout, and the old (lambdex) layout is deprecated and using it is noisy in 2.18.

WorkerPants · 2023-12-11T04:22:16Z

I tried to automatically cherry-pick this change back to each relevant milestone, so that it is available in those older releases of Pants.

❌ 2.18.x

I was unable to cherry-pick this PR to 2.18.x, likely due to merge-conflicts.

Steps to Cherry-Pick locally

To resolve:

(Ensure your git working directory is clean)

Run the following script to reproduce the merge-conflicts:

git fetch https://github.com/pantsbuild/pants main \
  && git fetch https://github.com/pantsbuild/pants 2.18.x \
  && git checkout -b cherry-pick-20237-to-2.18.x FETCH_HEAD \
  && git cherry-pick 9d13e70f744c808b8691a582bfddf6e2b990a9b0

Fix the merge conflicts and commit the changes
Run build-support/cherry_pick/make_pr.sh "20237" "2.18.x"

Please note that I cannot re-run CI if a job fails. Please work with your PR approver(s) to re-run CI if necessary.

✔️ 2.19.x

Successfully opened #20280.

When you're done manually cherry-picking, please remove the needs-cherrypick label on this PR.

Thanks again for your contributions!

🤖 Beep Boop here's my run link

…ifacts (#20237) This adds an `pex3_venv_create_extra_args` field to all FaaS targets (`python_aws_lambda_function`, `python_aws_lambda_layer`, `python_google_cloud_function`). This allows adding arbitrary extra arguments that are passed to the `pex3 venv create --layout=flat-zipped ...` invocation that is used to create the final zip file. Most acutely, this is driven by making it possible to pass the `--collisions-ok` flag. This allows work around dependencies that are packaged with files outside a namespaced directories, e.g. commonly a LICENCE or README file, or `tests/` directory, since they'll have different content. A command like `pip install ...` will happily install them and have one of the files "win" arbitrarily, while PEX is more correct and flags that it doesn't know what to do in that circumstance. Fixes #20224 This is marked for cherry picking back to 2.18 because it can block adoption of the new layout, and the old (lambdex) layout is deprecated and using it is noisy in 2.18.

…ifacts (Cherry-pick of #20237) (#20280) This adds an `pex3_venv_create_extra_args` field to all FaaS targets (`python_aws_lambda_function`, `python_aws_lambda_layer`, `python_google_cloud_function`). This allows adding arbitrary extra arguments that are passed to the `pex3 venv create --layout=flat-zipped ...` invocation that is used to create the final zip file. Most acutely, this is driven by making it possible to pass the `--collisions-ok` flag. This allows work around dependencies that are packaged with files outside a namespaced directories, e.g. commonly a LICENCE or README file, or `tests/` directory, since they'll have different content. A command like `pip install ...` will happily install them and have one of the files "win" arbitrarily, while PEX is more correct and flags that it doesn't know what to do in that circumstance. Fixes #20224 This is marked for cherry picking back to 2.18 because it can block adoption of the new layout, and the old (lambdex) layout is deprecated and using it is noisy in 2.18. Co-authored-by: Huon Wilson <huon@exoflare.io>

…ifacts (Cherry-pick of #20237) (#20281) This adds an `pex3_venv_create_extra_args` field to all FaaS targets (`python_aws_lambda_function`, `python_aws_lambda_layer`, `python_google_cloud_function`). This allows adding arbitrary extra arguments that are passed to the `pex3 venv create --layout=flat-zipped ...` invocation that is used to create the final zip file. Most acutely, this is driven by making it possible to pass the `--collisions-ok` flag. This allows work around dependencies that are packaged with files outside a namespaced directories, e.g. commonly a LICENCE or README file, or `tests/` directory, since they'll have different content. A command like `pip install ...` will happily install them and have one of the files "win" arbitrarily, while PEX is more correct and flags that it doesn't know what to do in that circumstance. Fixes #20224 This is marked for cherry picking back to 2.18 because it can block adoption of the new layout, and the old (lambdex) layout is deprecated and using it is noisy in 2.18.

Similar to #20237, this allows passing arbitrary arguments to the first pex invocation when building a FaaS (AWS Lambda or Google Cloud Function) artifact. This can serve as a stop-gap for #19256, because a user can write then pass `--exclude` themselves when they really need that (or similar) functionality. --------- Co-authored-by: Huon Wilson <huon@exoflare.io>

huonw added 2 commits November 24, 2023 09:06

Add support for --collisions-ok flag for pex3 venv create invocation

2caf5e0

Plumb collisions-ok through to FaaS targets

ee00992

huonw added needs-cherrypick category:new feature labels Nov 23, 2023

huonw added this to the 2.18.x milestone Nov 23, 2023

huonw marked this pull request as ready for review November 24, 2023 00:19

huonw requested a review from benjyw November 24, 2023 00:21

huonw mentioned this pull request Nov 24, 2023

Add compress/uncompressed flag to pex_binary #20227

Open

benjyw reviewed Nov 25, 2023

View reviewed changes

huonw added 3 commits December 9, 2023 14:52

Add extra_args support to PexVenv

6093042

Switch field

c4a0107

docs

0c4e581

huonw changed the title ~~Allow passing --collisions-ok when building FaaS artifacts~~ Allow passing arbitrary args to PEX invocation when building FaaS artifacts Dec 11, 2023

benjyw approved these changes Dec 11, 2023

View reviewed changes

huonw merged commit 9d13e70 into main Dec 11, 2023

huonw deleted the huonw/20224-collisions-ok branch December 11, 2023 04:20

WorkerPants added the auto-cherry-picking-failed Auto Cherry-Picking Failed label Dec 11, 2023

huonw mentioned this pull request Dec 11, 2023

Allow silencing dependency collision warnings when packaging python_aws_lambda_function #20224

Closed

huonw removed the needs-cherrypick label Jan 7, 2024

thejcannon mentioned this pull request Jan 12, 2024

Fold pants#20237 into 2.18/2.19 pantsbuild/pantsbuild.org#94

Closed

huonw mentioned this pull request Feb 13, 2024

Allow updating docs from unreleased pants changes, not just release pantsbuild/pantsbuild.org#148

Open

huonw mentioned this pull request May 21, 2024

Allow Extra Args to be passed to FaaS Pex Build #20939

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Allow passing arbitrary args to PEX invocation when building FaaS artifacts #20237

Allow passing arbitrary args to PEX invocation when building FaaS artifacts #20237

Uh oh!

huonw commented Nov 23, 2023 •

edited

Loading

Uh oh!

benjyw left a comment

Uh oh!

benjyw Nov 25, 2023

Uh oh!

huonw Nov 26, 2023

Uh oh!

benjyw Nov 26, 2023

Uh oh!

huonw Dec 11, 2023

Uh oh!

huonw commented Nov 26, 2023

Uh oh!

benjyw commented Nov 26, 2023

Uh oh!

huonw commented Nov 26, 2023

Uh oh!

huonw commented Nov 26, 2023

Uh oh!

benjyw commented Nov 27, 2023

Uh oh!

cedric-fauth commented Dec 7, 2023

Uh oh!

huonw commented Dec 8, 2023

Uh oh!

huonw commented Dec 11, 2023

Uh oh!

WorkerPants commented Dec 11, 2023

Uh oh!

Uh oh!

Uh oh!

Allow passing arbitrary args to PEX invocation when building FaaS artifacts #20237

Allow passing arbitrary args to PEX invocation when building FaaS artifacts #20237

Uh oh!

Conversation

huonw commented Nov 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benjyw left a comment

Choose a reason for hiding this comment

Uh oh!

benjyw Nov 25, 2023

Choose a reason for hiding this comment

Uh oh!

huonw Nov 26, 2023

Choose a reason for hiding this comment

Uh oh!

benjyw Nov 26, 2023

Choose a reason for hiding this comment

Uh oh!

huonw Dec 11, 2023

Choose a reason for hiding this comment

Uh oh!

huonw commented Nov 26, 2023

Uh oh!

benjyw commented Nov 26, 2023

Uh oh!

huonw commented Nov 26, 2023

Uh oh!

huonw commented Nov 26, 2023

Uh oh!

benjyw commented Nov 27, 2023

Uh oh!

cedric-fauth commented Dec 7, 2023

Uh oh!

huonw commented Dec 8, 2023

Uh oh!

huonw commented Dec 11, 2023

Uh oh!

WorkerPants commented Dec 11, 2023

❌ 2.18.x

✔️ 2.19.x

Uh oh!

Uh oh!

huonw commented Nov 23, 2023 •

edited

Loading