Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add DurationOption to store time duration values #19006

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

AlexTereshenkov
Copy link
Member

@AlexTereshenkov AlexTereshenkov commented May 15, 2023

Fixes #18698

I am not sure if this is what we need, but this is what I think what's necessary.

If I understand correctly, we want all options that declare time duration (e.g. https://www.pantsbuild.org/docs/reference-python_tests#codetimeoutcode or https://www.pantsbuild.org/docs/reference-global#remote_cache_read_timeout_millis) to use this type instead of IntOption. To unify the declaration, I suggest using a custom syntax that we publish in the docs:

1h2m3s4ms5us

with support for hours, minutes, seconds, milliseconds, and microseconds. I don't think it would be reasonable to support days (one can always use multiple hours for that anyway). Using a reg exp, a user value would be loaded into datetime.timedelta which will be used internally when passing the values to the scheduler (or any other place).

What I am not sure is how we'd like to use this type for an Option, e.g.

diff --git a/src/python/pants/option/global_options.py b/src/python/pants/option/global_options.py
index e7c4aca861..eb6e4677c9 100644
--- a/src/python/pants/option/global_options.py
+++ b/src/python/pants/option/global_options.py
@@ -1500,9 +1500,9 @@ class BootstrapOptions:
         removal_hint="Use the `remote_cache_rpc_timeout_millis` option instead.",
         help="Timeout value for remote cache lookups in milliseconds.",
     )
-    remote_cache_rpc_timeout_millis = IntOption(
+    remote_cache_rpc_timeout_millis = DurationOption(
         advanced=True,
-        default=DEFAULT_EXECUTION_OPTIONS.remote_cache_rpc_timeout_millis,
+        default=f"{DEFAULT_EXECUTION_OPTIONS.remote_cache_rpc_timeout_millis}ms",
         help="Timeout value for remote cache RPCs in milliseconds.",
     )
     remote_execution_address = StrOption(
~
~
~
~

@AlexTereshenkov AlexTereshenkov added category:internal CI, fixes for not-yet-released features, etc. category:user api change and removed category:internal CI, fixes for not-yet-released features, etc. labels May 15, 2023
@AlexTereshenkov
Copy link
Member Author

@tdyas did you have something like this in mind?

@tdyas
Copy link
Contributor

tdyas commented May 15, 2023

@tdyas did you have something like this in mind?

Yes seems like what I had in mind. Although I would also deprecate any option name with a time unit in its name and replace it with a new option without the time unit in the name. So remote_cache_rpc_timeout_millis would become remote_cache_rpc_timeout.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add DurationOption type for duration-type config options
2 participants