Generically compute dynamic defaults for `Field`s #16206

stuhood · 2022-07-18T19:50:45Z

As discussed on #16175, we don't currently consume the "dynamic" defaults of field values for the purposes of parametrize. That is at least partially because there is no generic way to do so: a Field has no way to declare a dynamic default currently, because Fields cannot declare a dependency @rule_helper to compute their value (...yet? see #12934 (comment)).

This change adds a mechanism for generically declaring the default value of a Field. This is definitely not the most ergonomic API: over the next few versions, many dynamic Field defaults will hopefully move to __defaults__. And #12934 (comment) will hopefully allow for significantly cleaning up those that remain.

Fixes #16175.

Eric-Arellano

Looks good overall. I would expect this to need to be consumed by parametrize.py?

stuhood · 2022-07-18T21:18:02Z

Looks good overall. I would expect this to need to be consumed by parametrize.py?

Yea, will rebase atop #16197 for that.

Eric-Arellano

Thanks!

Do you think we will want to hook this up to ./pants help, in a follow up?
We have other candidate fields, like those from [pex-binary-defaults]. Should those be hooked up in a follow up?
Should call sites of the resolve fields be refactored to use FieldDefaults instead, for DRY purposes?

src/python/pants/engine/internals/parametrize.py

Eric-Arellano · 2022-07-18T22:42:36Z

src/python/pants/engine/target.py

+
+
+@dataclass(frozen=True)
+class FieldDefaults:


What about calling it DynamicFieldDefaults?

I think that all imperative code is implicitly "dynamic"... I wanted to mention dynamic in the docstring to differentiate from static Field defaults, but we wouldn't want to rename that field to static_default, so not using it in the name here seems ok.

Eric-Arellano · 2022-07-18T22:43:00Z

src/python/pants/engine/target.py

+    def value_or_default(self, field: Field) -> Any:
+        return (self.factory(type(field)))(field)


Worth a unit test?

I think that with both unit tests of parametrize_test.py and integration tests in graph_test.py, we're probably good.

…for Fields dynamically. [ci skip-rust] [ci skip-build-wheels]

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

[ci skip-rust] [ci skip-build-wheels]

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

stuhood · 2022-07-18T23:13:12Z

We have other candidate fields, like those from [pex-binary-defaults]. Should those be hooked up in a follow up?

Should call sites of the resolve fields be refactored to use FieldDefaults instead, for DRY purposes?

Possibly. But as mentioned in the description, my hope is that most of the usecases of this facility are replaced with __defaults__, and that any that aren't are replaced with the facility from #12934 (comment).

Do you think we will want to hook this up to ./pants help, in a follow up?

Yea, probably. Whether that should happen before/after a #12934 (comment) Field.compute_default_value-as-@rule_helper facility is the primary question.

