Turn on --v2 by default now that the V2 backends are not loaded by default

[Eric-Arellano]

Now that we have backend_packages vs. backend_packages2, turning on --v2 by default will have zero impact on users who have not explicitly opted in to V2 backends because backend_packages2 defaults to [] and we no-op for V2 goals with no implementations.

So, if this has no impact on users, why do this? It lowers the barrier to adoption for using V2. Currently, to use V2 Black, for example, you must do two things:

1. backend_packages2. add = ['pants.backend.python.lint.black']
2. v2 = true

Now, you solely must do backend_packages2.add = ['pants.backend.python.lint.black'].

[stuhood]

I know that it is only symbolic, but: yay!

[Eric-Arellano]

CI caught a big issue for us! Yay CI!

Certain V2 rules don't work properly when they have no backends providing an implementation, such as run, fmt2, and test. This is addressed by #9002.

The issue with this PR is that now, for ambiguous goals like test and run, by default we try to run with both V1 and V2. But, there is no V2 implementation so we get exceptions.

The solution is to extend #9002, which only impacts the output of ./pants goals, to also somehow deregister / skip the V2 goals when they are not implemented. That is, ./pants --v1 --v2 run should not run the V2 @goal_rule if there are no rules providing an implementation for it.

[stuhood]

Another consideration here is: does this conflict with enabling --v2-ui by default (if --v2 is enabled)? As discussed with @gshuflin : it is already very useful, and further dogfooding will further improve it.

[stuhood]

Certain V2 rules don't work properly when they have no backends providing an implementation, such as run, fmt2, and test. This is addressed by #9002.

When you say: "don't work properly", what do you mean? Shouldn't they noop? The is_implemented method feels redundant with the logic in the goal itself: if the nothing matches a union, we should not trigger any logic, right?

So, if anything, it feels like the @goal_rule and/or UnionMembership should expose/use the possibility that a union is empty, so that the goal can do the right thing locally...?

[Eric-Arellano]

Shouldn't they noop? The is_implemented method feels redundant with the logic in the goal itself: if the nothing matches a union, we should not trigger any logic, right?

Yes, IMO they should no-op.

Are you proposing that this logic should live at the goal level, rather than at one level higher of abstraction like here?

pants/src/python/pants/init/engine_initializer.py

Lines 201 to 242 in 155393b

	def run_goal_rules(
	self,
	options_bootstrapper: OptionsBootstrapper,
	options: Options,
	goals: Iterable[str],
	specs: Specs,
	):
	"""Runs @goal_rules sequentially and interactively by requesting their implicit Goal products.
	For retryable failures, raises scheduler.ExecutionError.
	:returns: An exit code.
	"""
	global_options = options.for_global_scope()
	console = Console(
	use_colors=global_options.colors,
	session=self.scheduler_session if global_options.v2_ui else None,
	)
	workspace = Workspace(self.scheduler_session)
	interactive_runner = InteractiveRunner(self.scheduler_session)
	for goal in goals:
	goal_product = self.goal_map[goal]
	params = Params(
	specs.provided_specs,
	options_bootstrapper,
	console,
	workspace,
	interactive_runner,
	)
	logger.debug(f'requesting {goal_product} to satisfy execution of `{goal}` goal')
	try:
	exit_code = self.scheduler_session.run_goal_rule(goal_product, params)
	finally:
	console.flush()
	if exit_code != PANTS_SUCCEEDED_EXIT_CODE:
	return exit_code
	return PANTS_SUCCEEDED_EXIT_CODE

That would work, but IMO it makes more sense for the engine to never even try running the goal if there is no actual implementation for it. Beyond this seeming like a more appropriate level of abstraction, this would enable us to do things like output in ./pants goals that the core goals don't yet have implementations and that the user should run ./pants backends to fix this (#9002 (comment)).

[benjyw]

Note that I have a pending change (#8997) that will only register --v2-ui at all if --v2 is true, in case that affects the concern raised above.

[Eric-Arellano]

Note that I have a pending change (#8997) that will only register --v2-ui at all if --v2 is true, in case that affects the concern raised above.

Tricky. V2 UI does not work great when also running V1. Possibly, we should improve this interaction.

Possibly, this PR is a bad idea (but I think the necessary precursor changes are still good ones).

[Eric-Arellano]

Note that I have a pending change (#8997) that will only register --v2-ui at all if --v2 is true, in case that affects the concern raised above.

A fix for the edge case of --v1 --v2 with the default value for --v2-ui: default to --v2-ui iff --no-v1 --v2. Use --no-v2-ui by default if --v1 is enabled because it does not work well to have both the V1 and V2 UIs at the same time.

[stuhood]

Use --no-v2-ui by default if --v1 is enabled because it does not work well to have both the V1 and V2 UIs at the same time.

So, it shouldn't necessarily be the case that this is true... exactly one codepath should be reading and using the --v2-ui flag: this one:

pants/src/python/pants/engine/scheduler.py

Lines 464 to 479 in aa0d9c2

	def run_goal_rule(self, product, subject):
	"""
	:param product: A Goal subtype.
	:param subject: subject for the request.
	:returns: An exit_code for the given Goal.
	"""
	request = self.execution_request([product], [subject])
	returns, throws = self.execute(request)
	if throws:
	_, state = throws[0]
	exc = state.exc
	self._trace_on_error([exc], request)
	return PANTS_FAILED_EXIT_CODE
	_, state = returns[0]
	return state.value.exit_code

I think that issues mixing v1 and v2 likely stem from the fact that we currently push this flag value into the Session, here:

pants/src/python/pants/bin/local_pants_runner.py

Lines 124 to 132 in aa0d9c2

	v2_ui = options.for_global_scope().get('v2_ui', False)
	zipkin_trace_v2 = options.for_scope('reporting').zipkin_trace_v2
	#TODO(#8658) This should_report_workunits flag must be set to True for
	# StreamingWorkunitHandler to receive WorkUnits. It should eventually
	# be merged with the zipkin_trace_v2 flag, since they both involve most
	# of the same engine functionality, but for now is separate to avoid
	# breaking functionality associated with zipkin tracing while iterating on streaming workunit reporting.
	stream_workunits = len(options.for_global_scope().streaming_workunits_handlers) != 0
	graph_session = graph_scheduler_helper.new_session(zipkin_trace_v2, RunTracker.global_instance().run_id, v2_ui, should_report_workunits=stream_workunits)

We don't want to use the UI for "the whole Session": rather, precisely the invoke of the engine that runs a @goal_rule. I think that this fix would make it safe to enable the UI in a v1 run (which will call into the scheduler/engine a few times, but never wants the UI in those codepaths).