https://pantsbuild.org/ logo
Join Slack
Powered by
A poll, votes and comments appreciated! Right now ...
# general
h
A poll, votes and comments appreciated! Right now you write test timeouts like this:
Copy code
python_tests(timeout=120)
Should the value instead be like this?
Copy code
"120s"
"10s"
"2m"
"1.5m"
A for the status quo, B for setting the time unit Update going to accept both styles to avoid breaking BUILD files
🅰️ 1
🅱️ 5
f
Could it accept a number and a string in the position?
h
What do you mean?
f
i.e., convert using the units if a string but treat as seconds if a number
so 
timeout=120
and 
timeout="2m"
would both be valid
h
Yeah we could do that. Not sure it's worth the ambiguity tho, that we have to put in our help strings everywhere "(in seconds)". Optimize for reading BUILD files over writing them (Btw this idea comes from looking at how Go handles timeouts)
f
re putting “in seconds” everywhere, isn’t that already the only supported unit currently?
h
Yes, we only support seconds currently. But we have to make that clear in our help message because you can't be 100% sure from looking at only this BUILD file what the unit is:
Copy code
python_tests(timeout=5)
Probably 5 seconds, but could be 5 minutes
The help message
Copy code
timeout
    type: int | None
    default: None
    A timeout (in seconds) used by each test file belonging to this target.
c
could be another field, too.. 
python_tests(timeout_after="2m30s")
.. ? 😉
h
what's the benefit of another field? like we have both mechanisms? that seems like overkill
h
"120s" is preferable in general, but I fear that changing the type now would be disruptive to users and with only limited benefit
Since the unit is fairly obviously seconds in practice...
I am very loth to break existing BUILD files any more than we have to
👍 2
And this seems low priority?
h
changing the type now would be disruptive to users
could be mitigated by 
update-build-files
I more want to get this right for Go. I've been adding timeout support today. By extension, we would then "fix" Python and Shell to use the preferred approach
low priority?
only a priority in that I want to get Go right
f
supporting both number and string would ensure we don’t break existing BUILD files
➕ 1
h
And we could make me happy if we didn't document the legacy approach, but still accepted it. Thoughts?
f
But that would leave users confused if they looked at the help text to see if the existing syntax were still valid, right?
h
I'm confused as to why this needs to change for Go? We can convert our ints into whatever the Go cmd line expects?
h
Indeed, which is what I'm doing. I'm saying I'd like to get the UX right for our Go plugin before we commit to something suboptimal
h
If we change the field type from int to string, we can still ingest 
120
on the cmd line or in env vars, and just assume seconds. What happens with toml? Can an unquoted 
120
be read as a string type?
Oh wait this is in a target, nm
h
Yep! So the 
timeout
field uses Target API and we can teach it to be type 
str | int | None
for 
pants.toml
, what's relevant is 
[pytest].timeout_default
, which should also use this same affordance imo. Just verified that I can change 
type=str
and using an int in the TOML file still works
h
So if we can continue to ingest existing BUILD files and config files as-is, then I see no problem in also accepting strings
💯 1
we can document that seconds is the default unit
No need to be coy about it
h
Great!
h
My strong reluctance is to break existing BUILD files, if we can ingest them without change then all is well
👍 1
i
I: • prefer proper units • prefer fewest options • prefer not to introduce new parameters This problem touches all of these and they are at odds. Since I'm very new to both pants and python I don't really have a strong opinion... However, is this change necessary? What would happen if you didn't make this change?
w
fwiw, bazel has test sizes rather than per-test timeout values. i think that i like per-test precise values more, but something to consider
👍 2
h
Since I'm very new to both pants and python I don't really have a strong opinion...
Thank you for sharing your feedback, @incalculable-yacht-75851! It's a particularly helpful perspective, you don't have the curse of knowledge https://en.wikipedia.org/wiki/Curse_of_knowledge Agreed with those preferences.
However, is this change necessary?
Not strictly. What we have works, only is suboptimal and I was hesitant to add timeouts to Go using a suboptimal implementation But I'm going to put this change down for now to focus on higher priority things. Will create a ticket to summarize the idea and discussion
Open in Slack
PreviousNext
Powered by