Pants: Replace the hardcoded `fatal_warnings` flag with "options sets"

[stuhood]

Rather than the built in warnings and fatal_warnings flags for JVM targets, it would be awesome to have generic named "options sets". The most obvious case of course would be to implement things like fatal_warnings.

In general, we like the ability to control what compiler flags are used in a repo, because it significantly improves consistency to not have hundreds of permutations of compiler flags in use on different code. But things like fatal warnings allow for opting in to stricter behaviour, which is also beneficial. The idea behind options sets would be that rather than having hardcoded arguments to targets like:

scala_library(
  # Enables the extra compiler options defined in the `--fatal-warnings-enabled-args` option.
  fatal_warnings=True,
)

... users could define named sets of options in pants.ini that targets could individually opt into by passing something like:

scala_library(
  # Enables the extra compiler options defined in the `--compiler-options-sets-enabled-args` option
  # for the sets 'fatal_warnings' and 'go_extra_fast'.
  compiler_options_sets={'fatal_warnings', 'go_extra_fast'}
)

...and configuring a set of flags named "go_extra_fast" in pants.ini.

---

At a high level, this will look like adding new compiler.options.sets options to replace the fatal.warnings options, and then deprecating the fatal.warnings options.

The existing options are --fatal-warnings (a boolean defining whether fatal warnings should be enabled... ie Boolean in scala syntax) and --fatal-warnings-(enabled|disabled)-args (each of which is a list of compiler args that should be passed if fatal warnings is enabled/disabled... ie List[String] in scala syntax). The result is that toggling the boolean --fatal-warnings arg decides which of the enabled/disabled args are passed to the compiler.

The new analogous options should look like --compiler-options-sets (a list of string "set names" that define a named set of compiler args... ie List[String] in scala syntax) and --compiler-options-sets-(enabled|disabled)-args (dicts of "set name" to list of compiler args for the set... ie Map[String,List[String]] in scala syntax).

---

At a lower level, this change should look like:

• Adding options to replace the fatal-warnings flags defined here https://github.com/pantsbuild/pants/blob/d45d6eb47e3ad41a1e0f1469f70b9aa144cf22a5/src/python/pants/backend/jvm/subsystems/zinc_language_mixin.py#L21-L23 and here https://github.com/pantsbuild/pants/blob/d45d6eb47e3ad41a1e0f1469f70b9aa144cf22a5/src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile.py#L122-L128 (note the line ranges). I expect that the replacement options will be something like compiler_options_sets (which would be a list of string set names as described above) and compiler_options_sets_(enabled|disabled)_args... where the latter options are dicts from set name to list of compiler args.
• Adding a method/getter to ZincLanguageMixin that merges the old and new forms of the options into just the new form (a list of enabled options sets, and dicts of options sets to enabled/disabled args).
• Adding a method/getter to JvmTarget to merge the old and new forms of the existing fatal_warnings property here https://github.com/pantsbuild/pants/blob/fb0ebf720b73a65f10534fb70ceab38df77aaad3/src/python/pants/backend/jvm/targets/jvm_target.py#L140-L147. Additionally, adding a conditional_deprecation for the fatal_warnings target argument in JvmTarget.__init__. An example of conditional deprecation is here: https://github.com/pantsbuild/pants/blob/919c0e187b7193fa078f9dbe440ad845fbf8ab9b/contrib/go/src/python/pants/contrib/go/targets/go_thrift_library.py#L31-L34
• Consuming the method/getter in ZincLanguageMixin and the JvmTarget property in the zinc compilation task here and a few other places: https://github.com/pantsbuild/pants/blob/fb0ebf720b73a65f10534fb70ceab38df77aaad3/src/python/pants/backend/jvm/tasks/jvm_compile/zinc/zinc_compile.py#L350-L353
• Deprecating the --fatal-warnings* options mentioned above. To deprecate an option, pass the removal_version and removal_hint arguments to the register function: something like removal_version='1.7.0.dev0' and removal_hint='Use the my_new_suggested_option option instead' (example usage of the removal args: https://github.com/pantsbuild/pants/blob/fb0ebf720b73a65f10534fb70ceab38df77aaad3/src/python/pants/backend/codegen/thrift/java/thrift_defaults.py#L26-L29).

[ShaneDelmore]

@stuhood I'll take this issue.

[ShaneDelmore]

@stuhood It turns out this is more than I can handle at the moment, I am going to have to rescind my offer.

[alanbato]

Hi @stuhood,
I've started work on this issue and so far I got a couple of questions.
Do we want to replace the --warnings and --warning-args options too, since they follow the same format?
https://github.com/pantsbuild/pants/blob/d45d6eb47e3ad41a1e0f1469f70b9aa144cf22a5/src/python/pants/backend/jvm/tasks/jvm_compile/jvm_compile.py#L111-L120
Also, when changing this part
https://github.com/pantsbuild/pants/blob/fb0ebf720b73a65f10534fb70ceab38df77aaad3/src/python/pants/backend/jvm/tasks/jvm_compile/zinc/zinc_compile.py#L350-L353
over to the new format, we don't have a canonical list of all possible option_sets, so while inspecting the enabled_args for each option is trivial, the consumption of disabled_args is not as elegant. So far it looks like this:

# Getting the enabled args
for option_set in compiler_option_sets:
  enabled_args = self.get_options().compiler_option_sets_enabled_args.get(option_set, [])
  zinc_args.extend(enabled_args)
# Getting the disabled args, not as efficient due to the O(N) lookup
for option_set, disabled_args in self.get_options().compiler_option_sets_disabled_args:
  if option_set not in compiler_option_sets:
    zinc_args.extend(disabled_args)

Is that ok? Or should we aim for something better?

[stuhood]

Thanks!

Do we want to replace the --warnings and --warning-args options too, since they follow the same format?

@alanbato : I believe that we can, yes. Although doing that as a separate followup PR would be good.

Is that ok? Or should we aim for something better?

That should be fine for now, although I imagine that merging the enabled/disabled args into a single dictionary from set name to a namedtuple of the enabled/disabled args in the constructor of the class might make that code cleaner. We construct the class once per pants run, but the code that you pointed to will run once per target (so on the order of thousands of times per pants run).

[alanbato]

That should be fine for now, although I imagine that merging the enabled/disabled args into a single dictionary from set name to a namedtuple of the enabled/disabled args in the constructor of the class might make that code cleaner.

I see. I think this approach will work for a solution for now. However, even combining enabled/disabled args under the same object would need multiple passes, because the problem is that even though we know what sets are enabled through compiler_option_sets, we can't know which ones are _not_ enabled because there is no single repository of all the option sets.
So, we would still need to loop through the disabled_args and test for presence on the compiler_option_sets in order to know if we need to include those args or not.

I know my explanation is a bit confusing and we don't need to solve this problem right now, just wanted to clarify what I meant in my previous comment.

We construct the class once per pants run, but the code that you pointed to will run once per target (so on the order of thousands of times per pants run).

I would like to move the definition to the class so it doesn't get constructed once per target, if you have any pointers as to how to do it I'd appreciate it :)

