Switch to using the native options parser values #21385

benjyw · 2024-09-09T00:47:44Z

But still compare against the legacy parser so we can warn about
discrepancies.

But still compare against the legacy parser.

benjyw · 2024-09-09T00:48:33Z

src/python/pants/option/options.py

-        legacy_values = self.get_parser(scope).parse_args(
-            parse_args_request, log_warnings=log_parser_warnings
-        )
+        native_values = self.get_parser(scope).parse_args_native(self._native_parser)


Now we always create the native values but only create the legacy values if we need to do the discrepancy check, which is the converse of what we did before.

huonw

Sweet!

huonw · 2024-09-09T00:57:51Z

src/python/pants/option/options.py

@@ -499,8 +499,7 @@ def log(log_func):
                raise Exception(
                    "Option value mismatches detected, see logs above for details. Aborting."
                )
-        # TODO: In a future release, switch to the native_values as authoritative.
-        return legacy_values
+        return native_values


I wonder if we want to allow a user to fallback to the legacy values if things are wildly wrong. E.g. [GLOBAL].use_deprecated_legacy_options_values = true or something. Thoughts?

(Happy to get this PR merged ASAP, as is, and we can cherry-pick any tweaks along these back to 2.23.x if we decide on them)

I don't think we do - 2.22.x is where you're supposed to discover if things are wildly wrong.

cognifloyd

minor exception message ~~nitpick~~ issue. Otherwise LGTM.

cognifloyd · 2024-09-09T18:17:51Z

src/python/pants/option/options.py

+                parse_args_request = self._make_parse_args_request(flags_in_scope, values_builder)
+                legacy_values = self.get_parser(scope).parse_args(
+                    parse_args_request, log_warnings=log_parser_warnings
+                )
            except Exception as e:
                native_mismatch_msgs.append(
                    f"Failed to parse options with native parser due to error:\n    {e}"


Suggested change

f"Failed to parse options with native parser due to error:\n {e}"

f"Failed to parse options with legacy parser due to error:\n {e}"

Not a nitpick, that was actually wrong. Fixed, but also looking into the test failures, which are due to things either not failing when they should (presumably because the checks happen later or not at all on the native parser) or just failing with a slightly different error message.

- Fix missing validations on the native side. - Change the python error type raised by the native parser. - Various error types and error message differences.

benjyw · 2024-09-11T04:44:54Z

This is going to take another day or so, as I figure out one thing that the tests uncovered, namely that we still rely on the legacy parser to detect unconsumed cmd-line flags. Will have a little think on how best to address this, probably in another PR.

huonw · 2024-09-11T06:58:14Z

No worries. Is there a chance we could keep the 2.23 releases moving forward by either:

cherry-picking the switch back to 2.23.x
delaying the native->legacy switchover until 2.24.x

(My thinking here is that we're a bit stuck in a spiral of long releases because delays lead to bigger releases lead to more things that can go wrong lead to delays 😄 )

cognifloyd · 2024-09-11T15:08:36Z

I think delaying the switch to 2.24 makes the most sense. Cherry-picking back to 2.23 is fine if the issues can be resolved quickly.

benjyw · 2024-09-11T18:58:07Z

I don't want to delay, but we can cherry-pick.

huonw · 2024-09-11T21:20:22Z

Okay, I'll merge #21383 to start that rolling

benjyw · 2024-09-13T22:07:31Z

OK, tracking arg consumption is implemented here: #21410

So we can error if unrecognized args are found.

benjyw · 2024-09-18T16:11:00Z

src/python/pants/option/options_test.py

@@ -443,28 +448,6 @@ def register(opts: Options) -> None:
    assert vals2.qux == "uu"


-def test_scope_deprecation_default_config_section(caplog) -> None:


This was testing emergent behavior, not API-defined behavior. Implementing this behavior in the native parser would be tricky and IMO not worth it:

A) It's not at all clear that this is the correct behavior. Generally we treat a config entry in DEFAULT as-if it was in the relevant section, so special-casing this seems unnecessary.
B) This behavior is only triggered when we deprecate an entire scope, which we haven't done in a while, and currently have no such deprecations pending, so no user will see any effect.

benjyw · 2024-09-18T16:12:24Z

src/rust/engine/src/externs/options.rs

@@ -12,6 +11,8 @@ use options::{

 use std::collections::HashMap;

+pyo3::import_exception!(pants.option.errors, ParseError);


We now raise our own ParseError instead of the generic ValueError. However it seemed like overkill to raise a bunch of ParseError subtypes (they were probably overkill to begin with even in the legacy parser).

benjyw · 2024-09-18T16:12:45Z

This is ready for another look, thanks!

huonw

This looks reasonable.

I feel obliged to flag my discomfort with landing such a large chunk of new code so late in the release cycle, vs. just letting the switchover slip to 2.24 (and thus potentially being more likely to be able to stick to a regular/short release cycle due to fewer last-minute issues).

This is just discomfort, not a blocking concern, so lets get this in and hope for the best.

huonw · 2024-09-18T17:41:18Z

src/python/pants/option/options_test.py

+        # We set allow_unknown_options=True to ensure that it's the native
+        # parser that checks for unconsumed args, and not the legacy parser.


Why does setting this arg have this effect? It seems a bit indirect?

Ah, if we don't set it then it will fail earlier, in for_scope(), and never get to the explicit verification call.

huonw · 2024-09-18T17:43:13Z

src/python/pants/option/options_integration_test.py

@@ -27,12 +27,9 @@ def test_invalid_options() -> None:
        "ERROR] Invalid option 'bad_option' under [pytest]",
    ]

