https://pantsbuild.org/ logo
Powered by
w

witty-crayon-22786

06/24/2020, 8:27 PM
re: per-workunit/per-rule logging
the idea would be to be able to set log levels per workunit/rule subgraph
ie:
Copy code
--levels={
	'pants.backend.python.rules.pex.Pex': debug, 
}
…which would set that log level for rules below the 
Pex
type in the rule graph
cc @hundreds-father-404, @hundreds-breakfast-49010
h

hundreds-breakfast-49010

06/24/2020, 8:29 PM
I'm not sure this makes sense for rule subgraphs
w

witty-crayon-22786

06/24/2020, 8:30 PM
most log systems are per-file, but it’s not particularly meaningful
h

hundreds-breakfast-49010

06/24/2020, 8:30 PM
it means that pants administrators have to have a reasonably sophisticated idea of what the rules are (and how to specify them) that are relevant to debugging their problem
👍 1
w

witty-crayon-22786

06/24/2020, 8:30 PM
you always do.
h

hundreds-breakfast-49010

06/24/2020, 8:30 PM
I'm also not sure how this would work with the 
EngineAware
log-level changing (I have a PR that uses this functionality nearly ready)
w

witty-crayon-22786

06/24/2020, 8:31 PM
@hundreds-breakfast-49010: that’s the “write” side… this is the “read” side
currently the “read” side is set by 
-ldebug
, which is a recursive option (boo)
h

hundreds-father-404

06/24/2020, 8:32 PM
I’m concerned about leaking file names to end users in the options system. That would make it much harder to move around symbols and to rename things
w

witty-crayon-22786

06/24/2020, 8:32 PM
it means that pants administrators have to have a reasonably sophisticated idea of what the rules are (and how to specify them) that are relevant to debugging their problem
i don’t see what the alternative is when debugging a problem. it’s just a question of whether it’s file level, class level, rule-graph level, etc.
@hundreds-father-404: we will already need to commit to making some of these types public API
so there would be relative stability.
h

hundreds-father-404

06/24/2020, 8:33 PM
Yes, but public to plugin authors is very different than public to the options system and every day users For example, with plugin authors, we can re-export the symbol and do a deprecation
w

witty-crayon-22786

06/24/2020, 8:33 PM
but i don’t expect this kind of stuff to be left in place for long… it would be very noisy.
For example, with plugin authors, we can re-export the symbol and do a deprecation
same here?
h

hundreds-father-404

06/24/2020, 8:34 PM
this kind of stuff
what stuff
w

witty-crayon-22786

06/24/2020, 8:34 PM
debugging
this launched out of GPW needing a way to get debug logs for PEX, which you can do by setting the log level: https://github.com/pantsbuild/pants/blob/ee9ca02ba891803861862a28f04b66784cf2453f/src/python/pants/backend/python/rules/pex.py#L281-L304
but you can only set the log level globally currently… and setting 
-ldebug
globally is rarely workable
h

hundreds-father-404

06/24/2020, 8:36 PM
Right. So I think it makes a lot of sense to have a subsystem option like 
--python-setup-level
, 
--pex-level
, or 
--pytest-level
My concern is leaking to end users every single file and symbol that we’ve written as now being valid to use in the options system
👍 1
h

happy-kitchen-89482

06/24/2020, 8:38 PM
Setting log levels per module is idiomatic, hence 
getLogger(__name__)
.
I think it's fine for now, end users are only setting this when debugging
w

witty-crayon-22786

06/24/2020, 8:39 PM
@happy-kitchen-89482: sure, but we’ve discussed how that needs to be adapted in the presence of rules. this is sortof a continuation of that conversation
@happy-kitchen-89482: well, re: fine for now: we have no way of configuring per-module right now
h

happy-kitchen-89482

06/24/2020, 8:40 PM
PS we already have a subsystem-level 
--level
option, because that option is recursive, so it's registered on every scope.
w

witty-crayon-22786

06/24/2020, 8:40 PM
@hundreds-father-404: we do, but it’s a 
recursive
option, which as discussed yesterday is problematic… almost nothing consumes it.
@happy-kitchen-89482: right. which means it doesn’t actually work to configure module-level logging.
h

happy-kitchen-89482

06/24/2020, 8:40 PM
Yes, you have to read it from your subsystem options instead of from global options
h

