-
Notifications
You must be signed in to change notification settings - Fork 211
Add benchmark sweeper configuration and auto-override for benchmark type #1554
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: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Tadiwa Magwenzi <[email protected]>
_target_: hydra._internal.core_plugins.basic_sweeper.BasicSweeper | ||
max_batch_size: null | ||
params: | ||
'application_workers': 1, 4, 16, 64, 128 |
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.
So we don't miss it later, can we pls remove 128 app workers from here as discussed?
@@ -3,6 +3,7 @@ | |||
|
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 could be misunderstanding something but is it possible to move all this default configuration (upto line 31) into base.yml
too and refer to that as @hydra.main(version_base=None, config_path="hydra/sweeper", config_name="base")
or so?
We could also then move the benchmark specific static/default configs to their respective config files, for e.g. mountpoint.mountpoint_max_background: !!null # or say, 64
can move into the Fio yml file so it's easier to see what all defaults we're configuring and which all are then being overridden in the sweeper
params: | ||
'application_workers': 1, 4, 16, 64, 128 | ||
'iteration': "range(${iterations})" | ||
'mountpoint.fuse_threads': 1, 16, 64 |
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 don't think this should be here. It should either be in fio.yml
for now or a separate mountpoint.yml which the Fio one (and others in the future) can inherit this configuration from
@@ -0,0 +1,3 @@ | |||
# @package hydra.sweeper | |||
params: | |||
'benchmarks.prefetch.max_memory_target': null, 1073741824, 2147483648 # null, 1GB, 2GB |
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.
Should this mem_limiter config really be turned on by default?
@@ -0,0 +1,3 @@ | |||
# @package hydra.sweeper | |||
params: | |||
'benchmarks.client_backpressure.read_window_size': 8388608, 2147483648 # 8MB, 2GB |
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.
While we're at it, let's also think about what reasonable values we would want here. Let's not put 8MB as a test, as it's too small and might not be too meaningful. I think 2GB, 8GB and maybe 16GB are the values I'd suggest testing here.
max_batch_size: null | ||
params: | ||
'application_workers': 1, 4, 16, 64, 128 | ||
'iteration': "range(${iterations})" |
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.
Can we pls make this default to 10 (or something >1) here?
if arg.startswith("benchmark_type="): | ||
benchmark_type = arg.split("=", 1)[1] | ||
break |
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.
Do we stay backward compatible and default to benchmark_type=fio
here as well so we don't miss sweeping the extra params, given we already silently default to fio benchmark with benchmark_type: "fio"
?
if benchmark_type: | ||
sweeper_override = f"+hydra/benchmark_sweeper={benchmark_type}" | ||
has_sweeper_override = any( | ||
arg.startswith("+hydra/benchmark_sweeper=") or arg.startswith("hydra/benchmark_sweeper=") | ||
for arg in sys.argv[1:] | ||
) | ||
|
||
if not has_sweeper_override: | ||
# we add the sweeper override before the benchmark_type argument and find position of benchmark_type argument | ||
for i, arg in enumerate(sys.argv): | ||
if arg.startswith("benchmark_type="): | ||
sys.argv.insert(i, sweeper_override) | ||
break | ||
else: | ||
sys.argv.insert(1, sweeper_override) |
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'm not sure I understand. Do you mind elaborating how this works or a sample usage? Where is the hydra/benchmark_sweeper=
value coming from?
Can we do something like below, or maybe I'm missing something?
def get_config_name():
env = os.getenv("benchmark_type", "fio")
config_map = {
"fio": "config_fio",
"prefetch": "config_prefetch",
"client_bp": "config_client_bp",
"client": "config_client",
"crt": "config_crt"
}
return config_map.get(env, "config")
@hydra.main(version_base=None, config_path="conf/hydra/sweeper", config_name=get_config_name())
def run_experiment(cfg: DictConfig) -> None:
blah blah
What changed and why?
Fixed an issue where benchmark parameters for unselected benchmark types were incorrectly included in multirun sweeps. Previously, all benchmark parameters were defined in the common sweeper config, causing irrelevant parameters to be swept even when running specific benchmark types.
Changes:
benchmark_type
parameter to automatically select appropriate sweeper configfio.yaml
,prefetch.yaml
,client-bp.yaml
, etc.)base.yaml
sweeper configExample: Running
benchmark_type=fio
now only sweeps FIO-specific parameters instead of including unrelated prefetch or client-backpressure parameters.Does this change impact existing behavior?
No breaking changes. Existing commands work as before, but now sweep only relevant parameters for the specified benchmark type.
Does this change need a changelog entry? Does it require a version change?
None needed
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license and I agree to the terms of the Developer Certificate of Origin (DCO).