https://pantsbuild.org/ logo
Join Slack
Powered by
Any ideas if it’s possible to synchronously reques...
# development
h
Any ideas if it’s possible to synchronously request a subsystem registered with 
subsystem_rule
? These both failed
Copy code
changed_subsystem, = session.product_request(Changed, [Options, options])  # Options is not a RootRule
Copy code
changed_subsystem, = session.product_request(Changed, [])  # Must give a subject
a
i think you can always make something a RootRule too by just adding that to the list of rules
but the problem you describe would be great to address
h
Agreed, but I think we likely don’t want to make 
Options
be a 
RootRule
only to support this edge case I’m wondering how the async API ends up getting the Subsystem when it’s in a rule signature? I think that we convert the Subsystem into a TaskRule?
a
Is there an equivalent problem with 
await Get[X](...)
? i suppose we still don’t support params in Get yet
h
hm, I haven’t tried that
we always request subsystems in the rule signature
a
Agreed, but I think we likely don’t want to make 
Options
be a 
RootRule
only to support this edge case
i think this might actually be ok if necessary!
I’m wondering how the async API ends up getting the Subsystem when it’s in a rule signature? I think that we convert the Subsystem into a TaskRule?
what do you mean “in a rule signature”? do you mean “not a Get”?
h
Copy code
@rule
def my_rule(isort: Isort) -> LintResult
Rather than
Copy code
@rule
def my_rule() -> LintResult:
  isort = await Get[Isort](..)
a
if you check optionable.py you’ll see where we convert to a TaskRule
it’s a method ending in 
_signature
h
Copy code
@classmethod
  def signature(cls):
    """Returns kwargs to construct a `TaskRule` that will construct the target Optionable.

    TODO: This indirection avoids a cycle between this module and the `rules` module.
    """
    partial_construct_optionable = functools.partial(_construct_optionable, cls)

    # NB: We must populate several dunder methods on the partial function because partial functions
    # do not have these defined by default and the engine uses these values to visualize functions
    # in error messages and the rule graph.
    snake_scope = cls.options_scope.replace('-', '_')
    partial_construct_optionable.__name__ = f'construct_scope_{snake_scope}'
    partial_construct_optionable.__module__ = cls.__module__
    _, class_definition_lineno = inspect.getsourcelines(cls)
    partial_construct_optionable.__line_number__ = class_definition_lineno

    return dict(
        output_type=cls.optionable_cls,
        input_selectors=tuple(),
        func=partial_construct_optionable,
        input_gets=(Get.create_statically_for_rule_graph(ScopedOptions, Scope),),
        dependency_optionables=(cls.optionable_cls,),
      )
a
that is then consumed in rules.py
h
I think 
input_gets=(Get.create_statically_for_rule_graph(ScopedOptions, Scope),)
is relevant? Does this suggest that you should pass 
ScopedOptions
?
a
ScopedOptions is almost definitely going to be provided for you i think, i’d try ripgrep searching that symbol
h
changed_subsystem, = session.product_request(Changed, [Scope(Changed.options_scope)])
leads to
Copy code
Exception message: No installed @rules can compute Changed for input Params(Scope), but there were @rules that could compute it using:
  Params(OptionsBootstrapper)
a
where is this erroring?
is this in a test?
h
When running 
./pants
a
oh
oh well
h
(I’m trying to add 
changed_subsystem, = session.product_request(Changed, [Scope(Changed.options_scope)])
to 
specs_calculator.py
. It has access to 
Options
, but not 
OptionsBootstrapper
)
a
i also have a PR that rewrites changed calculation as prework for --query
it uses @rules and doesn’t have this issue because it avoids using the sync API
what branch are you on?
h
I saw! But it looked like that was blocked so I’m trying to cherry-pick the parts that we can land as prework for deprecating 
--owner-of
a
yes!
i was going to attempt to determine whether that PR (or some of it) could work around the issue you’re seeing
h
Here’s the WIP: https://github.com/pantsbuild/pants/compare/master...Eric-Arellano:decouple-changed?expand=1 My main goal is “decouple 
--owned
and 
--changed-*
. This is an optimization to remove the 
ChangedRequest
dataclass - not critical, only would be neat to do
a
i’m still not quite sure why those errors are happening atm
h
The second commit does everything I want, I only would like to land the 3rd if possible to make things cleaner
a
why are we removing the ChangedRequest dataclass?
h
It’s a weird wrapper around the 
Changed
subsystem that duplicates it’s functionality. Those methods should live in the subsystem, in a perfect world. We only have 
ChangedRequest
as a way to consume the subsystem outside of V2 rules
a
--query makes use of it effectively
if this seems better to you, go for it
h
Could 
--query
directly request the 
Changed
subsystem? That’s how all other rules work. This is the only instance where we wrap the options defined in a 
Subsystem
a
? not really? i’ve started to use the Factory inner class subsystem pattern a lot to get a dataclass from subsystem options, and i then expose it with a single @rule which converts from the subsystem to a dataclass
i like that this approach decouples “the data a subsystem provides” from “the literal class declaration of a specific subsystem with a specific options_scope”
h
Hm okay I’m going to give up on this part of the PR - not very important to the filesystem specs changes I’m making
👍 1
a
i was also going to say that but didn’t wanna slow you down
❤️ 1
plz lmk if you want to pair on any followup
h
Thanks for talking it over! Will have this up for review in a couple minutes after some cleanup and adding a description 🙂
a
thanks for the work!!!!
❤️ 1
👖 1
w
check the visualization
to see what is needed to compute Changed
we should probably have a CLI-rendered visualization as well for this case.
👍 1
Open in Slack
PreviousNext
Powered by