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

Modify rally to allow multiple cars with complex configuration #1859

Merged
merged 26 commits into from
Jul 10, 2024

Conversation

favilo
Copy link
Contributor

@favilo favilo commented Jun 21, 2024

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.

@favilo favilo requested a review from a team June 21, 2024 22:11
@favilo favilo marked this pull request as ready for review June 21, 2024 22:11
Copy link
Contributor

@gbanasiak gbanasiak left a 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.

tests/mechanic/team_test.py Show resolved Hide resolved
esrally/utils/modules.py Outdated Show resolved Hide resolved
.pre-commit-config.yaml Outdated Show resolved Hide resolved
@gbanasiak
Copy link
Contributor

@favilo Do you think we should add esrally.utils.io and esrally.utils.process to mypy overrides too? My thinking is, if we start adding type hints somewhere, let's mypy do a proper check there, instead of a narrowed-down one. This will raise more mypy errors though, and the scope of PR will increase. Alternatively, we can retract type hint changes from those 2 modules, and add them in a separate PR.

Copy link
Contributor

@gbanasiak gbanasiak left a 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.

@favilo
Copy link
Contributor Author

favilo commented Jul 2, 2024

@favilo Do you think we should add esrally.utils.io and esrally.utils.process to mypy overrides too? My thinking is, if we start adding type hints somewhere, let's mypy do a proper check there, instead of a narrowed-down one. This will raise more mypy errors though, and the scope of PR will increase. Alternatively, we can retract type hint changes from those 2 modules, and add them in a separate PR.

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

@favilo
Copy link
Contributor Author

favilo commented Jul 2, 2024

@gbanasiak I went a little crazy with those two files... Turned on a stricter mode of mypy than I absolutely needed. But this way, we can be sure we are future proof

pyproject.toml Show resolved Hide resolved
This will let it use the correct python environment by default. Ideally.

I will revert if this breaks CI again.
pyproject.toml Outdated Show resolved Hide resolved
Copy link
Contributor

@gbanasiak gbanasiak left a 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.

esrally/utils/io.py Outdated Show resolved Hide resolved
@gbanasiak
Copy link
Contributor

I'm testing simplified mypy config in 2058474.

@gbanasiak
Copy link
Contributor

gbanasiak commented Jul 9, 2024

56ef7e5 changed file object type in FileOffsetTable from generic IO to byte-based IO, but files are opened in text-mode in this IO wrapper. The split() is called against str not bytes at runtime here:

line_number, offset_in_bytes = (int(i) for i in line.strip().split(b";"))

[..]
  File "/opt/buildkite-agent/builds/bk-agent-prod-gcp-1720474499404871070/elastic/rally-it/.nox/it-3-8/lib/python3.8/site-packages/esrally/utils/io.py", line 656, in skip_lines
    offset, remaining_lines = file_offset_table.find_closest_offset(number_of_lines_to_skip)

  File "/opt/buildkite-agent/builds/bk-agent-prod-gcp-1720474499404871070/elastic/rally-it/.nox/it-3-8/lib/python3.8/site-packages/esrally/utils/io.py", line 558, in find_closest_offset
    line_number, offset_in_bytes = (int(i) for i in line.strip().split(b";"))

TypeError: must be str or None, not bytes

Attempt to fix in f5125d2

@favilo favilo merged commit 4ec752d into elastic:master Jul 10, 2024
17 checks passed
@favilo favilo deleted the multiple-hooks branch July 10, 2024 19:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants