-
Notifications
You must be signed in to change notification settings - Fork 313
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
Modify rally to allow multiple cars with complex configuration #1859
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am happy with current form, only non-critical comments left.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.