As discussed on pantsbuild#16175, we don't currently consume the "dynamic" defaults of field values for the purposes of `parametrize`. That is at least partially because there is no generic way to do so: a `Field` has no way to declare a dynamic default currently, because `Field`s cannot declare a dependency `@rule_helper` to compute their value (...yet? see pantsbuild#12934 (comment)). This change adds a mechanism for generically declaring the default value of a `Field`. This is definitely not the most ergonomic API: over the next few versions, many dynamic `Field` defaults will hopefully move to `__defaults__`. And pantsbuild#12934 (comment) will hopefully allow for significantly cleaning up those that remain. Fixes pantsbuild#16175. [ci skip-rust] [ci skip-build-wheels]

) (#16220) As discussed on #16175, we don't currently consume the "dynamic" defaults of field values for the purposes of `parametrize`. That is at least partially because there is no generic way to do so: a `Field` has no way to declare a dynamic default currently, because `Field`s cannot declare a dependency `@rule_helper` to compute their value (...yet? see #12934 (comment)). This change adds a mechanism for generically declaring the default value of a `Field`. This is definitely not the most ergonomic API: over the next few versions, many dynamic `Field` defaults will hopefully move to `__defaults__`. And #12934 (comment) will hopefully allow for significantly cleaning up those that remain. Fixes #16175. [ci skip-rust] [ci skip-build-wheels]

) (#16219) As discussed on #16175, we don't currently consume the "dynamic" defaults of field values for the purposes of `parametrize`. That is at least partially because there is no generic way to do so: a `Field` has no way to declare a dynamic default currently, because `Field`s cannot declare a dependency `@rule_helper` to compute their value (...yet? see #12934 (comment)). This change adds a mechanism for generically declaring the default value of a `Field`. This is definitely not the most ergonomic API: over the next few versions, many dynamic `Field` defaults will hopefully move to `__defaults__`. And #12934 (comment) will hopefully allow for significantly cleaning up those that remain. Fixes #16175. [ci skip-rust] [ci skip-build-wheels]

As discussed on pantsbuild#16175, we don't currently consume the "dynamic" defaults of field values for the purposes of `parametrize`. That is at least partially because there is no generic way to do so: a `Field` has no way to declare a dynamic default currently, because `Field`s cannot declare a dependency `@rule_helper` to compute their value (...yet? see pantsbuild#12934 (comment)). This change adds a mechanism for generically declaring the default value of a `Field`. This is definitely not the most ergonomic API: over the next few versions, many dynamic `Field` defaults will hopefully move to `__defaults__`. And pantsbuild#12934 (comment) will hopefully allow for significantly cleaning up those that remain. Fixes pantsbuild#16175. [ci skip-rust] [ci skip-build-wheels]

stuhood added the category:plugin api change label Jul 18, 2022

Eric-Arellano reviewed Jul 18, 2022

View reviewed changes

stuhood force-pushed the stuhood/dynamic-field-defaults branch 2 times, most recently from 497143a to 91a21d8 Compare July 18, 2022 22:27

stuhood marked this pull request as ready for review July 18, 2022 22:34

stuhood requested review from Eric-Arellano and kaos July 18, 2022 22:34

Eric-Arellano reviewed Jul 18, 2022

View reviewed changes

stuhood added 5 commits July 18, 2022 15:48

Generalize the BSP dynamic-defaults code to allow computing defaults …

fa106f8

…for Fields dynamically. [ci skip-rust] [ci skip-build-wheels]

Implement FieldDefaultFactoryRequest for PythonResolveField.

a897487

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Use FieldDefaults in parametrize.py.

7ce5c36

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Remove redundant boolean comparison.

a58482d

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

Test FieldDefaults.

de167ce

[ci skip-rust] [ci skip-build-wheels]

stuhood force-pushed the stuhood/dynamic-field-defaults branch from 91a21d8 to de167ce Compare July 18, 2022 22:48

Eric-Arellano approved these changes Jul 18, 2022

View reviewed changes

Review feedback.

8e868f9

# Rust tests and lints will be skipped. Delete if not intended. [ci skip-rust] # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]

stuhood enabled auto-merge (squash) July 18, 2022 23:25

stuhood disabled auto-merge July 18, 2022 23:28

stuhood enabled auto-merge (squash) July 18, 2022 23:29

stuhood merged commit 9df18f3 into pantsbuild:main Jul 18, 2022

stuhood deleted the stuhood/dynamic-field-defaults branch July 19, 2022 01:26

stuhood added the needs-cherrypick label Jul 19, 2022

stuhood added this to the 2.12.x milestone Jul 19, 2022

stuhood removed the needs-cherrypick label Jul 19, 2022

Eric-Arellano mentioned this pull request Jul 19, 2022

Wire up dynamic Field defaults to ./pants help #16222

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generically compute dynamic defaults for `Field`s #16206

Generically compute dynamic defaults for `Field`s #16206

stuhood commented Jul 18, 2022 •

edited

Eric-Arellano left a comment

stuhood commented Jul 18, 2022

Eric-Arellano left a comment

Eric-Arellano Jul 18, 2022

stuhood Jul 18, 2022 •

edited

Eric-Arellano Jul 18, 2022

stuhood Jul 18, 2022

stuhood commented Jul 18, 2022 •

edited

		def value_or_default(self, field: Field) -> Any:
		return (self.factory(type(field)))(field)



		@dataclass(frozen=True)
		class FieldDefaults:

Generically compute dynamic defaults for Fields #16206

Generically compute dynamic defaults for Fields #16206

Conversation

stuhood commented Jul 18, 2022 • edited

Eric-Arellano left a comment

Choose a reason for hiding this comment

stuhood commented Jul 18, 2022

Eric-Arellano left a comment

Choose a reason for hiding this comment

Eric-Arellano Jul 18, 2022

Choose a reason for hiding this comment

stuhood Jul 18, 2022 • edited

Choose a reason for hiding this comment

Eric-Arellano Jul 18, 2022

Choose a reason for hiding this comment

stuhood Jul 18, 2022

Choose a reason for hiding this comment

stuhood commented Jul 18, 2022 • edited

Generically compute dynamic defaults for `Field`s #16206

Generically compute dynamic defaults for `Field`s #16206

stuhood commented Jul 18, 2022 •

edited

stuhood Jul 18, 2022 •

edited

stuhood commented Jul 18, 2022 •

edited