Dynamic defaults for target field values

[kaos]

Design doc: https://docs.google.com/document/d/1KHkAUonCCMrdI7TqEx06mUpPZqItXJBwlCD3tBz5-xw/edit#

Starting a discussion about how we want to support defaults that span multiple targets and/or BUILD files.
PR: #15836
Relates to #13767 and #15803.

I've found long winded comments in Google docs hard to follow, so I think we're better served to keep the discussion going here for the general direction, and leave room for comments on the document itself for grammar nits and structural feedback. Let's try that, thank you :)

[thejcannon]

I think most of the core of the discussion will likely be focused on syntax, as (I believe) almost everyone will be on board with the core idea and the implementation discussion can be handled in PR.

[thejcannon]

Thoughts on syntax:

• I think (and this might be technically challenging, but worth it) we should enforce defaults targets are the first targets in a BUILD file. The reasoning is to eliminate any user "surprises" or bad assumptions about a target above the defaults not inheriting the defaults.
• Can we call the object just fields? I know in isolation is looks weird, but in context its duplicitous to have defaults there twice.
• Alternatively, what if we just used a raw dict? These are already a collection of metadata and using a dict allows users to do custom things.
• Should we try and make this stand out visually? Something like __defaults__(...)?

[thejcannon]

FYI "That reduces the boilerplate, but it hurts readability slightly, imo." you can just as easily use dict(...)

So

...
  python_sources=dict(sources=["*.py"]),
...

(I'm a fan of dict when the keys are valid identifiers ;) )

[kaos]

Yeah, that is better than the {"key": "value"} syntax.

[thejcannon]

Also we could eliminate default_fields if we had two "targets" (meta-targets?) __target_defaults__ and __field_defaults__ (somewhat bikeshed on names)

The former takes kwarg mapping a target to the mapping of field to defaults, the ladder takes mapping of field to default

So...

__field_defaults__(
    tags = ["some-tag"], 
    description = "dunno why I'd want to default the description for all targets here..",
)

__target_defaults__(
    python_sources=dict(...),
    ....
)

Alternate names:

• target_defaults and all_target_defaults?
• ...

[thejcannon]

Having two targets also eliminates any confusion (IMO) on if the default_fields positional value also applies to any default_fields as values of a key-value pair. We could enforce __field_defaults__ appears before __target_defaults__ (just like we want to enforce any of these comes before real targets) so that the user is most likely to assume correctly.

... wait we didn't discuss what the semantics of this are 😂 Gonna make new thread.

[kaos]

Yeah, also see latest updates to doc where I've dropped the positional syntax in favour of a all_targets key instead.

[thejcannon]

🧵 Do the default values given in in the positional default_fields get overriden by any defaults provided in a "mapped" default_fields?

My vote is definitely. It allows for intuitive overriding of defaults within the defaults overrider (woof)

If +1, we should call this out explicitly in doc.

[kaos]

I'm not convinced we want/need two meta-targets. But I agree that if you provide defaults for say, tags, on both all targets and for a specific target, that the ladder should override the former, being more specific.

[Eric-Arellano]

It would be neat if we could support multiple args, similar to overrides field:

__defaults__({
   "go_package": {"tags": ["go"]},
   ("python_source", "python_test"): {"tags": ["py"]},
})

It's proven very useful with overrides to have the tuple syntax for DRY.

I like this syntax also because it's 100% aligned to overrides. Less for us to teach. Especially because not all Pants users are Python devs, that is valuable.

[Eric-Arellano]

One tricky thing here is target generators vs atom targets. Do we expect people to set the override for python_source or for python_sources?

I think in practice, it shouldn't matter much because the python_sources value will be used when generating python_source targets...

Only probably matters if users are manually declaring python_source atom targets. Thus, it's probably safest to recommend setting to the atom target.

[kaos]

I would leave it at teaching about the nuance. What is best may differ from case to case so would leave it up to the user to decide what suits best for their project. (and could even mix and match in different parts fwiw)

[stuhood]

Overall, I really love the proposal: thanks!

I left one comment related to whether we can actually eliminate global/built-in defaults for things like the [python].resolve_default. I'm slightly concerned that requiring someone to learn both the __defaults__ mechanism and pants.toml to get a simple repo started is a high bar, so it would be good to find a way to avoid (effectively) requiring __defaults__ to get started.

[kaos]

I realize that we want to keep the door open for providing options to the __defaults__ call, such as if the new defaults should replace any previous defaults, or merely extend them, etc.
In order to achieve this, I think we should drop the kwargs syntax, and only stick with the overrides version, leaving any kwargs for options to the defaults call itself.

__defaults__(
  {
    my_target: dict(field_a=1, field_b=2),
    ...
  },
  replace=True,
  opt_foo="bar",
  ...
)

[kaos]

yeah, and now I've made the symbol inherit from str, so it is str or str :P

[thejcannon]

That is a change worth dissecting. I have a fear someone might accidentally use it as a string in another context and it "works", but not in the intended way. Whereas now you'd get a (possibly confusing) error.

[Eric-Arellano]

My first reaction was to also think we should require a string, for parity with overrides. But I think this maybe is different...overrides uses arbitrary values like file paths, rather than a fixed set of target types.

I think I'm in favor of letting you use only the symbol python_source without needing the quotes. Probably accept the quotes too.

[thejcannon]

Also on:

symbol inherit from str

I think providing __str__ is safer. inheriting from builtins has a lot of weird quirks 😵

[kaos]

Great inputs. Thanks!

Take away:

• Add __str__ to parser Registrar rather than interhit from str.
• Allow either Registrar or str as keys for the defaults dictionary.

[thejcannon]

Just calling this out, probably OK. __all__ already has a meaning in Python code. Our BUILD files are syntactically Python, but environmentally metadata. So they are kinda-but-not-really-Python.

Probably OK, just takes a new tint on my BUILD file goggles.

[kaos]

There are two undocumented assumptions in your comment Eric (which are currently not as designed). Care to raise them for discussion?

[kaos]

(resolved out-of-band with Eric)
I overlooked that all wasn't part of the first dict arg. The other point was about the help which is dependant on #14832, and seeing that we don't know what that will look like, postponing that consideration.

[kaos]

The remaining assumption is about unknown fields being ignored. I think this may be an option for __defaults__ whether to err or ignore for unknown fields.

[Eric-Arellano]

Cool. Sounds good that the main motivation is to avoid a special symbol like __all_targets__. It's simpler to simply use a kwarg to __defaults__

[kaos]

I'm in favour of moving __all__ out of the dict arg and into an all kwarg.

[Eric-Arellano]

FYI one challenge here will be that some target types do not consistently use the same name for a field. Namely, Python targets call it resolve, but codegen targets get a plugin field called python_resolve. I don't think it is worth it to try to magically handle that.

This might suggest there are some times where we want to keep subsystem options, like [python].default_resolve, as supplementary to __defaults__.

[kaos]

I don't really see the issue here.

__defaults__({
  python_sources: dict(resolve="my-resolve"),
  protobuf_sources: dict(python_resolve="my-resolve"),
  ...
})

[Eric-Arellano]

It's generally not a big issue, not blocking at all.

My main takeaway is that I agree with Stu in the thread #15809 (comment) that it is valuable sometimes to still have a subsystem option to set the repo default. For Python resolves, if we got rid of that repo resolve, then to set a repo default you would need to use __defaults__ and remember the gotcha about python_resolve vs resolve.

[stuhood]

My main takeaway is that I agree with Stu in the thread #15809 (comment) that it is valuable sometimes to still have a subsystem option to set the repo default. For Python resolves, if we got rid of that repo resolve, then to set a repo default you would need to use __defaults__ and remember the gotcha about python_resolve vs resolve.

I left a comment in the doc (oops) that I think that moving those to the Field's default value should actually work fine: https://docs.google.com/document/d/1KHkAUonCCMrdI7TqEx06mUpPZqItXJBwlCD3tBz5-xw/edit?disco=AAAAa8Vxqbo