https://pantsbuild.org/ logo
Join Slack
Powered by
Stu, I think this means time outs might not be wor...
# development
h
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
…oh. that test doesn’t have a timeout.
and maybe v2 isn’t defaulting…?
h
I thought that we default to 60?
w
unclear… it is 100% possible that the defaulting is not working.
h
timeout_seconds=field_set.timeout.calculate_from_global_options(pytest)
. And we have unit tests to verify that that works properly
w
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
okay, I can fix
w
i’ll give setting that a spin.
h
oh, I see. It’s because we didn’t configure a default, right?
w
yea.
and … i wonder whether we should set/add a default.
h
I’m not sure if we should. Seems frustrating if you’re a new user
w
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
sure, that would be great. Thanks
and possibly set the max value too
unclear if we need that given RBE
w
iirc, we set it in travis. but yea
h
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
@hundreds-father-404: but… yea. thoughts on setting a default here? this feels like something a new repo should have on by default.
h
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
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
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
yea. would be a good place for scoped subsystems.
☝️ 1
h
how is that relevant?
w
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
How does that result in the engine being able to make a better error message?
The error is thrown in Rust
w
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
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
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
Right. Which a user last week wanted. They were bummed 
--cache-test-ignore
went away
w
yea. i think that with this we could bring it back as something consumed by our 
Process
machinery.
h
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
recursive is also different from scoped subsystems, heh
would be happy to nuke recursive 😃
❤️ 1
h
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
so… let’s do that … in the next few weeks i think?
sooo much, argh. sorry. need to keep focused, heh
h
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
if we have any 
recursive
options currently registered, would be good to remove them
h
Yes, agreed with removing recursive in 2.0. Those are a pain to deprecate
w
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
Open in Slack
PreviousNext
Powered by