[stuhood]

However, even combining enabled/disabled args under the same object would need multiple passes,

I think that I have been assuming that the Target's option_sets argument would be an actual set, in which case you'd be iterating over the dictionary of enabled/disabled args, looking up the set name in the Target's option_sets value (in O(1)), and then using either the enabled/disabled args, depending. But yea, I don't think this is performance critical. Can figure it out later.

I would like to move the definition to the class so it doesn't get constructed once per target, if you have any pointers as to how to do it I'd appreciate it :)

One tool we have here that works pretty well is @memoized_property. Example usage:
https://github.com/pantsbuild/pants/blob/d30cca1e0ecb9cc0e1b7e2cd0ff6e7e077e62a52/contrib/go/src/python/pants/contrib/go/tasks/go_binary_create.py#L26-L27

[alanbato]

I think that I have been assuming that the Target's option_sets argument would be an actual set...

That makes sense, haha. I keep forgetting that BUILD files are valid python so you can use whatever data structure you want.

One tool we have here that works pretty well is @memoized_property. Example usage:

Thanks! I did see it in some parts of the code :)

While doing the duplication/adaption of the fatal_warnings/option_sets behaviors, there are some times where the fatal_warnings get passed in as a parameter (like in the JvmTarget constructor). But I haven't found any reference as to where does it get passed, so I can pass option_sets instead.
Maybe I'm understanding how arguments get passed around between targets/goals? 🤔
My initial guess is that after calling register on an option it gets passed in to constructors and the interface needs to be changed (adding a new parameter to __init__), but for some reason it seems a bit magical for me, haha

[stuhood]

While doing the duplication/adaption of the fatal_warnings/option_sets behaviors, there are some times where the fatal_warnings get passed in as a parameter (like in the JvmTarget constructor). But I haven't found any reference as to where does it get passed, so I can pass option_sets instead.

There is a lot of inheritance of jvm target types: all of the subclasses of JvmTarget eventually pass the argument up to JvmTarget, where it gets stored in the "payload" (...basically just "fingerprinted settings") of the target here: https://github.com/pantsbuild/pants/blob/4aae5dcc86ec488fa9676b2e3d69dd7b73b5c367/src/python/pants/backend/jvm/targets/jvm_target.py#L106

It's then consumed out of the payload here: https://github.com/pantsbuild/pants/blob/4aae5dcc86ec488fa9676b2e3d69dd7b73b5c367/src/python/pants/backend/jvm/targets/jvm_target.py#L133-L140

---

My initial guess is that after calling register on an option it gets passed in to constructors and the interface needs to be changed (adding a new parameter to __init__), but for some reason it seems a bit magical for me, haha

register_option is only for CLI / env / pants.ini options... so unfortunately, it has zero correspondence to Target parameters. They're independent and sortof duplicated in this case.

[cosmicexplorer]

register_option is only for CLI / env / pants.ini options... so unfortunately, it has zero correspondence to Target parameters. They're independent and sortof duplicated in this case.

I left a TODO to create some method of formalizing correspondences between cli and target options in the new C/C++ targets, but hadn't figured out yet which options should be allowed on which targets (e.g. platform-specific args would likely apply only to binaries or exported libraries, not individual compilation targets), and the decoupling of subsystems from targets can be pretty useful even if it means a non-obvious correspondence between cli args and target kwargs, because things that are visible in BUILD files can be hard to change later. I think this could be made more ergonomic to define but am not sure how yet.

[ShaneDelmore]

@alanbato Today I wanted to enable -Ytyper-debug without enabling for every dependent target. Option Sets to the rescue! Thanks again for this super helpful feature. ❤️

[alanbato]

It was my pleasure! I'm super happy to know the things I worked on are helping you all even now!