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

hundreds-father-404

11/24/2021, 8:40 PM
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

fast-nail-55400

11/24/2021, 9:06 PM
Could it accept a number and a string in the position?
h

hundreds-father-404

11/24/2021, 9:06 PM
What do you mean?
f

fast-nail-55400

11/24/2021, 9:06 PM
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

hundreds-father-404

11/24/2021, 9:07 PM
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

fast-nail-55400

11/24/2021, 9:08 PM
re putting “in seconds” everywhere, isn’t that already the only supported unit currently?
h

hundreds-father-404

11/24/2021, 9:09 PM
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

curved-television-6568

11/24/2021, 9:25 PM
could be another field, too..
python_tests(timeout_after="2m30s")
.. ? 😉
h

hundreds-father-404

11/24/2021, 9:43 PM
what's the benefit of another field? like we have both mechanisms? that seems like overkill
h

happy-kitchen-89482

11/24/2021, 9:45 PM
"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

hundreds-father-404

11/24/2021, 9:47 PM
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

fast-nail-55400

11/24/2021, 9:55 PM
supporting both number and string would ensure we don’t break existing BUILD files
1
h

hundreds-father-404

11/24/2021, 9:55 PM
And we could make me happy if we didn't document the legacy approach, but still accepted it. Thoughts?
f

fast-nail-55400

11/24/2021, 10:00 PM
But that would leave users confused if they looked at the help text to see if the existing syntax were still valid, right?
h

happy-kitchen-89482

11/24/2021, 10:24 PM
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

hundreds-father-404

11/24/2021, 10:25 PM
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

happy-kitchen-89482

11/24/2021, 10:29 PM
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

hundreds-father-404

11/24/2021, 10:32 PM
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

happy-kitchen-89482

11/24/2021, 10:32 PM
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

hundreds-father-404

11/24/2021, 10:33 PM
Great!
h

happy-kitchen-89482

11/24/2021, 10:34 PM
My strong reluctance is to break existing BUILD files, if we can ingest them without change then all is well
👍 1
i

incalculable-yacht-75851

11/25/2021, 4:22 AM
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

witty-crayon-22786

11/29/2021, 7:03 PM
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

hundreds-father-404

12/01/2021, 6:57 PM
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