stop swallowing warnings from Pex by default #20480

cburroughs · 2024-02-01T20:31:57Z

Pants has traditionally run Pex with --no-emit-warnings which as the name implies... doesn't emit warnings. This hides pertinent information like "the pex file you created won't actually work" from the user and sends them off on a debugging hunt.

Create an emit_warnings option, defaulting to True like the underlying tool..
A little logic for pipeing Pex's stderr to Pants' logging.
Fix the logic for old per target emit_warnings that didn't quite work.

ref #19957

Pants has traditionally run Pex with --no-emit-warnings which as the name implies... doesn't emit warnings. This hides pertinent information like "the pex file you created won't actually work" from the user and sends them off on a debugging hunt. * Create an `emit_warnings` option, defaulting to True like the underlying tool.. * A little logic for pipeing Pex's stderr to Pants' logging. ref pantsbuild#19957

cburroughs · 2024-02-01T21:02:13Z

Hmm hold on, we also have PexEmitWarningsField(TriBoolField)?

cburroughs · 2024-02-01T22:09:30Z

I think the answer is just.... the field version doesn't work:

    def generate_additional_args(self, pex_binary_defaults: PexBinaryDefaults) -> Tuple[str, ...]:
        args = []
        if self.emit_warnings.value_or_global_default(pex_binary_defaults) is False:
            args.append("--no-emit-warnings")

There is no logic that True into --emit-warnings

You either get --no-emit-warnings (default) or --no-emit-warnings --no-emit-warnings with emit_warnings=False on the target.

cburroughs · 2024-02-02T03:11:13Z

Being able to set emit_warnings per pex_binary per pex_binary would be kind of nice in some cases but this appears to have "never worked" in at least two days.

It never set the value to true.
Pants would still swallow stderr

So I'm inclined to go with deprecating the per target way. I'm a little uncertain about how common warnings are in practice since Pants has been swallowing them.

cognifloyd

I think the answer is just.... the field version doesn't work:
    def generate_additional_args(self, pex_binary_defaults: PexBinaryDefaults) -> Tuple[str, ...]:
        args = []
        if self.emit_warnings.value_or_global_default(pex_binary_defaults) is False:
            args.append("--no-emit-warnings")
There is no logic that True into --emit-warnings

You either get --no-emit-warnings (default) or --no-emit-warnings --no-emit-warnings with emit_warnings=False on the target.

What about fixing that field logic so people can debug their pex_binary targets independently from everything else in pants that also runs via pex? I still think the global flag on the subsystem will be beneficial for everything else pants does with pex, but I'm not sure deprecating the field is a good idea. The logic could probably look like:

    def generate_additional_args(self, pex_binary_defaults: PexBinaryDefaults) -> Tuple[str, ...]:
        args = []
        if self.emit_warnings.value_or_global_default(pex_binary_defaults) is False:
            args.append("--no-emit-warnings")
        if self.emit_warnings.value_or_global_default(pex_binary_defaults) is True:
            args.append("--emit-warnings")

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

huonw

Thanks for this!

src/python/pants/backend/python/target_types.py

huonw · 2024-02-06T23:05:53Z

src/python/pants/backend/python/target_types.py

@@ -545,6 +564,19 @@ class PexEmitWarningsField(TriBoolField):
    def value_or_global_default(self, pex_binary_defaults: PexBinaryDefaults) -> bool:
        if self.value is None:
            return pex_binary_defaults.emit_warnings
+        warn_or_error(
+            "2.21.0.dev0",


This is a relatively small amount of code, so maybe we could extend the deprecation for longer, to be nicer to users/make it easier to upgrade (e.g. if someone is using 2.19.0, and then testing major features/fixes in a hypothetical 2.20.0rc3 and 2.21.0.dev4, it's nice if they're not completely blocked until they make changes for "minor" stuff like shuffling fields around, which removing this in 2.21.0.dev0 would force.)

Suggested change

"2.21.0.dev0",

"2.23.0.dev0",

Thoughts?

Given the fix-instead-of-deprecate option shot per other review feedback.

huonw · 2024-02-06T23:07:05Z

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

+    log_output = stderr.decode()
+    if log_output and pex_verbosity > 0:
+        logger.info("%s", log_output)
+    elif log_output and "PEXWarning:" in log_output:


What do you think about potentially being even more lax here: I would think showing more warnings is better than missing some, e.g. just check for warning (case insensitively)?

I'm worried about making a wishy washy guess that someday might cause someone to have to think about unicode case folding in a whacky corner case. I'd rather either have the PEXWarning check, or stderr unconditionally.

cognifloyd

LGTM

huonw

Nice one!

cburroughs · 2024-02-14T20:21:41Z

src/python/pants/backend/python/goals/package_pex_binary_integration_test.py

                )

                pex_binary(
                    dependencies=[':wheel'],
                    entry_point="none",
                    include_sources=False,
+                    interpreter_constraints=["CPython==3.10.*"]


Sorry missed that this test was failing in the pipeline report.

These tighter constarints avoid

ARNING pants.backend.python.util_rules.pex_cli:pex_cli.py:217 /home/ecsb/.cache/pants/named_caches/pex_root/installed_wheels/f52d67567b65f94aedbde9c10f417df9df1c78a2568cbc7820706a7b3ee6ace2/pex-2.1.159-py2.py3-none-any.whl/pex/bin/pex.py:923: PEXWarning: Could not calculate a targeted shebang for: cp37-cp37m-manylinux_2_37_x86_64 interpreter at /usr/local/bin/python3.7 cp38-cp38-manylinux_2_37_x86_64 interpreter at /usr/bin/python3.8 cp39-cp39-manylinux_2_37_x86_64 interpreter at /home/ecsb/.cache/pants/named_caches/pex_root/venvs/f2a0eb4c2e2336b7e77df777bff977abf3ca390a/d56abaad3dd7630971f10921587fc3ac5e2cee3c/bin/python3.9 cp39-cp39-manylinux_2_37_x86_64 interpreter at /usr/bin/python3.9 Using shebang: #!/usr/bin/env python3.7 If this is not appropriate, you can specify a custom shebang using the --python-shebang option. pex_warnings.warn(

which in term allows the "there is no output" assertion in the test to pass.

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

@cburroughs

This is a short-term workaround for #20577 and #20586, where Pants' internal/default use of Pex triggers user-visible warnings, and those warnings are now visible due to #20480... so, instead of showing them by default, let's hide them for now. Users with desire for insight into the warnings can still enable this. Doing this means we're not having to rush in fixes for the root causes of these warnings for 2.20.0 stable release, and thus reduce feature-creep/risk. We can/should reenable warnings by default in a future release. Per @cburroughs's idea in #20586 (comment), this explicitly switches it on for the Pants repo itself, so we're eating our own dogfood and catching real and/or spurious errors earlier.

@cburroughs

This is a short-term workaround for #20577 and #20586, where Pants' internal/default use of Pex triggers user-visible warnings, and those warnings are now visible due to #20480... so, instead of showing them by default, let's hide them for now. Users with desire for insight into the warnings can still enable this. Doing this means we're not having to rush in fixes for the root causes of these warnings for 2.20.0 stable release, and thus reduce feature-creep/risk. We can/should reenable warnings by default in a future release. Per @cburroughs's idea in #20586 (comment), this explicitly switches it on for the Pants repo itself, so we're eating our own dogfood and catching real and/or spurious errors earlier.

@cburroughs

…#20593) This is a short-term workaround for #20577 and #20586, where Pants' internal/default use of Pex triggers user-visible warnings, and those warnings are now visible due to #20480... so, instead of showing them by default, let's hide them for now. Users with desire for insight into the warnings can still enable this. Doing this means we're not having to rush in fixes for the root causes of these warnings for 2.20.0 stable release, and thus reduce feature-creep/risk. We can/should reenable warnings by default in a future release. Per @cburroughs's idea in #20586 (comment), this explicitly switches it on for the Pants repo itself, so we're eating our own dogfood and catching real and/or spurious errors earlier. Co-authored-by: Huon Wilson <huon@exoflare.io>

cburroughs added category:bugfix Bug fixes for released features backend: Python Python backend-related issues labels Feb 1, 2024

cburroughs marked this pull request as draft February 1, 2024 21:23

deprecate old way

3cb4995

cburroughs self-assigned this Feb 2, 2024

cburroughs marked this pull request as ready for review February 2, 2024 03:12

cburroughs changed the title ~~stop swalling warnings from Pex by default~~ stop swallowing warnings from Pex by default Feb 5, 2024

cognifloyd reviewed Feb 6, 2024

View reviewed changes

src/python/pants/backend/python/util_rules/pex_cli.py Outdated Show resolved Hide resolved

huonw mentioned this pull request Feb 6, 2024

plumb through Pex's --check zipapp validation #20481

Merged

huonw reviewed Feb 6, 2024

View reviewed changes

cburroughs added 3 commits February 7, 2024 11:32

drop spurios cross branch copy

cee1f14

roll back deprecate

9819b0d

try to fix old boolean logic

b064a69

cburroughs requested review from huonw and cognifloyd February 7, 2024 17:45

cognifloyd approved these changes Feb 7, 2024

View reviewed changes

huonw approved these changes Feb 7, 2024

View reviewed changes

warnings be gone!

6633bee

cburroughs commented Feb 14, 2024

View reviewed changes

kaos reviewed Feb 14, 2024

View reviewed changes

src/python/pants/backend/python/util_rules/pex_cli.py Outdated Show resolved Hide resolved

invert conditional

fe1c470

kaos approved these changes Feb 15, 2024

View reviewed changes

kaos merged commit b3072b8 into pantsbuild:main Feb 15, 2024
24 checks passed

This was referenced Feb 19, 2024

Building internal PEXes for built-in tools with [pex].emit_warnings shows warnings that seem useless #20577

Open

Silence warnings from Pex by default, for now #20590

Merged

WorkerPants mentioned this pull request Feb 22, 2024

Silence warnings from Pex by default, for now (Cherry-pick of #20590) #20593

Merged

huonw mentioned this pull request Feb 29, 2024

Expose pex's --check validation #19957

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

stop swallowing warnings from Pex by default #20480

stop swallowing warnings from Pex by default #20480

cburroughs commented Feb 1, 2024 •

edited

Loading

cburroughs commented Feb 1, 2024

cburroughs commented Feb 1, 2024

cburroughs commented Feb 2, 2024

cognifloyd left a comment

huonw left a comment

huonw Feb 6, 2024

cburroughs Feb 7, 2024

huonw Feb 6, 2024

cburroughs Feb 7, 2024

cognifloyd left a comment

huonw left a comment

cburroughs Feb 14, 2024

stop swallowing warnings from Pex by default #20480

stop swallowing warnings from Pex by default #20480

Conversation

cburroughs commented Feb 1, 2024 • edited Loading

cburroughs commented Feb 1, 2024

cburroughs commented Feb 1, 2024

cburroughs commented Feb 2, 2024

cognifloyd left a comment

Choose a reason for hiding this comment

huonw left a comment

Choose a reason for hiding this comment

huonw Feb 6, 2024

Choose a reason for hiding this comment

cburroughs Feb 7, 2024

Choose a reason for hiding this comment

huonw Feb 6, 2024

Choose a reason for hiding this comment

cburroughs Feb 7, 2024

Choose a reason for hiding this comment

cognifloyd left a comment

Choose a reason for hiding this comment

huonw left a comment

Choose a reason for hiding this comment

cburroughs Feb 14, 2024

Choose a reason for hiding this comment

cburroughs commented Feb 1, 2024 •

edited

Loading