Change options declarations to use inner-classes

[thejcannon]

I want to fix #18856, which has a secondary benefit of making rule inputs more fine-grained.

The only way to accomplish that is by converting options declarations to be types, so that they are valid annotations.

As a strawman, that'd look something like:

class GlobalOptions(BootstrapOptions, Subsystem):
    options_scope = GLOBAL_SCOPE
    help = "Options to control the overall behavior of Pants."

    @bool_option
    class Colors:
        default = sys.stdout.isatty()
        help = softwrap(
            """
            Whether Pants should use colors in output or not. This may also impact whether
            some tools Pants runs use color.

            When unset, this value defaults based on whether the output destination supports color.
            """
        )

which was previously:

    colors = BoolOption(
        default=sys.stdout.isatty(),
        help=softwrap(
            """
            Whether Pants should use colors in output or not. This may also impact whether
            some tools Pants runs use color.

            When unset, this value defaults based on whether the output destination supports color.
            """
        ),
    )

Then rule code needing the option would use:

@rule
def my_rule(colors_container: GlobalOptions.Colors) -> ...:
    colors_container.value  # known to be type `bool`
    # And we'd also have `colors_container.is_default_value` of type `bool`
    #    instead of `subsystem.options.is_default("<name>")`

This would extend to environment-aware options as well (although the details are fuzzy to me)

And this also gives us a place to provide argument conversion (E.g. the class can define a convert_value)

One downside is that helper methods on subsystems would be harder to call. E.g. my_subsystem.super_cool_method(...) would likely be class methods taking the individual options: MySubsystem.super_cool_method(..., ..., ...)

[thejcannon]

Only downside i can see so far is there might not be a way to enforce the class attributes type-wise.

E.g. if you forget to define help, mypy won't yell at you.

[thejcannon]

UGH python/mypy#3135

[thejcannon]

So I guess the only mypy-compliant way would be subclassing

[huonw]

One option might be __init_subclass__ taking all args that BoolOption (etc.) take directly now, which might strike a balance between setting properties on subclasses and difficulties with decorators: https://docs.python.org/3/reference/datamodel.html#customizing-class-creation

class GlobalOptions(BootstrapOptions, Subsystem):
    options_scope = GLOBAL_SCOPE
    help = "Options to control the overall behavior of Pants."

    class Colors(
        BoolOption,
        default=sys.stdout.isatty(),
        help=softwrap("""..."""),
    ):
         pass # or whatever other methods would be helpful

It's not particularly pretty but I think it'll probably give the best type checker integration, with the usual function-arg-like static checks rather than having to rely on the dynamic checks in a decorator. https://mypy-play.net/?mypy=latest&python=3.9&gist=05bcb6808456aa4f3952b45e59804ff4

Another option would be ABCs with abstract classvars, maybe? But I'm not sure how to get that working.

[thejcannon]

I think @kaos protocol idea would likely work

[cognifloyd]

Looks like PyCharm doesn't support static typing for class decorators either, though the issue is marked "in progress": https://youtrack.jetbrains.com/issue/PY-30190/Inspection-cancelled-when-I-decorate-a-class.

[Eric-Arellano]

