Split bench_cfg.py into focused configuration modules #688
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Reviewer's GuideRefactors the monolithic bencher.bench_cfg module into a bench_cfg package of focused configuration classes, introduces a compositional BenchRunCfg with dynamic delegation to sub-configs, and preserves backward compatibility via re-exports and attribute delegation, with tests covering defaults, composition, and import behavior. Sequence diagram for BenchRunCfg initialization and delegated attribute accesssequenceDiagram
actor User
participant BenchRunCfg
participant BenchPlotSrvCfg as ServerCfg
participant CacheCfg
participant ExecutionCfg
participant DisplayCfg
participant VisualizationCfg
participant TimeCfg
User->>BenchRunCfg: new BenchRunCfg(cache_results=True, repeats=5)
activate BenchRunCfg
Note over BenchRunCfg: __init__(**params)
BenchRunCfg->>BenchPlotSrvCfg: create default instance
BenchRunCfg->>CacheCfg: create default instance
BenchRunCfg->>ExecutionCfg: create default instance
BenchRunCfg->>DisplayCfg: create default instance
BenchRunCfg->>VisualizationCfg: create default instance
BenchRunCfg->>TimeCfg: create default instance
Note over BenchRunCfg: Route legacy flat params via _DELEGATION_MAP
BenchRunCfg->>CacheCfg: set cache_results=True
BenchRunCfg->>ExecutionCfg: set repeats=5
BenchRunCfg-->>User: BenchRunCfg instance
deactivate BenchRunCfg
User->>BenchRunCfg: cfg.cache_results
activate BenchRunCfg
Note over BenchRunCfg: __getattr__("cache_results")
BenchRunCfg->>CacheCfg: get cache_results
CacheCfg-->>BenchRunCfg: True
BenchRunCfg-->>User: True
deactivate BenchRunCfg
User->>BenchRunCfg: cfg.repeats = 10
activate BenchRunCfg
Note over BenchRunCfg: __setattr__("repeats", 10)
BenchRunCfg->>ExecutionCfg: set repeats=10
deactivate BenchRunCfg
Class diagram for refactored benchmark configuration classesclassDiagram
class BenchPlotSrvCfg {
<<param.Parameterized>>
+int port
+bool allow_ws_origin
+bool show
}
class CacheCfg {
<<param.Parameterized>>
+bool cache_results
+bool clear_cache
+bool cache_samples
+bool only_hash_tag
+bool clear_sample_cache
+bool overwrite_sample_cache
+bool only_plot
}
class ExecutionCfg {
<<param.Parameterized>>
+int repeats
+int level
+Executors executor
+bool nightly
+bool headless
}
class DisplayCfg {
<<param.Parameterized>>
+bool summarise_constant_inputs
+bool print_bench_inputs
+bool print_bench_results
+bool print_pandas
+bool print_xarray
+bool serve_pandas
+bool serve_pandas_flat
+bool serve_xarray
}
class VisualizationCfg {
<<param.Parameterized>>
+bool auto_plot
+bool use_holoview
+bool use_optuna
+bool render_plotly
+bool raise_duplicate_exception
+int plot_size
+int plot_width
+int plot_height
}
class TimeCfg {
<<param.Parameterized>>
+bool over_time
+bool clear_history
+str time_event
+str run_tag
+datetime run_date
}
class BenchRunCfg {
<<param.Parameterized>>
+BenchPlotSrvCfg server
+CacheCfg cache
+ExecutionCfg execution
+DisplayCfg display
+VisualizationCfg visualization
+TimeCfg time
+deep()
+from_cmd_line()
+__getattr__(name)
+__setattr__(name, value)
}
class BenchCfg {
<<param.Parameterized>>
+list input_vars
+list result_vars
+list const_vars
+list result_hmaps
+list meta_vars
+list all_vars
+list iv_time
+list iv_time_event
+str name
+str title
+bool raise_duplicate_exception
+str bench_name
+str description
+str post_description
+bool has_results
+bool pass_repeat
+str tag
+str hash_value
+list plot_callbacks
+hash_persistent(include_repeats)
+inputs_as_str()
+to_latex()
+describe_sweep(width, accordion)
+sweep_sentence()
+describe_benchmark()
+to_title(panel_name)
+to_description(width)
+to_post_description(width)
+to_sweep_summary(name, description, describe_sweep, results_suffix, title)
+optuna_targets(as_var)
}
class DimsCfg {
+list dims_name
+list dim_ranges
+list dims_size
+list dim_ranges_index
+list dim_ranges_str
+dict coords
}
BenchRunCfg o-- BenchPlotSrvCfg : server
BenchRunCfg o-- CacheCfg : cache
BenchRunCfg o-- ExecutionCfg : execution
BenchRunCfg o-- DisplayCfg : display
BenchRunCfg o-- VisualizationCfg : visualization
BenchRunCfg o-- TimeCfg : time
BenchCfg --|> BenchRunCfg
DimsCfg ..> BenchCfg : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 6 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `bencher/bench_cfg/run_cfg.py:147-149` </location>
<code_context>
+ **params,
+ )
+
+ def __getattr__(self, name):
+ """Delegate attribute access to sub-configs for backward compatibility."""
+ if name in _DELEGATION_MAP:
+ sub_name = _DELEGATION_MAP[name]
+ sub_cfg = object.__getattribute__(self, sub_name)
</code_context>
<issue_to_address>
**issue (bug_risk):** Fallback in __getattr__ should delegate to super().__getattr__ to avoid breaking Parameterized behavior.
Because this implementation raises `AttributeError` for any name not in `_DELEGATION_MAP`, it bypasses `param.Parameterized`'s own `__getattr__` handling and can break dynamic parameters or other attributes `Parameterized` normally resolves. Instead, delegate non-mapped names to `super().__getattr__(name)` and only raise if that also fails, e.g.
```python
def __getattr__(self, name):
if name in _DELEGATION_MAP:
sub_name = _DELEGATION_MAP[name]
sub_cfg = object.__getattribute__(self, sub_name)
return getattr(sub_cfg, name)
return super().__getattr__(name)
```
</issue_to_address>
### Comment 2
<location> `test/test_bench_cfg.py:57-66` </location>
<code_context>
+class TestBenchRunCfgComposition(unittest.TestCase):
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests to verify that sub-config instances are not shared between BenchRunCfg copies
The `repeats` parameter exercises the `deep()` helper, but only at the top level. Please also assert that `cfg.cache is not copy.cache` (and likewise for other sub-configs) and that mutating `copy.cache.cache_results` does not affect `cfg.cache.cache_results`. This will ensure sub-configs are truly deep-copied and protect against future shared-state regressions.
Suggested implementation:
```python
self.assertIsInstance(cfg.display, DisplayCfg)
self.assertIsInstance(cfg.visualization, VisualizationCfg)
self.assertIsInstance(cfg.time, TimeCfg)
def test_sub_configs_not_shared_between_copies(self):
"""Sub-config instances are deep-copied, not shared, when using repeats."""
cfg = BenchRunCfg()
# Set a known value so we can verify it does not change when mutating the copy.
cfg.cache.cache_results = False
# Create a copy that exercises the deep() helper via the repeats parameter.
cfg_copy = cfg.copy(repeats=2)
# Sub-config objects themselves must not be shared between cfg and cfg_copy.
self.assertIsNot(cfg.cache, cfg_copy.cache)
self.assertIsNot(cfg.execution, cfg_copy.execution)
self.assertIsNot(cfg.display, cfg_copy.display)
self.assertIsNot(cfg.visualization, cfg_copy.visualization)
self.assertIsNot(cfg.time, cfg_copy.time)
# Mutating a sub-config on the copy must not affect the original.
cfg_copy.cache.cache_results = True
self.assertTrue(cfg_copy.cache.cache_results)
self.assertFalse(cfg.cache.cache_results)
```
If `BenchRunCfg` does not expose a `copy(repeats=...)` API, update the line `cfg_copy = cfg.copy(repeats=2)` to call whichever method/constructor actually uses the `repeats` parameter and internally invokes the `deep()` helper (for example, `cfg_copy = cfg.with_repeats(2)` or similar). The rest of the test assertions can remain as-is.
</issue_to_address>
### Comment 3
<location> `test/test_bench_cfg.py:80-87` </location>
<code_context>
+ _ = cfg.over_time
+ _ = cfg.port
+
+ def test_flat_parameter_init(self):
+ """Flat parameters can be set via constructor."""
+ cfg = BenchRunCfg(cache_results=True, repeats=5, auto_plot=False)
+ self.assertTrue(cfg.cache_results)
+ self.assertEqual(cfg.repeats, 5)
+ self.assertFalse(cfg.auto_plot)
+
+ def test_flat_parameter_setter(self):
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test case for initializing BenchRunCfg with explicit sub-config objects (new-style API)
Current tests only cover flat-parameter construction on `BenchRunCfg`, not the new composition API where callers pass `cache=CacheCfg(...)`, `execution=ExecutionCfg(...)`, etc. Please add a test that builds `BenchRunCfg(cache=CacheCfg(cache_results=True), execution=ExecutionCfg(repeats=7))` and asserts both the grouped (`cfg.cache.cache_results`, `cfg.execution.repeats`) and flat (`cfg.cache_results`, `cfg.repeats`) attributes to validate this path.
```suggestion
def test_flat_parameter_init(self):
"""Flat parameters can be set via constructor."""
cfg = BenchRunCfg(cache_results=True, repeats=5, auto_plot=False)
self.assertTrue(cfg.cache_results)
self.assertEqual(cfg.repeats, 5)
self.assertFalse(cfg.auto_plot)
def test_subconfig_init(self):
"""Sub-config objects can be passed explicitly and are reflected in flat attributes."""
cfg = BenchRunCfg(
cache=CacheCfg(cache_results=True),
execution=ExecutionCfg(repeats=7),
)
# grouped access
self.assertTrue(cfg.cache.cache_results)
self.assertEqual(cfg.execution.repeats, 7)
# flat access
self.assertTrue(cfg.cache_results)
self.assertEqual(cfg.repeats, 7)
def test_flat_parameter_setter(self):
```
</issue_to_address>
### Comment 4
<location> `test/test_bench_cfg.py:70-85` </location>
<code_context>
+ self.assertIsInstance(cfg.time, TimeCfg)
+ self.assertIsInstance(cfg.server, BenchPlotSrvCfg)
+
+ def test_flat_parameter_access(self):
+ """Parameters accessible directly on BenchRunCfg."""
+ cfg = BenchRunCfg()
+ # Should not raise
+ _ = cfg.cache_results
+ _ = cfg.repeats
+ _ = cfg.auto_plot
+ _ = cfg.over_time
+ _ = cfg.port
+
+ def test_flat_parameter_init(self):
</code_context>
<issue_to_address>
**suggestion (testing):** Consider testing error behavior of delegated attribute access for unknown attributes
Current tests only verify successful delegation. Please add a test that accessing/setting an unknown attribute (e.g. `cfg.nonexistent_param`) raises `AttributeError`, and confirm this is consistent for both `BenchRunCfg` and `BenchCfg`, to guard against future delegation-map regressions.
```suggestion
def test_flat_parameter_access(self):
"""Parameters accessible directly on BenchRunCfg."""
cfg = BenchRunCfg()
# Should not raise
_ = cfg.cache_results
_ = cfg.repeats
_ = cfg.auto_plot
_ = cfg.over_time
_ = cfg.port
def test_flat_parameter_init(self):
"""Flat parameters can be set via constructor."""
cfg = BenchRunCfg(cache_results=True, repeats=5, auto_plot=False)
self.assertTrue(cfg.cache_results)
self.assertEqual(cfg.repeats, 5)
self.assertFalse(cfg.auto_plot)
def test_unknown_flat_parameter_access_raises(self):
"""Accessing unknown delegated attributes raises AttributeError."""
for cfg_cls in (BenchRunCfg, BenchCfg):
with self.subTest(cfg_cls=cfg_cls.__name__):
cfg = cfg_cls()
with self.assertRaises(AttributeError):
_ = cfg.nonexistent_param
def test_unknown_flat_parameter_set_raises(self):
"""Setting unknown delegated attributes raises AttributeError."""
for cfg_cls in (BenchRunCfg, BenchCfg):
with self.subTest(cfg_cls=cfg_cls.__name__):
cfg = cfg_cls()
with self.assertRaises(AttributeError):
cfg.nonexistent_param = 42
```
</issue_to_address>
### Comment 5
<location> `test/test_bench_cfg.py:119-128` </location>
<code_context>
+ self.assertEqual(copy.repeats, 10)
+
+
+class TestBenchCfgInheritance(unittest.TestCase):
+ """Test BenchCfg inherits delegation from BenchRunCfg."""
+
+ def test_inherits_flat_access(self):
+ """BenchCfg has flat parameter access."""
+ cfg = BenchCfg(
+ input_vars=[],
+ result_vars=[],
+ const_vars=[],
+ bench_name="test",
+ title="test",
+ cache_results=True,
+ repeats=3,
+ )
+ self.assertTrue(cfg.cache_results)
+ self.assertEqual(cfg.repeats, 3)
+
+ def test_over_time_works(self):
+ """over_time parameter works correctly."""
+ cfg = BenchCfg(
+ input_vars=[],
+ result_vars=[],
+ const_vars=[],
+ bench_name="test",
+ title="test",
+ over_time=True,
+ )
+ self.assertTrue(cfg.over_time)
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Extend BenchCfg tests to cover methods that depend on delegated fields (e.g., hash_persistent, describe_benchmark)
These tests only verify delegated fields individually. Please add a small config-based test that exercises higher-level helpers that combine delegated and BenchCfg-specific attributes, e.g. asserting:
- `hash_persistent(True) != hash_persistent(False)` when `repeats` differs
- `describe_benchmark()` output includes `run_date`, `run_tag`, `level`, `cache_results`, and `cache_samples`.
This will better protect behavior across refactors.
</issue_to_address>
### Comment 6
<location> `bencher/bench_cfg/run_cfg.py:18` </location>
<code_context>
+from bencher.bench_cfg.time_cfg import TimeCfg
+
+# Mapping of delegated attribute names to their sub-config
+_DELEGATION_MAP = {
+ # Server
+ "port": "server",
</code_context>
<issue_to_address>
**issue (complexity):** Consider generating `_DELEGATION_MAP` from the sub-config classes’ `param` definitions so delegation stays in sync without maintaining a large manual map.
You can keep the current behavior (including backwards-compatible flat attributes) while removing the “second source of truth” and a chunk of manual wiring by generating `_DELEGATION_MAP` from the sub-configs’ `param` definitions.
### 1. Replace manual `_DELEGATION_MAP` with introspection
Instead of hand-maintaining every attribute in `_DELEGATION_MAP`, derive the delegation map from the sub-config classes once, so adding a new param only requires touching the sub-config.
```python
# Before: large static map
_DELEGATION_MAP = {
# Server
"port": "server",
"allow_ws_origin": "server",
"show": "server",
# Cache
"cache_results": "cache",
"clear_cache": "cache",
...
}
```
You can replace that with:
```python
_SUBCONFIG_SPECS = {
"server": BenchPlotSrvCfg,
"cache": CacheCfg,
"execution": ExecutionCfg,
"display": DisplayCfg,
"visualization": VisualizationCfg,
"time": TimeCfg,
}
# Optional: if you only want to expose a subset of parameters for each sub-config,
# define an allow-list per sub-config here instead of the large flat map.
_EXPOSED_PARAMS = {
# None/empty means "all param parameters" on that sub-config
"server": None,
"cache": None,
"execution": None,
"display": None,
"visualization": None,
"time": None,
}
_DELEGATION_MAP = {}
for sub_name, cfg_cls in _SUBCONFIG_SPECS.items():
allowed = _EXPOSED_PARAMS[sub_name]
for pname in cfg_cls.param:
if allowed is not None and pname not in allowed:
continue
# skip param system internals if necessary
if pname in ("name", "objects", "parameters"):
continue
if pname in _DELEGATION_MAP:
# in case of a clash, you can decide to raise or keep first
raise ValueError(f"Delegated param '{pname}' defined on multiple sub-configs")
_DELEGATION_MAP[pname] = sub_name
```
Now:
- Adding/removing parameters happens only in `CacheCfg`, `ExecutionCfg`, etc.
- `_DELEGATION_MAP` is always consistent with the sub-config definitions.
- The existing `__getattr__`, `__setattr__`, and `__init__` routing logic work unchanged.
### 2. Minor cleanup in `__init__` routing
You can also make the legacy-parameter routing slightly clearer and less error-prone by using the same `_SUBCONFIG_SPECS` keys and avoiding repeating the literal strings:
```python
def __init__(self, **params):
server = params.pop("server", None) or BenchPlotSrvCfg()
cache = params.pop("cache", None) or CacheCfg()
execution = params.pop("execution", None) or ExecutionCfg()
display = params.pop("display", None) or DisplayCfg()
visualization = params.pop("visualization", None) or VisualizationCfg()
time_cfg = params.pop("time", None) or TimeCfg()
sub_configs = {
"server": server,
"cache": cache,
"execution": execution,
"display": display,
"visualization": visualization,
"time": time_cfg,
}
# Route legacy flat parameters
for key in list(params):
sub_name = _DELEGATION_MAP.get(key)
if sub_name is not None:
setattr(sub_configs[sub_name], key, params.pop(key))
super().__init__(
server=server,
cache=cache,
execution=execution,
display=display,
visualization=visualization,
time=time_cfg,
**params,
)
```
This keeps all existing functionality (flat access, sub-configs, CLI wiring) but eliminates a large manual map and makes the system data-driven and easier to extend.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #688 +/- ##
==========================================
+ Coverage 89.39% 89.58% +0.18%
==========================================
Files 91 99 +8
Lines 4432 4503 +71
==========================================
+ Hits 3962 4034 +72
+ Misses 470 469 -1
🚀 New features to boost your workflow:
|
blooop
reviewed
Dec 26, 2025
| only_hash_tag: bool = param.Boolean( | ||
| False, | ||
| doc="By default when checking if a sample has been calculated before it includes the " | ||
| "hash of the greater benchmarking context. This is safer because it means that data " |
Collaborator
Author
There was a problem hiding this comment.
I thknk the multi-line strings were from ruff auto-formatting - can revert to single lines if preferred
blooop
reviewed
Dec 26, 2025
| default=0, | ||
| bounds=[0, 12], | ||
| doc="The level parameter is a method of defining the number samples to sweep over in a " | ||
| "variable agnostic way, i.e you don't need to specify the number of samples for each " |
Owner
|
I like the look of this in principle, but less sure when it comes to the implementation. Ie before the code was "bad" because it was monolithic, but was relatively simple because it was all flat and made all the hashing logic easy to verify. Now it's "better" and more modular, but seems there there is more room for error and hashing bugs. There is also more logic now to go wrong. Maybe this is covered in the tests? I'm not sure if this will be easier to maintain. |
Collaborator
Author
Your call :) The hashing logic is unchanged - hash_persistent still accesses self.repeats, self.over_time etc. which now delegate to sub-configs but return identical values. The tests verify delegation works and hash behavior is preserved |
Owner
|
Can't test on computer, but does bench_cfg.xxx still auto complete as before? If it does then happy to try this. If it doesn't would an inheritance approach work? |
Owner
|
Now I have been able to check out on my computer and see in more detail, looks good. |
blooop
pushed a commit
that referenced
this pull request
Dec 29, 2025
…rameter access This commit completes the config migration started in PR #688 by fully converting to the new grouped config structure and removing backwards compatibility infrastructure. Key changes: - Converted all flat parameter accesses (e.g., cfg.cache_results) to grouped structure (e.g., cfg.cache.cache_results) - Removed __getattr__ and __setattr__ delegation methods from BenchRunCfg - Removed _build_delegation_map and _DELEGATION_MAP - Simplified BenchRunCfg.__init__ by removing legacy flat parameter routing - Updated all core files: bencher.py, bench_runner.py, sweep_executor.py, result_collector.py - Updated result and plotting files: bench_result_base.py, optuna_result.py, surface_result.py, plt_cnt_cfg.py - Updated bench_cfg_class.py to use grouped access in hash_persistent() and describe_benchmark() - Updated from_cmd_line() to create sub-config objects - Updated documentation to reflect new structure - Updated tests to use new config structure All config parameters now accessed via sub-configs: - cfg.cache.cache_results, cfg.cache.cache_samples, cfg.cache.only_plot - cfg.execution.repeats, cfg.execution.level, cfg.execution.executor - cfg.display.print_bench_inputs, cfg.display.print_pandas - cfg.visualization.auto_plot, cfg.visualization.plot_size - cfg.time.over_time, cfg.time.run_tag, cfg.time.run_date - cfg.server.port, cfg.server.show This simplifies the codebase by removing the complexity of maintaining dual access patterns.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Changes
bencher/bench_cfg/package with focused config classesbench_cfg.pybecomes a thin re-export layerSummary by Sourcery
Modularize benchmark configuration into focused submodules while preserving the existing public API via a thin compatibility layer.
New Features:
Enhancements:
Tests: