-
Notifications
You must be signed in to change notification settings - Fork 419
feat(test-fill,test-specs): add t8n call caching to test specs #2084
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
base: forks/amsterdam
Are you sure you want to change the base?
feat(test-fill,test-specs): add t8n call caching to test specs #2084
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## forks/amsterdam #2084 +/- ##
================================================
Coverage 86.07% 86.07%
================================================
Files 599 599
Lines 39472 39472
Branches 3780 3780
================================================
Hits 33977 33977
Misses 4862 4862
Partials 633 633
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| session_t8n.reset_traces() | ||
| session_t8n.call_counter = 0 | ||
| session_t8n.debug_dump_dir = dump_dir_parameter_level | ||
| # TODO: Configure the transition tool to count opcodes only when required. |
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.
Regarding this comment, we are currently counting opcodes every single time, but we could improve this by doing so only for tests that are marked to require it.
We could mark the tests that need this information with a new marker (pytest.mark.count_opcodes or similar). Perhaps we can set it at a folder level, e.g. for all tests in tests/benchmarking.
if request.node.get_closest_marker("count_opcodes"):
session_t8n.reset_opcode_count()
else:
session_t8n.remove_opcode_count()or similar.
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.
Created tracking issue so we can merge without this: #2102
marioevz
left a comment
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.
Running the latest version I see that we are adding @t8n-cache-<md5-hash> to the names of all tests for some reason. This seems incorrect to me, so I'd like to give this a proper re-review.
This is to ensure that all parametrized test formats (state_test, blockchain_test, blockchain_test_engine) for a single test case get distributed to the same xdist worker to ensure they use the same per-worker cache; the cache is not global across workers. |
a294da7 to
0b598ec
Compare
0b598ec to
bfae73d
Compare
bfae73d to
069d10c
Compare
|
Just a general comment (haven't looked at the code yet), but are the caches per-thread/per-process or global? If they aren't global, you might need to load group them to get the biggest benefit. |
- Enable pytest-xdist loadgroup distribution mode by default. - Required for xdist_group markers to control worker assignment.
- Add strip_fixture_format_from_nodeid() to extract base nodeid. - Add get_all_fixture_format_names() for format name lookup. - Used to ensure related fixture formats share cache keys.
- Add T8nOutputCache LRU cache class for storing t8n outputs.
- Add t8n_output_cache field to FillingSession.
- Add xdist_group markers during collection for --dist=loadgroup.
- Use t8n-cache-{hash} prefix to distinguish from user-defined groups.
- Strip cache-specific @t8n-cache-* suffix from nodeids in TestInfo.
- Add cache key helpers to BaseTest (_get_base_nodeid, _get_t8n_cache_key). - Add _get_filling_session() to access cache from test instances. - Cache t8n outputs in _generate_block_data() for reuse across formats. - Skip caching for engine_x and engine_sync variants (different execution).
- Test T8nOutputCache LRU behavior, eviction, and hit/miss tracking. - Test strip_fixture_format_from_nodeid for various nodeid patterns. - Test get_all_fixture_format_names ordering and contents. - Test cache key consistency across fixture format variants. - Test _strip_xdist_group_suffix preserves non-cache group markers.
Add tests to verify that test items are sorted during collection to ensure deterministic cache hits. The tests demonstrate: - Sorting groups related fixture formats by base nodeid. - Without xdist, items are correctly sorted. - With xdist, items are NOT sorted (BUG causing high variance). - Expected vs actual behavior comparison. The xfail test `test_xdist_sorting_required_for_cache_hits` asserts the correct behavior (sorting with xdist) and fails until the fix is applied.
- Add helper methods to TransitionToolCacheStats for serialization. - Initialize aggregated stats on xdist controller in pytest_configure. - Send worker stats via workeroutput in fixture teardown. - Add pytest_testnodedown hook to aggregate stats from workers. - Update pytest_terminal_summary to display aggregated stats.
- Clear `_cache` in `remove_cache()` to prevent stale data leakage. - Tests without `transition_tool_cache_key` (e.g., state_test) could previously retrieve cached results from prior tests via matching `call_counter` subkeys.
Sort test items by (is_slow, base_nodeid, nodeid) to optimize execution: - Slow tests first (LPT scheduling for xdist load balance). - Related fixture formats grouped together (cache locality). - Deterministic order within groups. If ANY fixture format variant of a test is marked slow, ALL variants are treated as slow to keep them grouped together for cache hits. Reuses the base_nodeid cache for xdist marker generation to avoid redundant strip_fixture_format_from_node calls.
BlockchainEngineXFixture and BlockchainEngineSyncFixture had can_use_cache=False which was dead code (never checked anywhere). Replace with transition_tool_cache_key="" which is the actual mechanism that controls caching — empty string means no caching.
For StateTest specs with --generate-all-formats, the _from_state_test label suffixes cause alphabetical sort to interleave cacheable and non-cacheable formats: blockchain_test_engine_from_state_test (cacheable) → blockchain_test_engine_x_from_state_test (non-cacheable, clears cache) → blockchain_test_from_state_test (cacheable, but cache is gone). Add has_cache_key to the sort key so cacheable formats cluster together within each base nodeid group, ensuring the second cacheable format hits the warm cache before any non-cacheable format clears it.
node_id_for_entropy strips fixture format and fork names from the node ID before hashing it for deterministic address generation. However, it did not strip the xdist @group_name suffix (e.g., @t8n-cache-abc12345), causing different addresses when running with vs without xdist workers. Strip the suffix so addresses are deterministic regardless of whether xdist is active.
Replace the raw hit/miss counts with an efficiency metric where 100% means all tests that could have hit the cache did hit it. Track unique cache keys to compute expected hits (total cacheable - unique keys). Also filter subkey stats to only count cacheable tests, eliminating phantom misses from non-cacheable tests that still interact with the OutputCache after remove_cache(). Before: T8n cache: key_hits=6, key_misses=6 (50.0%), subkey_hits=6, subkey_misses=18 (25.0%) After: T8n cache: 100% hit rate (6/6 expected), 6 t8n calls saved
069d10c to
f57d1d5
Compare
🗒️ Description
Add an in-memory LRU cache for transition tool outputs during fixture generation. When multiple fixture formats share the same t8n inputs (e.g.,
blockchain_testandblockchain_test_engine), the cache eliminates redundant t8n calls.Changes
T8nOutputCacheclass with bounded LRU eviction.strip_fixture_format_from_nodeid()to derive consistent cache keys.BlockchainTest._generate_block_data().xdist_groupmarkers to keep related formats on the same worker.Performance
Command (with xdist), note this also generates the
state_testformat, which doesn't benefit from caching:Benchmarked on
tests/shanghai(1408 tests across all forks through Osaka) with 6 parallel workers:Sequential:
Parallel (6 workers):
🔗 Related Issues or PRs
N/A.
✅ Checklist
toxchecks to avoid unnecessary CI fails, see also Code Standards and Enabling Pre-commit Checks:uvx tox -e statictype(scope):.mkdocs servelocally and verified the auto-generated docs for new tests in the Test Case Reference are correctly formatted.Cute Animal Picture