Would you expect rule authors to always define the specific options they want as rule inputs (#18856)?

@stuhood, any idea if this would blow up the rule graph in a bad way? Now we have one rule per option, rather than one per subsystem.

[thejcannon]

Yeah, that's the intent. We could also look into disallowing an entire subsystem as a rule input (either by not providing it in the graph, or just checking in the rule decorator)

[thejcannon]

Actually, the subsystem as a rule input would be very annoying to use, as you'd have to go through subsystem.options.<name> like before.

[Eric-Arellano]

I lean towards preferring having only one way to do it. We want to nudge people to write their rules the best way.

[thejcannon]

Without additional work, it'd be possible but annoying. I think that's an OK trade-off, primarily because we still need a way to get the subsystem as a whole (the auto-generated rule which takes in the subsystem and returns the option container)

[stuhood]

@stuhood, any idea if this would blow up the rule graph in a bad way? Now we have one rule per option, rather than one per subsystem.

It should be fine, as each of these types would only have one possible source.

[Eric-Arellano]

How do you think this would interact with MySubsystem.EnvironmentAware? That is our mechanism for defining options that depend on the environment (e.g. docker vs. RE vs local). MySubsystem.EnvironmentAware.MyOption?

[thejcannon]

Yup! There isn't much special about that container, so the same concepts on the subsystem apply there.

[tobni]

Considered typing.Annotated? I imagine that'd let you keep options as is.

@rule
def my_rule(colors_container: Annotated[BoolOption, GlobalOptions.colors]) -> ...:

[thejcannon]

I honestly think that'd be a bit harder to get working, and also much more verbose.

So I like the creativity, but I don't think that'd fit very nicely

[thejcannon]

Ah and for the record, changing the existing descriptors to return a type instance when objtype is None (e.g. when called on the class) doesn't pass mypy's sniff-test:

@rule
def whatever(colors_container: GlobalOptions.color) -> ...:
    ...

results in:

rc/python/pants/backend/docker/goals/package_image.py: note: In function "whatever":
src/python/pants/backend/docker/goals/package_image.py:359: error: Name "GlobalOptions.color" is not defined  [name-defined]
        colors_container: GlobalOptions.color,

[thejcannon]

Pyright does support this though 😂

[thejcannon]

Here's the output:

from typing import Any, overload, Type

class MyCoolTHing:
    ...

class MyDescriptor:

    @overload
    def __get__(self, obj: None, objtype: Any) -> Type[MyCoolTHing]:
        ...

    @overload
    def __get__(self, obj: object, objtype: Any) -> bool:
        ...

    def __get__(self, obj, objtype) -> Type[MyCoolTHing] | bool:
        ...



class Foo:
    bar = MyDescriptor()



reveal_type(Foo.bar)
reveal_type(Foo().bar)

def foo(variable: Foo.bar):
    reveal_type(variable)

/home/josh/work/pants/src/python/pants/option/foo.py
  /home/josh/work/pants/src/python/pants/option/foo.py:26:13 - information: Type of "Foo.bar" is "Type[MyCoolTHing]"
  /home/josh/work/pants/src/python/pants/option/foo.py:27:13 - information: Type of "Foo().bar" is "bool"
  /home/josh/work/pants/src/python/pants/option/foo.py:30:17 - information: Type of "variable" is "MyCoolTHing"

[thejcannon]

(I still would advocate for the decorator though. The descriptor is kinda nasty)

[cognifloyd]

class GlobalOptions(BootstrapOptions, Subsystem):
    options_scope = GLOBAL_SCOPE
    help = "Options to control the overall behavior of Pants."

    class Colors(BoolOption):
        default = sys.stdout.isatty()
        help = softwrap(
            """
            Whether Pants should use colors in output or not. This may also impact whether
            some tools Pants runs use color.

            When unset, this value defaults based on whether the output destination supports color.
            """
        )

I think I like subclassing because it is similar to defining a target Field.

BoolOption could have a metaclass that uses __new__ to reach into the encapsulating subsystem to perform additional registration logic if needed.

Why do you prefer a decorator over a subclass?

[thejcannon]

Inheritance is not a straightforward concept, and also can be misused. There's also acrobatics to do the validation, like __new__ or __init_subclass__. As an example, BoolOption is a subclass of the OptionsBase, does that make BoolOption itself an option? What about a PathOption which inherits from StrOption?

On the other hand, the decorator is explicit.

...in the end I expect we likely will just use the base class for ease of typechecking, but I'll do so begrudgingly.

[thejcannon]

Actually, in playing around with this, we really want the "constructor" thing to be a function, because functions have type inference. So we might have to jump to Pyright

[stuhood]

I don't feel strongly about this either way, but I like the improved granularity of option requests.

One relevant detail is that because Subsystems are currently the only type with generated @rules to construct them, they are the only place (iirc) where #18905 doesn't have a @rule/function "name" to refer to. This change would be more of the same... but it would make it challenging to ever use individual options "lazily" in a @rule body (I can't think of a use case for that currently though: lazy requests for types have typically been used to avoid actual work: finding binaries, etc).

[thejcannon]

FWIW, this change aligns with the other places we generate "inner" rules. So this would bring the number of ways we create nameless rules from 2 to 1. Then we'd only need to figure out how "inner" rules are requested in the new world instead of both "inner" rules AND subsystems. (And I suspect, as likely you do too, that unions will be the answer)