Add environment variable help options #19078

marcuslimdw · 2023-05-21T12:52:42Z

Addresses #17618.

…ption help info

marcuslimdw · 2023-05-21T13:28:47Z

src/python/pants/help/help_info_extracter.py

@@ -529,6 +536,14 @@ def load() -> TargetTypeHelpInfo:
            }
        )

+        env_var_to_help_info = LazyFrozenDict(


IMO this should actually be a FrozenDict, but one can't be instantiated from OptionHelpInfo because it contains mutable values (lists). this feels like a deeper issue that should be dealt with elsewhere (addendum: it doesn't seem okay that LazyFrozenDict's lazy values can be mutable?)

it doesn't seem okay that LazyFrozenDict's lazy values can be mutable?

Good point. I've overlooked this. When fetching the value here

pants/src/python/pants/util/frozendict.py

Line 153 in cc8be36

return cast("Callable[[], V]", self._data[k])()

it would likely be a good idea then to check it is hashable in a similar manner as is done for the values of a FrozenDict here:

pants/src/python/pants/util/frozendict.py

Lines 99 to 102 in cc8be36

try:

return hash(tuple(self._data.items()))

except TypeError as e:

raise TypeError(

can be dealt with in a separate PR, but this will definitely break at least this PR, and possibly other code that uses LazyFrozenDict - shall I create an issue for it?

Yea, an issue would be great. I don't think that the use of the LazyFrozenDict is very wide spread. I added it for this use case here and it might've found it's way in else where but my guess that is very limited.

benjyw

Thanks for this! It's very neat. I applied the diff locally and tried it out, and it looks good. I have one small nit (see below), and also a question about whether we should be calling out which subsystem the option belongs to, e.g., a line like this at the end:

Run `pants help-advanced python` to list all the options for this subsystem.

Otherwise it's hard to track this little nugget of help back to its context.

WDYT?

src/python/pants/help/help_printer.py

marcuslimdw · 2023-05-22T23:59:24Z

Thanks for this! It's very neat. I applied the diff locally and tried it out, and it looks good. I have one small nit (see below), and also a question about whether we should be calling out which subsystem the option belongs to, e.g., a line like this at the end:
Run `pants help-advanced python` to list all the options for this subsystem.
Otherwise it's hard to track this little nugget of help back to its context.

WDYT?

I'm new to the Pants ecosystem so I defer to you on how helpful this would be - at face value, at least, it does sound like a good thing to have. thanks for the suggestion!

this will require a small rewrite, though, because an OptionHelpInfo doesn't know which subsystem it comes from. I think the best solution is to modify env_var_to_help_info to map to a new dataclass that contains the parent subsystem's name and the associated OptionHelpInfo. how does that sound?

benjyw · 2023-05-23T14:31:34Z

this will require a small rewrite, though, because an OptionHelpInfo doesn't know which subsystem it comes from. I think the best solution is to modify env_var_to_help_info to map to a new dataclass that contains the parent subsystem's name and the associated OptionHelpInfo. how does that sound?

That sounds probably correct, but not something I want to drag you through right now, since it's more involved than I had anticipated. We can always do it as a followup. So I think we can merge this as-is, and I'll ask you to create an issue for the followup.

benjyw · 2023-05-23T14:32:48Z

After I merge #19099 I recommend that you merge it into your branch and push, and then CI on this PR is much more likely to pass.

kaos

This feels like it is duplicating already existing information in the AllHelpInfo which is already huge.

Was there a reason to not extract this information from current data as suggested in #17618 (comment) ?

I do realize that we may need the keys (i.e. the env var names) up front to avoid loading all the lazy data in the help printer, but that could be a slimmer version than this, where the env var name is just mapping to the option name to look up from scope_to_help_info.

Edit: For the help printer this is not a big deal as the data is gated behind lazy loading. My concern is mostly the size of the output for pants help-all.

marcuslimdw · 2023-05-28T08:20:16Z

@kaos could you elaborate on "extract this information from current data", please?

as for why we're not mapping to the option name - because it's not possible to look it up directly except by iterating over the available options, and I am not sure about whether that would be slow (though I guess it's unlikely since there wouldn't be more than a few options). or is there something else you had in mind?

kaos · 2023-05-29T15:51:36Z

@kaos could you elaborate on "extract this information from current data", please?

as for why we're not mapping to the option name - because it's not possible to look it up directly except by iterating over the available options, and I am not sure about whether that would be slow (though I guess it's unlikely since there wouldn't be more than a few options). or is there something else you had in mind?

The information is already in the scope to help info so could be picked up from there at presentation time, but as you say it is nested in several lists so no easy look up without a lot of re-arranging.

My comments where merely minor efficiency concerns on the size of output for help-all. I'm happy to leave it as-is here for now, after addressing the feedback from Benjy.

benjyw · 2023-05-30T05:22:05Z

I've re-kicked CI to get those flaky tests passing.

…tions

marcuslimdw · 2023-06-20T01:12:45Z

@kaos sounds good to me! didn't we agree that we'd address those issues in a separate PR, though?

kaos · 2023-06-20T13:43:09Z

@kaos sounds good to me! didn't we agree that we'd address those issues in a separate PR, though?

Ah yes, this right?

this will require a small rewrite, though, because an OptionHelpInfo doesn't know which subsystem it comes from. I think the best solution is to modify env_var_to_help_info to map to a new dataclass that contains the parent subsystem's name and the associated OptionHelpInfo. how does that sound?

That sounds probably correct, but not something I want to drag you through right now, since it's more involved than I had anticipated. We can always do it as a followup. So I think we can merge this as-is, and I'll ask you to create an issue for the followup.

…tions

benjyw · 2023-06-20T15:54:30Z

Kicking CI again.

benjyw · 2023-06-21T05:30:56Z

@kaos your thoughts on merging this?

kaos

I think there's some edges but nothing new really, so lgtm.

benjyw · 2023-06-21T15:08:34Z

Thanks @marcuslimdw !!

marcuslimdw added 4 commits May 21, 2023 20:43

Populate AllHelpInfo with mapping from environment variable name to o…

6fe8826

…ption help info

Add function to print environment variable help info

9c7beb3

Add help table entries for environment variable help

4bd29b0

Use explicit closure generator to satisfy mypy

e156139

marcuslimdw commented May 21, 2023

View reviewed changes

marcuslimdw marked this pull request as ready for review May 21, 2023 13:29

marcuslimdw added 2 commits May 22, 2023 13:17

Update test

d766032

Restart CI

69e05d3

stuhood requested review from kaos and benjyw May 22, 2023 18:31

stuhood added the category:new feature label May 22, 2023

benjyw reviewed May 22, 2023

View reviewed changes

src/python/pants/help/help_printer.py Outdated Show resolved Hide resolved

marcuslimdw added 2 commits May 23, 2023 07:53

Add .keys() call for consistency

5cb6fd6

Blacken code

09bf3de

kaos reviewed May 23, 2023

View reviewed changes

Merge branch 'main' into marcuslimdw/add-environment-variable-help-op…

643f254

…tions

marcuslimdw requested a review from benjyw June 20, 2023 01:12

Merge branch 'main' into marcuslimdw/add-environment-variable-help-op…

cd66b01

…tions

kaos approved these changes Jun 21, 2023

View reviewed changes

benjyw merged commit f53642d into pantsbuild:main Jun 21, 2023
24 checks passed

danxmoran mentioned this pull request Sep 12, 2023

./pants help should work with options as environment variables #17618

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add environment variable help options #19078

Add environment variable help options #19078

marcuslimdw commented May 21, 2023

marcuslimdw May 21, 2023

kaos May 23, 2023

marcuslimdw May 23, 2023

kaos May 24, 2023

benjyw left a comment

marcuslimdw commented May 22, 2023

benjyw commented May 23, 2023

benjyw commented May 23, 2023

kaos left a comment •

edited

marcuslimdw commented May 28, 2023

kaos commented May 29, 2023 •

edited

benjyw commented May 30, 2023

marcuslimdw commented Jun 20, 2023

kaos commented Jun 20, 2023

benjyw commented Jun 20, 2023

benjyw commented Jun 21, 2023

kaos left a comment

benjyw commented Jun 21, 2023

	try:
	return hash(tuple(self._data.items()))
	except TypeError as e:
	raise TypeError(

Add environment variable help options #19078

Add environment variable help options #19078

Conversation

marcuslimdw commented May 21, 2023

marcuslimdw May 21, 2023

Choose a reason for hiding this comment

kaos May 23, 2023

Choose a reason for hiding this comment

marcuslimdw May 23, 2023

Choose a reason for hiding this comment

kaos May 24, 2023

Choose a reason for hiding this comment

benjyw left a comment

Choose a reason for hiding this comment

marcuslimdw commented May 22, 2023

benjyw commented May 23, 2023

benjyw commented May 23, 2023

kaos left a comment • edited

Choose a reason for hiding this comment

marcuslimdw commented May 28, 2023

kaos commented May 29, 2023 • edited

benjyw commented May 30, 2023

marcuslimdw commented Jun 20, 2023

kaos commented Jun 20, 2023

benjyw commented Jun 20, 2023

benjyw commented Jun 21, 2023

kaos left a comment

Choose a reason for hiding this comment

benjyw commented Jun 21, 2023

kaos left a comment •

edited

kaos commented May 29, 2023 •

edited