hundreds-father-404

06/24/2020, 8:41 PM
Fwit, I’m more open to leaking file names if we make very clear that individual file names are not a public API. We are making no guarantees that the value 
pants.backend.python.rules.pex.Pex
will continue working across versions. So, you shouldn’t ever check into VCS the value in a config file. It’s only for ephemeral debugging
we do, but it’s a recursive option, which as discussed yesterday is problematic… almost nothing consumes it.
Right, which is why I keep advocating for call sites needing to explicitly opt in to them having this option. The issue isn’t with subsystems having the option; the issue is that they have it no matter what, even if they don’t consume it
w

witty-crayon-22786

06/24/2020, 8:42 PM
@hundreds-father-404: i think that’s fine. in practice, we will need to mark a bunch of these public APIs. maybe not that one, but yea.
The issue isn’t with subsystems having the option; the issue is that they have it no matter what, even if they don’t consume it
right. what i’m proposing is something else. native support in the engine for configuring per-subgraph levels.
h

hundreds-father-404

06/24/2020, 8:43 PM
in practice, we will need to mark a bunch of these public APIs. maybe not that one, but yea.
Yes, but only public to plugin authors. Not to every-day users. Every-day users shouldn’t need to know about the internals of Pants’ implementation
w

witty-crayon-22786

06/24/2020, 8:45 PM
so, does everyone understand the skeleton of what i’m proposing? if we agree that 
recursive
options don’t really work in practice, this is potentially a graph-aware path forward for 
--level
Copy code
src/python/pants/option/global_options.py
169-            "-l",
170-            "--level",
171-            type=LogLevel,
172-            default=<http://LogLevel.INFO|LogLevel.INFO>,
173:            recursive=True,
--
859-        register(
860-            "--colors",
861-            type=bool,
862-            default=sys.stdout.isatty(),
863:            recursive=True,
--
923-        register(
924-            "-q",
925-            "--quiet",
926-            type=bool,
927:            recursive=True,
--
941-        register(
942-            "--fail-fast",
943-            advanced=True,
944-            type=bool,
945:            recursive=True,
--
950-        register(
951-            "--cache-key-gen-version",
952-            advanced=True,
953-            default="200",
954:            recursive=True,
--
968-            "--max-subprocess-args",
969-            advanced=True,
970-            type=int,
971-            default=100,
972:            recursive=True,
h

hundreds-father-404

06/24/2020, 8:48 PM
Strong agreement with removing all 
recursive
options. Pants 2.0 is the time to do that
👍 1
w

witty-crayon-22786

06/24/2020, 8:50 PM
(and for folks who missed that conversation: scoped `Subsystem`s are probably fine, and different from recursive options because they’re opt in)
also, i think that what i’m proposing would “just work” for pex: it requests the 
LogLevel
, which is a singleton currently (here: https://github.com/pantsbuild/pants/blob/ee9ca02ba891803861862a28f04b66784cf2453f/src/python/pants/backend/python/rules/pex.py#L307-L344 )
being able to set in a sub-graph would just mean that the 
LogLevel
would not be a singleton… instead, a 
Param
that might be different in different subgraphs
(and one that was actually consumed by the engine itself to set log levels)
i’m actually pretty sure that this is sound. the big open question might just be what the config looks like.
h

happy-kitchen-89482

06/24/2020, 8:57 PM
Yes, this sounds great to me. Getting rid of 
recursive=True
options removes some complexity
👍 1
💯 1
w

witty-crayon-22786

06/24/2020, 8:59 PM
cool… so then the thing to nail down would be what the config looks like. i still think that “type-centric” would work well. but the role that that is playing is basically just “select a subgraph”… so it could be an 
@rule
“name” too…?
it feels like there is a connection to scoped subsystems as well. but don’t want to go off the deep end here. the good thing about scoped subsystems is that they’re explicit. and this is closer to being intrinsic.
took a walk, and the one additional thought that i had here is that perhaps a “scope” in the scoped subsystem sense could be an alias for something else. ie, we could essentially assign an alias to a particular request for a 
Subsystem
dumped here: https://github.com/pantsbuild/pants/issues/10160 … maybe this really is all “logging” related, and sticking to language conventions around logging would make sense. unclear.
Powered by