Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Plugin API] Add type CurrentGoals to allow rules to find out the current running goals. #17674

Closed
wants to merge 4 commits into from

Conversation

kaos
Copy link
Member

@kaos kaos commented Nov 29, 2022

Example:

from pants.engine.internals.scheduler import CurrentExecutingGoal

@rule
def my_rule(..., current_goal: CurrentExecutingGoal) -> MyResult:
  if "my-goal" == current_goal.name:
    # conditional stuff for my-goal

  return MyResult()

This is for #17671 (comment)

Copy link
Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm concerned that this will break in the presence of #10542, which doesn't have many other blockers left.

Could #17671 (comment) use a flag on the request type instead?

@kaos
Copy link
Member Author

kaos commented Dec 1, 2022

I'm concerned that this will break in the presence of #10542, which doesn't have many other blockers left.

Good point. I'll have another look with that in mind.

Could #17671 (comment) use a flag on the request type instead?

Ah, I'd like that, but as the linting goal would also trigger fetching all dependencies, it would still reach the visibility backend the regular way.

One thing I tried (and failed, but maybe that could be fixed) was to introduce a "context value" in the rule graph, akin to how React does. Conceptual example:

@rule
def context_value() -> MyContextValue:
  return MyContextValue(defaultValue)

@rule
def validate_visibility_dependency_rules(ctx_value: MyContextValue, request: ...) -> ValidatedDependencies:
  ...

# ---

# Here I may not understand masked_types properly, my thoughts where to "replace" any previous value with a new one
@goal_rule(_masked_types=[MyContextValue])
async def visibility_linter_goal(...):
  # The provided MyContextValue here is the key - I want it to replace any previous source for it with this new one.
  await Get(ValidatedDependencies, {request: RequestType, ctx_value: MyContextValue})
  ...

@kaos
Copy link
Member Author

kaos commented Dec 14, 2022

@stuhood I've adjusted to implement most of the goal rule identification on the rust side of the engine, hopefully that'll work also when running goals in parallel.

@kaos kaos changed the title [Plugin API] Add type CurrentExecutingGoal to allow rules to find out the current goal. [Plugin API] Add type CurrentGoals to allow rules to find out the current running goals. Dec 15, 2022
@kaos
Copy link
Member Author

kaos commented Dec 15, 2022

This does not block #10542 but needs an alternate solution to unblock #7654, as do RunId and SessionValues so this may be considered good enough until such an alternative solution is developed.

Comment on lines 136 to 139
@final
@classmethod
def is_goal(cls) -> bool:
return True
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should probably use is_subclass instead rather than a new property.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree, I can take another look at that. I tried that first and failed, but I've learned so much each day, so I may be able to pull it off this time. Perhaps :P

@@ -75,6 +75,13 @@
Workunit = Dict[str, Any]


@dataclass(frozen=True)
class CurrentGoals:
"""The currently running goals."""
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"""The currently running goals."""
"""The currently running goals if any."""

It's possible for no goals to be running: probably the intrinsic @rule should fail in that case.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why fail? The .rule_names could be empty in that case, is that not a valid scenario..?

class CurrentGoals:
"""The currently running goals."""

rule_names: tuple[str, ...]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be rule names, or the Goal.subsystem.name?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer the Goal.subsystem.name but failed to figure out how to get that :P
Then again, I realized this may be more robust, or too rigid I suppose, depending on the use case. Maybe it would be nice to capture both?

async fn run_node(self, context: Context, workunit: &mut RunningWorkunit) -> NodeResult<Value> {
let gil = Python::acquire_gil();
let py = gil.python();
let names = workunit.get_current_goals();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than doing this via workunits, you should probably make it a mutable property of the Session, similar to the SessionValues:

// Per-Session values that have been set for this session.
session_values: Mutex<PyObject>,

Then, in run_goal_rules, you would try-finally setting and clearing the CurrentGoals value.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ahh.. ok. Interesting. Yea, going via workunits has been a source of much headache and fiddling to tie the pieces together.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But wait.. if we have that in try/finally in the python world, that's pretty close to what I had originally only applied at a different spot, right?

aaa50d6

Where I stored it in SessionValues directly rather than as a property of the session, though.

Copy link
Member

@stuhood stuhood Dec 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. Yea, it is.

I think that I was confused by the mutation in that case... I think that you could store this directly in the SessionValues, which we already support setting? Then you don't need to mutate the CurrentGoals type, and can get it in a normal rule from the SessionValues via session_values[CurrentGoals]:

options_bootstrapper = session_values[OptionsBootstrapper]

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uhm, just coming back to this again--and in my initial version I stored the CurrentExecutingGoal type in the session values as you suggest. But I made it mutatable as my understanding is that I can't change the session values once the session has been created..?

session_values=SessionValues(
{
OptionsBootstrapper: options_bootstrapper,
CompleteEnvironmentVars: env,
CurrentExecutingGoal: CurrentExecutingGoal(),
}
),

Comment on lines +101 to +107
goal_param_types: ClassVar[tuple[type, ...]] = (
Specs,
Console,
Workspace,
EnvironmentName,
CurrentExecutingGoal,
)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stuhood I found this spot for it. Feels so much more appropriate than in session values! ;)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately, this will cause anything that consumes the CurrentExecutingGoal to get a new "identity" for each goal. So if you consumed this field below a graph calculation (for example), you would end up with two copies of the graph in memory.

Happy to have a video call to talk through this if need be.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ugh. OK. Let's do that, can sync on slack.

@kaos kaos requested a review from stuhood April 11, 2023 17:50
Comment on lines +152 to +154
params = Params(
specs, self.console, workspace, env_name, CurrentExecutingGoal(goal, goal_product)
)
Copy link
Member Author

@kaos kaos Apr 11, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

regarding #10542 I think that we must still be able to provide goal specific Params even when running multiple goals in parallel and as such this "should" be OK, no? (as it is flagged as uncachable)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is safe from a #10542 perspective. But it has the other problem I mentioned above: #17674 (comment) ... =(

Copy link
Member

@stuhood stuhood left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed offline, I'm fine with this as long as it is well labeled with the caveat that any @rule consuming the CurrentGoals will need to re-run for different input goals.

@kaos
Copy link
Member Author

kaos commented Apr 21, 2023

As discussed offline, I'm fine with this as long as it is well labeled with the caveat that any @rule consuming the CurrentGoals will need to re-run for different input goals.

Turns out this is a bigger caveat than I want us to swallow. Looking for alternative venues.

@kaos
Copy link
Member Author

kaos commented Apr 22, 2023

Superseded by #18788

@kaos kaos closed this Apr 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants