https://pantsbuild.org/ logo
Join Slack
Powered by
re: per-workunit/per-rule logging
# development
w
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
I'm not sure this makes sense for rule subgraphs
w
most log systems are per-file, but it’s not particularly meaningful
h
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
you always do.
h
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
@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
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
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
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
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
this kind of stuff
what stuff
w
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
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
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
@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
PS we already have a subsystem-level 
--level
option, because that option is recursive, so it's registered on every scope.
w
@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
Yes, you have to read it from your subsystem options instead of from global options
h
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
@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
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
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
Strong agreement with removing all 
recursive
options. Pants 2.0 is the time to do that
👍 1
w
(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
Yes, this sounds great to me. Getting rid of 
recursive=True
options removes some complexity
👍 1
💯 1
w
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.
Open in Slack
PreviousNext
Powered by