https://pantsbuild.org/ logo
#development
Title
# development
h

hundreds-father-404

06/23/2020, 10:40 PM
Stu, I think this means time outs might not be working locally? I can’t get it to timeout locally. Note that the code path is different than remote timeouts
w

witty-crayon-22786

06/24/2020, 12:19 AM
…oh. that test doesn’t have a timeout.
and maybe v2 isn’t defaulting…?
h

hundreds-father-404

06/24/2020, 12:19 AM
I thought that we default to 60?
w

witty-crayon-22786

06/24/2020, 12:20 AM
unclear… it is 100% possible that the defaulting is not working.
h

hundreds-father-404

06/24/2020, 12:20 AM
timeout_seconds=field_set.timeout.calculate_from_global_options(pytest)
. And we have unit tests to verify that that works properly
w

witty-crayon-22786

06/24/2020, 12:21 AM
might be good to double check. unit tests aren’t integration tests
(personally, i’ve never seen a v2 test timeout that didn’t have a timeout set)
Copy code
17:23:04.48 [INFO] Completed: Run Pytest for src/python/pants/engine:tests
17:23:04.49 [INFO] Tests failed: src/python/pants/engine:tests
𐄂 src/python/pants/engine:tests
Exceeded timeout of Some(30s) for local process execution, Run Pytest for src/python/pants/engine:tests
works when i set it to 30s locally
looking
Copy code
--pytest-timeout-default=<int>
      default: None
      The default timeout (in seconds) for a test target if the timeout field is not
      set on the target.
mm. yea, we only set it for
test.pytest
oh boy, heh.
h

hundreds-father-404

06/24/2020, 12:26 AM
okay, I can fix
w

witty-crayon-22786

06/24/2020, 12:26 AM
i’ll give setting that a spin.
h

hundreds-father-404

06/24/2020, 12:27 AM
oh, I see. It’s because we didn’t configure a default, right?
w

witty-crayon-22786

06/24/2020, 12:27 AM
yea.
and … i wonder whether we should set/add a default.
h

hundreds-father-404

06/24/2020, 12:27 AM
I’m not sure if we should. Seems frustrating if you’re a new user
w

witty-crayon-22786

06/24/2020, 12:28 AM
unclear… it’s one of those things that’s hard to do later, but easy to do at the beginning
@hundreds-father-404: dodging the larger question for now: i can get a PR out to set that for us?
h

hundreds-father-404

06/24/2020, 12:30 AM
sure, that would be great. Thanks
and possibly set the max value too
unclear if we need that given RBE
w

witty-crayon-22786

06/24/2020, 12:30 AM
iirc, we set it in travis. but yea
h

hundreds-father-404

06/24/2020, 12:30 AM
the sole reason we had it for v1 is to avoid Travis complaining about no output. But we don’t run sequentially anymore, so I don’t think it makes as much sense
iirc, we set it in travis. but yea
only on the task for v1, not the subsystem for v2
w

witty-crayon-22786

06/24/2020, 12:32 AM
@hundreds-father-404: but… yea. thoughts on setting a default here? this feels like something a new repo should have on by default.
h

hundreds-father-404

06/24/2020, 12:33 AM
Eh, maybe 5 minutes is a sane sensible default? Idk, I’m concerned about running a really long IT for the very first time and then Pants ending it. That’s not a nice experience
w

witty-crayon-22786

06/24/2020, 12:34 AM
alternatively: “ah, the system has my back”
i think that to the extent that timeouts are frustrating and low information, we should make them less frustrating, rather than not having them
h

hundreds-father-404

06/24/2020, 12:35 AM
If the error message said “Timed out! Set
--pytest-timeout-default
. See https://…“` But we can’t do that because the error message is from the engine
Is there a path forward to enrich error messages from the engine like this? That’s I think one of our biggest risks is that errors in the engine must be very very generic
w

witty-crayon-22786

06/24/2020, 12:36 AM
yea. would be a good place for scoped subsystems.
☝️ 1
h

hundreds-father-404

06/24/2020, 12:36 AM
how is that relevant?
w

witty-crayon-22786

06/24/2020, 12:37 AM
strawdesign: 1) have an optional
Processes
subsystem, 2) expect @rules to scope it, 3) consume a reference to it on
Process
[processes.pytest] timeout_default
h

hundreds-father-404

06/24/2020, 12:38 AM
How does that result in the engine being able to make a better error message?
The error is thrown in Rust
w

witty-crayon-22786

06/24/2020, 12:38 AM
because it would consume the subsystem
rather than values from the subsystem
3) consume a reference to it on 
Process
so then it could indicate how to pass an option for the subsystem.
… oh. and that would also allow for scoped cache-ignoring.
fyi @happy-kitchen-89482: ^
the conversation with GPW might also tie into this general theme… could additionally allow for passing more arguments to subprocesses on a scope by scope basis.
i’m actually pretty convinced by this. will tackle it tomorrow morning.
h

hundreds-father-404

06/24/2020, 2:13 AM
I’m still a little scarred from subscopes because of the confusion that
lint.mypy
still runs all of MyPy. But that’s an irrational fear - we’re not adding back task scopes
w

witty-crayon-22786

06/24/2020, 2:13 AM
roughly the same as the strawdesign above, to allow for 1) per-process cache-disabling, 2) per-process timeout setting, 3) per-process arg extension
yea, that’s a totally different thing
that’s “goals containing tasks” rather than scoped subsystems
👍 1
the best example of a scoped subsystem around today is
--cache-$x
h

hundreds-father-404

06/24/2020, 2:15 AM
Right. Which a user last week wanted. They were bummed
--cache-test-ignore
went away
w

witty-crayon-22786

06/24/2020, 2:16 AM
yea. i think that with this we could bring it back as something consumed by our
Process
machinery.
h

hundreds-father-404

06/24/2020, 2:16 AM
My one pitch would be to be very careful making it automatic and/or recursive. I think recursive options were the source of a lot of challenges with v1, like
--skip
being recursive
w

witty-crayon-22786

06/24/2020, 2:16 AM
recursive is also different from scoped subsystems, heh
would be happy to nuke recursive 😃
❤️ 1
h

hundreds-father-404

06/24/2020, 2:18 AM
Me too. Too much danger with it. There should be a little bit of boilerplate to add an option - it’s a public API we commit to.
w

witty-crayon-22786

06/24/2020, 2:19 AM
so… let’s do that … in the next few weeks i think?
sooo much, argh. sorry. need to keep focused, heh
h

hundreds-father-404

06/24/2020, 2:20 AM
I don’t think it’s a blocker for 2.0. It’s a nice-to-have
We can add it later still. It’s adding a new feature, not taking away something
w

witty-crayon-22786

06/24/2020, 2:20 AM
if we have any
recursive
options currently registered, would be good to remove them
h

hundreds-father-404

06/24/2020, 2:20 AM
Yes, agreed with removing recursive in 2.0. Those are a pain to deprecate
w

witty-crayon-22786

06/24/2020, 7:42 PM
opened https://github.com/pantsbuild/pants/issues/10156 about this… the open questions make me think that it’s probably not worth rushing into to tackle GPW’s problem