rptest: more typing 2 #16134
Merged
rptest: more typing 2 #16134
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
travisdowns
requested review from
andrewhsu,
savex,
StephanDollberg and
dotnwat
travisdowns
commented
Jan 17, 2024
| def omb_runner(context, redpanda, driver, workload, omb_config): | ||
| def omb_runner(context: TestContext, redpanda: RedpandaServiceCloud, | ||
| driver: str, workload: dict[str, | ||
| Any], omb_config: ValidatorDict): |
There was a problem hiding this comment.
very weird formatting decision by yapf
travisdowns
commented
Jan 17, 2024
| @@ -216,7 +223,9 @@ class HighThroughputTest(PreallocNodesTest): | |||
| # Default value | |||
| msg_timeout = 120 | |||
|
|
|||
| def __init__(self, test_ctx: TestContext, *args, **kwargs): | |||
| redpanda: RedpandaServiceCloud | |||
There was a problem hiding this comment.
basically the .redpanda member can be either RedpandaService or RedpandaServiceCloud from the typer checker's point of view (and it's not wrong), but with our giant human brains we know that currently this test only runs in the cloud. So we type hint it at the class level so that we get good type checking in this class.
The need to do this will be obviated by some upcoming changes to the base class structure of the tests.
There was a problem hiding this comment.
but with our giant human brains
🤣
travisdowns
commented
Jan 17, 2024
| @@ -244,6 +253,8 @@ def __init__(self, test_ctx: TestContext, *args, **kwargs): | |||
| disable_cloud_storage_diagnostics=True, | |||
| **kwargs) | |||
|
|
|||
| assert isinstance(self.redpanda, RedpandaServiceCloud) | |||
There was a problem hiding this comment.
Runtime check for our type assertion above (type checkers are not smart enough for this assert alone to be enough).
travisdowns
commented
Jan 17, 2024
| @@ -897,17 +913,17 @@ def test_decommission_and_add(self): | |||
|
|
|||
| # Generate a realistic number of segments per partition. | |||
| self.load_many_segments() | |||
| producer = None | |||
| try: | |||
| producer = KgoVerifierProducer( | |||
There was a problem hiding this comment.
Issue found by type checking. This assignment should be outside the try (and the None assignment removed), since otherwise if the construction throws we will deference None in the handler. This same bug repeats several times below. Found by type checking.
travisdowns
commented
Jan 17, 2024
| @@ -1797,9 +1812,6 @@ def _get_metrics(bench): | |||
| producers = 1 * (self._num_brokers // 3) + 1 | |||
| consumers = producers * 2 | |||
|
|
|||
| if partitions not in ["min", "max"]: | |||
There was a problem hiding this comment.
This is subsumed by an assert below.
travisdowns
commented
Jan 17, 2024
| # Select number of partitions | ||
| if partitions == "min": | ||
| _num_partitions = self._partitions_min | ||
| elif partitions == "max": | ||
| else: |
There was a problem hiding this comment.
Example of a runtime change to help the type checker. One of these checks will succeed but the type checker cna't know that, so _num_partitoins could also be Unbond from its PoV. Instead, always enter the last branch and use an assert to check that it has the expected value. Ends up being less code overall anyway since we can remove the explicit check above.
Enum could perhaps accomplish the same thing.
There was a problem hiding this comment.
I don't mind much about this random python testing code so please leave as is but I do find the original way easier to understand. One problem with that though is that the allowed values are duplicated.
Enum could perhaps accomplish the same thing.
I guess this would be the best as then you can avoid duplicating the different enum values, have a "default" handler and probably the type check is also happy.
There was a problem hiding this comment.
@StephanDollberg wrote:
I do find the original way easier to understand.
I agree, the original way is easier to understand though arguably less correct (since it admits a third state after the two checks instead of the desired 2).
The problem though is that it does not type check, so the alternative w/o changing types I think is something adding a third and final else:
if partitions == "min":
_num_partitions = self._partitions_min
elif partitions == "max":
_num_partitions = self._partitions_upper_limit
else:
raise RuntimeError('oh no')
I don't if Enum solves it exactly as it would require the type checker to know that the flow always goes down one of the two branches even though they are not exhaustive (i.e., there is no else clause) through value analysis.
Actuallly since we don't have switch in Python I kind of like using a dict like:
_num_partitions = {
'min': self._partitions_min,
'max': self._partitions_upper_limit
}[partitions]
WDYT? Not sure if it's idiomatic, but it type checks and gives a pretty good error (showing the key value, which is the important part).
There was a problem hiding this comment.
Actuallly since we don't have switch in Python I kind of like using a dict like:
Yeah I like that.
travisdowns
commented
Jan 17, 2024
| verb: str, | ||
| path: str, | ||
| node: MaybeNode = None, | ||
| **kwargs: Any): |
There was a problem hiding this comment.
For args and kwargs we are mostly just stuck using Any since the args captured generally don't have any particular common type.
travisdowns
commented
Jan 17, 2024
| @@ -7,12 +7,14 @@ | |||
| # the Business Source License, use of this software will be governed | |||
| # by the Apache License, Version 2.0 | |||
|
|
|||
| from __future__ import annotations | |||
There was a problem hiding this comment.
Need to allow a class method to accept an argument of the same type as the class itself, e.g.:
class Foo:
def takes_foo(self, another_foo: Foo):
...
travisdowns
commented
Jan 17, 2024
| def get_workload_int(self, key: str) -> int: | ||
| """Get the workload property specified by key: it must exist and be an int.""" | ||
| v = self.workload[key] | ||
| assert isinstance(v, int), f"value {v} for {key} was not an int" |
There was a problem hiding this comment.
do a bit of runtime type checking here
travisdowns
commented
Jan 17, 2024
| @@ -187,6 +188,14 @@ | |||
| ] | |||
|
|
|||
|
|
|||
| class RemoteClusterNode(Protocol): | |||
There was a problem hiding this comment.
Protocol only used by the type checker.
travisdowns
commented
Jan 17, 2024
tests/rptest/services/redpanda.py
Outdated
| @@ -201,8 +210,8 @@ def f(sample): | |||
|
|
|||
|
|
|||
| class MetricsEndpoint(Enum): | |||
| METRICS = 1 | |||
| PUBLIC_METRICS = 2 | |||
| METRICS = '/metrics' | |||
There was a problem hiding this comment.
Simplifies the place this is used below, helps with type checking.
travisdowns
commented
Jan 17, 2024
| @@ -256,6 +266,8 @@ def get_cloud_storage_type(applies_only_on: list(CloudStorageType) = None, | |||
| cloud_storage_type = [CloudStorageType.S3] | |||
| elif cloud_provider == "azure": | |||
| cloud_storage_type = [CloudStorageType.ABS] | |||
| else: | |||
| raise RuntimeError(f"bad cloud provider: {cloud_provider}") | |||
There was a problem hiding this comment.
helps the type checking know this attribute is always set
travisdowns
commented
Jan 17, 2024
|
|
||
| if not self._si_settings.bypass_bucket_creation: | ||
| assert self._si_settings.cloud_storage_bucket, "No SI bucket configured" |
There was a problem hiding this comment.
A lot of asserts had to be added here, as the type checker can see that these proeprties are sometimes null (when cloud storage isn't configured): some outside force presumably ensures we don't run cloud storage-using tests in that scenario, but the type checker doens't know here.
Some additional refactoring/abstraction could help here but I just went the easy route.
|
I threw a few reviewers on here, but I'll merge with 1 approval unless anyone objects. |
travisdowns
force-pushed
the
td-rptest-ducktape-typing
branch
from
694ca5b
to
2cca71a
Compare
|
Force 2cca71a fixes a circular import I introduced between redpanda.py and admin.py, using a small |
|
new failures in https://buildkite.com/redpanda/redpanda/builds/43864#018d1a9b-8772-4faa-a029-1d12e7b11343: new failures in https://buildkite.com/redpanda/redpanda/builds/43864#018d1a9b-877c-4a6b-b327-c88883f9fe7f: new failures in https://buildkite.com/redpanda/redpanda/builds/43864#018d1a9b-8776-4275-8bce-06e9263439ef: new failures in https://buildkite.com/redpanda/redpanda/builds/43864#018d1a9b-8779-48c6-910d-d71354f51027: new failures in https://buildkite.com/redpanda/redpanda/builds/43905#018d1e45-8746-42b0-b67e-53cecdbf14ad: new failures in https://buildkite.com/redpanda/redpanda/builds/43905#018d1e45-8740-4956-91c8-60268959f457: |
StephanDollberg
previously approved these changes
Jan 18, 2024
| @@ -267,6 +278,7 @@ def __init__(self, test_ctx: TestContext, *args, **kwargs): | |||
| config_profile['machine_type']] | |||
|
|
|||
| tier_product = self.redpanda.get_product() | |||
| assert tier_product, "Could not get product into " | |||
There was a problem hiding this comment.
"Could not get product info"
There was a problem hiding this comment.
Fixed in a subsequent big change to this same file, hope that's alright.
There was a problem hiding this comment.
Did you push this? Might be blind but can't see it.
Edit: Discussed per pm. Will be in a follow up.
| # Select number of partitions | ||
| if partitions == "min": | ||
| _num_partitions = self._partitions_min | ||
| elif partitions == "max": | ||
| else: |
There was a problem hiding this comment.
I don't mind much about this random python testing code so please leave as is but I do find the original way easier to understand. One problem with that though is that the allowed values are duplicated.
Enum could perhaps accomplish the same thing.
I guess this would be the best as then you can avoid duplicating the different enum values, have a "default" handler and probably the type check is also happy.
| counts = {self.idx(node): None for node in self.nodes} | ||
| counts: dict[int, int | None] = { | ||
| self.idx(node): None | ||
| for node in self.nodes |
There was a problem hiding this comment.
This formatting is also slightly confusing
Type more parts of rptest, concentrating on getting high_throughput_test to type fully at pyright "basic" setting. After this change it types fully with some additional ducktape stubbing (not part of this change). This change is largely pure typing (which can't affect runtime) though there are a handful of small runtime changes to assist in the typing, e.g., asserts that properties are not None immediately prior to using them which would otherwise by a typing failure. This only changes a crash at use to a crash immediately before at the assert.
travisdowns
force-pushed
the
td-rptest-ducktape-typing
branch
from
2cca71a
to
de179ec
Compare
StephanDollberg
approved these changes
Jan 19, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Type more parts of rptest, concentrating on getting high_throughput_test to type fully at pyright "basic" setting.
After this change it types fully with some additional ducktape stubbing (not part of this change).
This change is largely pure typing (which can't affect runtime) though there are a handful of small runtime changes to assist in the typing, e.g., asserts that properties are not None immediately prior to using them which would otherwise be a typing failure. This only changes a crash at use to a crash immediately before at the assert.
Backports Required
Release Notes