Duration/Memory Convenience Click Types #448
Conversation
Added the `DurationParamType` for specifying custom durations in a user friendly way for CLI interfaces along with corresponding unit tests.
Also added unit tests, but still need to add documentation.
Fleshed out documentation for the `gempyor.shared_cli` module. Added the ability to optionally return the converted memory as an int instead of as a float to `MemoryParamType`. Removed pre-instantiated custom click param types.
Bug in `MemoryParamType.convert` was failing if a unit was not given with a value. Similar to `DurationParamType` it assumes that numbers without a unit are given in the default unit of the type.
TimothyWillard
requested review from
pearsonca,
jcblemai,
fang19911030,
saraloo,
MacdonaldJoshuaCaleb and
emprzy
TimothyWillard
added
enhancement
There was a problem hiding this comment.
Seems mostly fine. One question about forbidding years (seems like the type should conceptually support it, even if we don't actually yet or don't want to accept values that large).
Also feels like shared_cli might be getting a bit bloated (even before adding these).
Does it make sense to start having a click extensions submodule or somesuch? And then shared_cli is importing generic capabilities from there?
|
|
||
|
|
||
| @pytest.mark.parametrize("nonnegative", (True, False)) | ||
| @pytest.mark.parametrize("value", ("abc", "$12.34", "12..3", "12years", "12.a2")) |
There was a problem hiding this comment.
is forbidding "years" about that being conceptually wrong or simply a duration we don't want to contemplate? seems as a generic thing, its appropriate for the tool to potentially support years, so a bit weird to explicitly test that it doesn't.
There was a problem hiding this comment.
Two issues with years: 1) years do not have a constant length (2024 was 366 days but most years are 365 days), and 2) python's timedelta class which doesn't have a years argument: https://docs.python.org/3/library/datetime.html#datetime.timedelta.
I could however see an argument for adding micro/milliseconds on conceptual ground if you would like? I just excluded those since it was too short for most practical use cases.
There was a problem hiding this comment.
so this is a duration we explicitly don't want to contemplate (primarily because it's an ambiguous unit for our case) - that's fine! It does feel worth noting somewhere that we don't allow this unit of time - like maybe in the method documentation? But I don't think we need, say, a special error message for years.
re ms - I think it's fine to not support it at this time. I think you're saying this would be one that we'd be willing to contemplate (i.e. we aren't testing to ensure its exclusion), but that we haven't bothered to yet support (because we don't think its practically useful).
Do you want me to move these to |
If that's the standard approach to these kind of external-dependency extension/customizations, that seems the best way to manage it. |
|
Docs question: where are we capturing how bare numbers behave for these types? As in, where would I look, other than code, to see if |
I'll document. |
* Added parameters to explicitly control the behavior of unitless values to both `DurationParamType` and `MemoryParamType`. * Fleshed out documentation for custom types.
Extracted `DurationParamType` and `MemoryParamType` out of `gempyor.shared_cli` into `gempyor._click`.
|
Describe your changes.
This pull request add convenience click types for durations and memory. This is particularly useful in specifying time/memory limits for batch jobs in a user friendly way (i.e.
--time=30minor--memory=4GB). While memory is probably specific to batch, durations could be used in other actions such as specifying forecast intervals.Does this pull request make any user interface changes? If so please describe.
The user interface changes are adding
DurationParamTypeandMemoryParamTypetogempyor.shared_cli, but these changes are new and have no affect on existing user interfaces.Those are reflected in updates to the documentation in docstrings of added functionality.
What does your pull request address? Tag relevant issues.
This pull request was spun out of GH-394, while it does not correspond to an exact issue it does help move GH-365 along by shrinking that PR's size.