Modify rally to allow multiple cars with complex configuration#1859
Modify rally to allow multiple cars with complex configuration#1859favilo merged 26 commits intoelastic:masterfrom
Conversation
There was a problem hiding this comment.
I've tested with:
esrally install --distribution-version=8.14.0 --node-name="rally-node-0" --network-host="127.0.0.1" --http-port=39200 --master-nodes="rally-node-0" --seed-hosts="127.0.0.1:39300" --team-path=../rally-teams --car="defaults,x-pack-security,apm-tracing" --car-params=/tmp/car_params.json
and confirmed both X-Pack and APM Tracing (elastic/rally-teams#85) configuration was included.
The change LGTM overall. Thank you.
Apart from the enhancement I like the fact that type hints are introduced here and there. However, current mypy configuration in Rally is extremely narrow after #1798 (prior to that PR there were no mypy checks at all), so in 13f1272 I've added override section where we could add modules that we are revisiting to include hints. For starters, I've included the following modules, which is a subset of modules touched in the PR:
[[tool.mypy.overrides]]
module = [
"esrally.mechanic.team",
"esrally.utils.modules",
]
[..]
Feel free to add remaining modules changed in the PR (make lint to trigger mypy check).
Majority of 13f1272 is about fixing unit tests, but I also had to fix some details in the introduced hints.
mypy was introduced through pre-commit and now I'm no longer happy with this approach. pre-commit creates a dedicated virtual environment so we need to specify additional dependencies separately and explicitly (--install-types is not recommended as per https://github.com/pre-commit/mirrors-mypy). I would prefer to have a single source of dev optional dependencies and include them both in pyproject.toml and mypy configuration. We can streamline this later.
|
@favilo Do you think we should add |
I will add those modules in, I like the idea of decreasing debt continuously. And I still need this branch around for my config to load properly |
|
@gbanasiak I went a little crazy with those two files... Turned on a stricter mode of |
Also revert naked `raise` back to `SystemSetupError`. I'd only done that for debugging purposes
It is the fastest way to get this working without writing my own Importer
I turned on a stricter mode than what we have configured in pyproject.toml
This will let it use the correct python environment by default. Ideally. I will revert if this breaks CI again.
gbanasiak
left a comment
There was a problem hiding this comment.
The changes in esrally.utils.io and esrally.utils.process LGTM.
|
I'm testing simplified mypy config in 2058474. |
lines between overridden type definitions
This ensures we are using a single return type for `read()` and friends.
|
56ef7e5 changed file object type in Line 554 in 56ef7e5 Attempt to fix in f5125d2 |
We'd made some decisions in the past to only allow a single car with a python based configuration.
I'm not certain of the reasoning behind that, but I assume it was because this simplified the evaluation of cars.
This changes that to allow multiple cars to run with complex python based configuration. In particular, it allows us to run APM Tracing and X-Pack-Security simultaneously, which is something I need for benchmarking reasons.