-
Couldn't load subscription status.
- Fork 185
feat(fill): add --gas-benchmark-values command to support single genesis file
#1895
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
feat(fill): add --gas-benchmark-values command to support single genesis file
#1895
Conversation
8675c6c to
d0413c8
Compare
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.
Looks great! I think this a great change for maintainability and make it easier for us to generate more vectors as they are required.
Small downside is that we have to remove Environment().gas_limit from most of tests, but I would say we do it the earlier the better.
cc @jsign for some feedback on my comments.
Thanks!
|
Nice! @LouisTsai-Csie @marioevz, is this compatible with supporting all the test formats too? (i.e. #1778). Mostly asking since I think this is coming from the fact of simplifying the single genesis for perfnets, but wondering if it should still be fine for the other formats that we need for zkVMs. |
Should be compatible out of the box, but I'll give that a look again and raise if the there's any concerns. |
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.
Thanks, this looks great @LouisTsai-Csie!
Shame, that this didn't occur to me up front in #1891, but I'd suggest that we move this codeto a new plugin that gets activated with fill by default. This should work well due to the composability of pytest plugins.
This means, we:
- Add these changes (and other benchmarking related pytest config, if any) to a separate pytest plugin, I'd suggest
src/pytest_plugins/filling/benchmarking.py. - Enable this plugin using
-pvia thefillcommand's pytest ini:
execution-spec-tests/src/cli/pytest_commands/pytest_ini_files/pytest-fill.ini
Lines 11 to 22 in 0f7c73a
addopts = -p pytest_plugins.concurrency -p pytest_plugins.filler.pre_alloc -p pytest_plugins.filler.filler -p pytest_plugins.filler.ported_tests -p pytest_plugins.filler.static_filler -p pytest_plugins.shared.execute_fill -p pytest_plugins.forks.forks -p pytest_plugins.eels_resolver -p pytest_plugins.help.help --tb short --ignore tests/cancun/eip4844_blobs/point_evaluation_vectors/
All benchmarking-related plugin customizations (e.g. pytest_addoption, pytest_generate_tests, etc.) currently in filler/filler.py can be moved directly to filler/benchmarking.py. This keeps the benchmarking logic self-contained. Pytest hooks from both modules should compose as expected.
To cleanly handle options/values that are specific to benchmarking, I'd suggestion the following approach, if you agree/like it feel free to go for it!
1. Define a filling mode enum in filler/filler.py:
from enum import StrEnum, unique
@unique
class FillMode(StrEnum):
CONSENSUS = "consensus"
BENCHMARKING = "benchmarking"2. In the filler plugin (filler.py), set the default:
from _pytest.config import Config
from .filler import FillMode
def pytest_configure(config: Config) -> None:
if not hasattr(config, "fill_mode"):
config.fill_mode = FillMode.CONSENSUS3. In the benchmarking plugin (filler/benchmarking.py), override only if --benchmark-gas-values is set:
from _pytest.config import Config
from .filler import FillMode
def pytest_configure(config: Config) -> None:
if config.getoption("--benchmark-gas-values") is not None:
config.fill_mode = FillMode.BENCHMARKING4. Example usage in filler logic, wrapped in a fixture:
import pytest
from ,filler import FillMode
GIGA_GAS = 1_000_000_000
@pytest.fixture
def env() -> Environment: # noqa: D103
return 1_000_000_000)
if config.fill_mode == FillMode.BENCHMARKING:
return Environment(gas_limit=GIGA_GAS)
else:
return Environment()d0413c8 to
946de75
Compare
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.
Thanks! This looks great to me!
One comment below.
|
@danceratopz Thank you for review, but I am wondering the following:
|
I don't think it's strictly necessary for the PR, but some sanity check that the flag works is nice, of course. Recently, I've been pointing Claude at unit testing tasks.
Does this work? If so, its' because of the macOS trick found in these lines (you can then set the env var locally): Lines 56 to 58 in dfdd433
|
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.
Looks great! I did my suggestions locally and execute is working with the new flag! 🎉
fa71d8d to
7e5f501
Compare
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.
Awesome work, I would like to see if we could rebase and use gas_benchmark_value in all benchmark tests so we can prepare for the next benchmark release if possible.
7e5f501 to
ca18d04
Compare
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.
LGTM, thanks!
…nesis file (ethereum#1895) * feat(fill): add benchmark gas valu command to support single genesis file * refactor(tests): update benchmark test for supported command * refactor(benchmark): consolidate benchmark configurations into a single entry * doc(fill): update command description and changelog * chore(fill): remove legacy gas benchmark values command * refactor(fill): create gas benchmakr value pytest plugin * test(fill): add pytest plugin test and update state test * refactor(fill): add env fixture for benchmarking with gas limit configuration * refactor: support both fill and execute mode * fix: update ci flag and test command
🗒️ Description
This PR introduces a new fill option,
--gas-benchmark-values. Supply a comma-separated list of gas amounts (in millions) to set the values used during benchmarking.The PR also adds two example tests in
tests/benchmark/test_worst_blocks.py. To generate their fixtures, run:Flag
--generate-pre-alloc-groupsis required for the enginex fixture format.The command creates two directories:
fixtures/blockchain_tests_engine_x/benchmark/worst_blocksfixtures/blockchain_tests_engine_x/pre_allocBecause only one
preAllocGroupis produced, this process generates a single genesis file.To generate the genesis file, please follow the documentation to run
hivelocally and run theextract_configcommandFor example:
uv run extract_config --fixture fixtures/blockchain_tests_engine_x/pre_alloc/0x10763c36b27696c5.jsonI would prefer to refactor the benchmark test in a separate PR, this task is updated in the issue.
I’ve reviewed the Filling Test section, and I see that the command and flag descriptions are generated by this script. However, I’m happy to contribute additional documentation if needed.
For pytest plugin test cases, I add three cases, you could run with the following command:
Case 1: Verify the
--gas-benchmark-valuesflag is addedCase 2: Verify the flag works as expected if provided
Case 3: Verify the non-benchmark test is not affected.
🔗 Related Issues or PRs
Issue #1891
✅ Checklist
toxchecks to avoid unnecessary CI fails, see also Code Standards and Enabling Pre-commit Checks:uvx --with=tox-uv tox -e lint,typecheck,spellcheck,markdownlinttype(scope):.mkdocs servelocally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.@ported_frommarker.