-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add back the quantization for LlamaTune unit tests #59
Add back the quantization for LlamaTune unit tests #59
Conversation
@@ -113,7 +113,7 @@ def test_smac_optimization_loop(mock_env_no_noise: MockEnv, smac_opt: MlosCoreOp | |||
"vmSize": "Standard_B2s", | |||
"idle": "mwait", | |||
"kernel_sched_migration_cost_ns": 297669, | |||
"kernel_sched_latency_ns": 290365100, | |||
"kernel_sched_latency_ns": 290365137, |
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.
Shouldn't this be quantized?
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 wish! (and the quantized value should be 300000000). Apparently, none of our optimizers actually samples ConfigSpace for new values directly (as it should) and instead just picks the argmax of the acquisition function and returns this as the new suggestion. That still does not explain the change in the value here, but it is consistent across all branches (main included). Could be a SMAC update?
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.
Hmm, when I was testing this with the older version of SMAC we were getting quantized values back. I think that's the behavior we should expect to see after the monkey patching hack happens, right?
for idx in range(n_integer_params): | ||
input_space.add(CS.UniformIntegerHyperparameter(name=f"int_{idx}", lower=-1, upper=256)) | ||
for idx in range(n_quantized_integer_params): | ||
input_space.add(CS.UniformIntegerHyperparameter(name=f"int_{idx}", lower=0, upper=256, q=16)) | ||
param_float = CS.UniformIntegerHyperparameter(name=f"int_{idx}", lower=0, upper=256) | ||
monkey_patch_quantization(param_float, 17) |
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 wonder if we should add some sort of internal use of meta
to parse q
or quantization_bins
out of that data and use it to apply the monkey patch on demand rather than encoding it explicitly here.
Thoughts?
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.
you mean, have something like monkey_patch(ConfigSpace: cs)
that goes over all hyperparameters and patches them according to the metadata? I like that! I'll push a PR later once we merge this one in
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.
Something like that. Basically, we shouldn't have to remember to call monkey patch - it should be handled for us internally. At least that's my high level design thought.
monkey_patch_quantization
and its unit tests frommlos_bench
tomlos_core
monkey_patch_quantization
in LlamaTune unit tests