-    # We error on invalid CLI options before validating the config file.


We now validate the config file and options together?

There was a version where the order was different but it's actually not now, so I have restored this test to how it was.

cognifloyd

LGTM as long as tests pass

benjyw · 2024-09-20T02:07:46Z

src/python/pants/help/flag_error_help_printer.py

@@ -41,7 +41,9 @@ def handle_unknown_flags(self, err: UnknownFlagsError):
                possibilities.update(oshi.collect_scoped_flags())

        for flag in err.flags:
-            print(f"Unknown flag {self.maybe_red(flag)} on {err.arg_scope or 'global'} scope")
+            print(
+                f"Unknown flag {self.maybe_red(flag)} in {(err.arg_scope + ' goal') if err.arg_scope else 'global'} context"


This is more precise phrasing. We don't actually know the scope of the flag, we just know its CLI context (global position or goal position).

benjyw · 2024-09-20T02:13:38Z

@huonw thanks for expressing your discomfort, which I acknowledge. I can wait to 2.24.x if you are more comfortable with that (and it sounds like @cognifloyd prefers that too). For now I will merge (once tests pass) without the auto-cherrypick, so we have a chance to discuss further.

Switch to the native options parser values as definitive.

8b22c5c

But still compare against the legacy parser.

benjyw added the category:user api change label Sep 9, 2024

benjyw requested a review from huonw September 9, 2024 00:47

benjyw commented Sep 9, 2024

View reviewed changes

huonw approved these changes Sep 9, 2024

View reviewed changes

cognifloyd approved these changes Sep 9, 2024

View reviewed changes

Respond to failed tests:

9e3387c

- Fix missing validations on the native side. - Change the python error type raised by the native parser. - Various error types and error message differences.

huonw mentioned this pull request Sep 11, 2024

Prepare 2.23.0a0 #21383

Merged

benjyw added this to the 2.23.x milestone Sep 13, 2024

benjyw added the needs-cherrypick label Sep 13, 2024

Track consumption of command-line args in the rust parser.

2d55753

So we can error if unrecognized args are found.

benjyw mentioned this pull request Sep 13, 2024

Track consumption of command-line args in the rust parser. #21410

Merged

benjyw added 5 commits September 15, 2024 09:08

Simplify arg tracking, check it after all scopes have been consumed

f4ae6c6

Merge branch 'arg_tracker' into use_native_option_values

e57a479

Merge branch 'main' into use_native_option_values

8bfa3f1

Merge branch 'main' into use_native_option_values

5cdad96

Test tweak

0279985

benjyw commented Sep 18, 2024

View reviewed changes

huonw approved these changes Sep 18, 2024

View reviewed changes

cognifloyd approved these changes Sep 19, 2024

View reviewed changes

benjyw added 2 commits September 19, 2024 15:28

Handle goal aliases that look like flags

d71b5ae

Fixes due to test failures

7e94e3f

benjyw commented Sep 20, 2024

View reviewed changes

benjyw removed this from the 2.23.x milestone Sep 20, 2024

benjyw removed the needs-cherrypick label Sep 20, 2024

benjyw merged commit c50c8fc into pantsbuild:main Sep 20, 2024
25 checks passed

benjyw deleted the use_native_option_values branch September 20, 2024 06:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Switch to using the native options parser values #21385

Switch to using the native options parser values #21385

benjyw commented Sep 9, 2024

benjyw Sep 9, 2024

huonw left a comment

huonw Sep 9, 2024

benjyw Sep 9, 2024

cognifloyd left a comment •

edited

Loading

cognifloyd Sep 9, 2024

benjyw Sep 9, 2024

benjyw commented Sep 11, 2024

huonw commented Sep 11, 2024

cognifloyd commented Sep 11, 2024 •

edited

Loading

benjyw commented Sep 11, 2024

huonw commented Sep 11, 2024

benjyw commented Sep 13, 2024

benjyw Sep 18, 2024

benjyw Sep 18, 2024

benjyw commented Sep 18, 2024

huonw left a comment

huonw Sep 18, 2024

benjyw Sep 18, 2024

huonw Sep 18, 2024

benjyw Sep 20, 2024

cognifloyd left a comment

benjyw Sep 20, 2024

benjyw commented Sep 20, 2024

	f"Failed to parse options with native parser due to error:\n {e}"
	f"Failed to parse options with legacy parser due to error:\n {e}"

		@@ -443,28 +448,6 @@ def register(opts: Options) -> None:
		assert vals2.qux == "uu"


		def test_scope_deprecation_default_config_section(caplog) -> None:

		@@ -12,6 +11,8 @@ use options::{

		use std::collections::HashMap;

		pyo3::import_exception!(pants.option.errors, ParseError);

		# We set allow_unknown_options=True to ensure that it's the native
		# parser that checks for unconsumed args, and not the legacy parser.

Switch to using the native options parser values #21385

Switch to using the native options parser values #21385

Conversation

benjyw commented Sep 9, 2024

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

cognifloyd left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjyw commented Sep 11, 2024

huonw commented Sep 11, 2024

cognifloyd commented Sep 11, 2024 • edited Loading

benjyw commented Sep 11, 2024

huonw commented Sep 11, 2024

benjyw commented Sep 13, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjyw commented Sep 18, 2024

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

Choose a reason for hiding this comment

cognifloyd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benjyw commented Sep 20, 2024

cognifloyd left a comment •

edited

Loading

cognifloyd commented Sep 11, 2024 •

edited

Loading