https://pantsbuild.org/ logo
#general
Title
# general
p

polite-garden-50641

01/17/2020, 12:18 AM
How would I access options defined on a GoalSubsystem subclass from logic implemented in a Subsystem class (specifically the one we use as a streaming_workunits_handlers) ? I just need the option values, not the instance of the GoalSubsystem subclass itself. @hundreds-father-404 @hundreds-breakfast-49010
h

hundreds-father-404

01/17/2020, 12:19 AM
That is, he wants to access the values of a
GoalSubsystem
in the method of a
Subsystem
, not in a rule Normally, we’d do this with
MySubsystem.global_instance()
, but `global_instance isn't defined on
GoalSubsystem
and I’m unclear if it should be? Any ideas? CC @witty-crayon-22786
👍 1
a

aloof-angle-91616

01/17/2020, 12:26 AM
i didn't realize a
GoalSubsystem
wouldn't be a
Subsystem
?
h

hundreds-father-404

01/17/2020, 12:27 AM
No, it implements
Optionable
and
SubsystemClientMixin
but not
Subsystem
. I’m not sure why that is, though?
a

aloof-angle-91616

01/17/2020, 12:28 AM
does git blame say anything useful?
p

polite-garden-50641

01/17/2020, 12:31 AM
h

hundreds-father-404

01/17/2020, 12:34 AM
FYI this is the PR that first added the idea of a goal subsystem, then called `Goal.Options`: https://github.com/pantsbuild/pants/pull/6880
w

witty-crayon-22786

01/17/2020, 12:35 AM
Because it wasn't a subystem before it was renamed... It was a Goal
h

hundreds-father-404

01/17/2020, 12:36 AM
But the inner class
Goal.Options
inherited
SubsystemClientMixin
and
Optionable
, rather than
Subsystem
. Do you remember why that is?
w

witty-crayon-22786

01/17/2020, 12:36 AM
I don't have any particular concerns with having it extend Subsystem, but you'd want to look at that.
👍 1
p

polite-garden-50641

01/17/2020, 12:36 AM
for run_tracker, we pass it all the options from
local_pants_runner
https://github.com/pantsbuild/pants/blob/master/src/python/pants/bin/local_pants_runner.py#L270 maybe we should just do the same for stream handlers ?
w

witty-crayon-22786

01/17/2020, 12:38 AM
For code that is sufficiently close to the core, it can be fine to access options directly, yea.
p

polite-garden-50641

01/17/2020, 12:39 AM
@hundreds-breakfast-49010 ^^
w

witty-crayon-22786

01/17/2020, 12:39 AM
But... It's a bit confusing to access the options of a Goal here. What's the usecase?
👍 1
@polite-garden-50641 ^
p

polite-garden-50641

01/17/2020, 12:41 AM
in our case, the
GoalSubsystem
subsystem implements the logic (and rules) to handle auth with our servers (OAuth flow, storing access tokens, etc...) so our stream handlers implementation needs to access the options to figure out where the access token is stored in order to load & use it.
w

witty-crayon-22786

01/17/2020, 12:42 AM
Why not move that to a Subsystem?
h

hundreds-father-404

01/17/2020, 12:42 AM
I think for namespacing. We want to be able to do
./pants auth --foo
rather than
./pants --auth-subsystem-foo
w

witty-crayon-22786

01/17/2020, 12:42 AM
It may not be specific to the goal if you're trying to use it elsewhere
p

polite-garden-50641

01/17/2020, 12:43 AM
I am not against that... I don't understand the semantics enough to have an opinion. @hundreds-father-404 wdyt ? should just make the auth options a regular subsystem ? can it then be consumed by other subsystems (and goals/rules) ?
ahh..Eric makes a good point...
h

hundreds-father-404

01/17/2020, 12:45 AM
Yes, if you make it a normal subsystem, then it could be used by other subsystems without needing any changes to Pants. Otherwise, we need to likely change Pants so that
GoalSubsystem
inherits
Subsystem
(which possibly makes sense - would need to audit more) The main downside I see is the namespace, as pointed out above.
w

witty-crayon-22786

01/17/2020, 12:45 AM
"auth" as a verb is one thing, but it sounds like there are options independent of that verb.
In general, Subsystem options have (in the past) been more frequently specified in
pants.ini
because they don't change with every invoke
h

hundreds-breakfast-49010

01/17/2020, 12:46 AM
yeah, if an option can be read outside of the
auth
goal, maybe that's a sign that it shouldn't be namespaced under
auth
h

hundreds-father-404

01/17/2020, 12:55 AM
Some of the options we have defined are: -
--base-url
-
--local-port
-
--output
(enum type) -
--from-env-var
Conceptually, I think it does make sense to have these options live in the goal, just as we have
./pants login --to
as an option. They modify how the goal runs. This goal is pretty simple - it doesn’t wrap any other tool like
Flake8
. It’s convenient and natural to be able to do
./pants auth --output=console
, as opposed to
./pants --authentication-output=console auth
-- The only subsystem names I could come up with are
AuthSettings
and
Authentication
, which is confusing why that’s distinct from the
AuthOptions(GoalSubsystem)
. That is, there’s no “natural” subsystem that comes to my mind, unlike the
Flake8
,
PythonSetup
, and
ScalaFix
subsystems, which all map to something very naturally.
h

hundreds-breakfast-49010

01/17/2020, 12:56 AM
@hundreds-father-404 so maybe that's an argument that
GoalSubsystem
should be a normal
Subsystem
that you can call
global_instance
on
h

hundreds-father-404

01/17/2020, 12:57 AM
Yes, I think that
GoalSubsystem
should inherit
Subsystem
. I can’t think of a strong conceptual motivation why we would not want that. (We should, though, still keep
GoalSubsystem
as a distinct class so that we can distinguish between normal subsystems. A
Goal.subsystem_cls
should only allow
GoalSubsystem
instances, not general
Subsystem
instances)
👍 1
w

witty-crayon-22786

01/17/2020, 1:00 AM
@hundreds-father-404: the options from https://pantsbuild.slack.com/archives/C046T6T9U/p1579222502123800?thread_ts=1579220301.118200&cid=C046T6T9U look like repo wide defaults rather than goal specific config
Why wouldn't you put those in pants.ini?
h

hundreds-father-404

01/17/2020, 1:01 AM
Indeed, they normally are defined in
pants.ini
under the scope
auth
w

witty-crayon-22786

01/17/2020, 1:01 AM
So then it shouldn't matter whether they are on a goal or not?
h

hundreds-breakfast-49010

01/17/2020, 1:03 AM
goals implicitly introduce namespacing that might be desirable
👍 1
h

hundreds-father-404

01/17/2020, 1:03 AM
then it shouldn’t matter whether they are on a goal or not?
What matters is the namespace. We want the options to live under the namespace
auth
(the same namespace as the goal). It’s confusing for us to have options defined on some subsystem called
Authentication
, so that you have to do
--authentication-local-port
rather than
--auth-local-port
. There’s no compelling reason for a new options scope, other than the constraints of Pants
h

hundreds-breakfast-49010

01/17/2020, 1:03 AM
the alternative is to invent a pseudo-namespace by just carefully naming the top-level rules
👍 1
w

witty-crayon-22786

01/17/2020, 1:04 AM
This seems related to the question of whether @rules should be able to depend on goal options... came up in the context of test coverage
👍 1
(which I don't have an answer to, really)
h

hundreds-breakfast-49010

01/17/2020, 1:04 AM
if an @rule needs them, then they should be exposed as requestable types, like what I did for GlobalOptions a while ago
but that doesn't apply to requesting them in a Subsystem
w

witty-crayon-22786

01/17/2020, 1:05 AM
It feels like there are compositional/circular concerns
h

hundreds-father-404

01/17/2020, 1:06 AM
I agree that these are related. I see no reason that normal
@rules
should not be able to request a
GoalSubsystem
h

hundreds-breakfast-49010

01/17/2020, 1:06 AM
why's that? options can conceptually be RootRules, right? since before engine execution pants should have access to those options?
on the other hand, yeah, nothing is stopping a rule from requesting
.global_instance
either
so maybe goal options don't need to be rule inputs
w

witty-crayon-22786

01/17/2020, 1:07 AM
.global_instance is the v1 way... In a purely v2 run it will fail, I think
h

hundreds-father-404

01/17/2020, 1:07 AM
It feels like there are compositional/circular concerns
This would be true with
Goal.Options
, but now `GoalSubsystem`s can be used without ever needing to touch the
Goal
. They’re still closely coupled, but decoupled enough that it’s safe for a normal rule to consume the
GoalSubsystem
without any circular dependency on the
Goal
h

hundreds-breakfast-49010

01/17/2020, 1:07 AM
hm, global_instance isn't safe to use in a v2 rule?
h

hundreds-father-404

01/17/2020, 1:08 AM
hm, global_instance isn’t safe to use in a v2 rule?
No, you should request the subsystem in your rule signature and that subsystem should be registered via a
subsystem_rule()
☝️ 1
w

witty-crayon-22786

01/17/2020, 1:08 AM
@hundreds-father-404: one class or two is not what made them coupled... Moreso the conceptual connection
h

hundreds-father-404

01/17/2020, 1:15 AM
Conceptually, I view it as the
GoalSubsystem
being able to stand on its own, whereas the
Goal
needs the
GoalSubsystem
to work. For example, I think it does make sense to have in
pants.ini
options defined on your goal, like
[dependencies] transitive: True
because that’s the default you decide you want for your organization. It makes sense to have that option configured without running
./pants dependencies
. That is, the goal options conceptually can stand on their own.
👍 1
w

witty-crayon-22786

01/17/2020, 2:14 AM
Which would be fine if you were actually using them (only) in the goal: but you're not
As you said: it's to help namespace things in a case where you've already used a good verb that is also a noun.
3 Views