-
Notifications
You must be signed in to change notification settings - Fork 65
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
Leveldb PR changes and going further questions added #141
Conversation
@@ -641,4 +641,4 @@ | |||
}, | |||
"nbformat": 4, | |||
"nbformat_minor": 4 | |||
} |
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 you please git restore
from main
this file (it's an unnecessary/superficial change).
@@ -618,4 +618,4 @@ | |||
}, | |||
"nbformat": 4, | |||
"nbformat_minor": 4 | |||
} |
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 you please git restore
from main
this file (it's an unnecessary/superficial change).
external/leveldb/LevelDbTuning.ipynb
Outdated
"cell_type": "code", | ||
"metadata": {}, | ||
"execution_count": null, | ||
"outputs": [] | ||
}, | ||
{ | ||
"cell_type": "code", |
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 you please remove these empty cells at the end?
I think I added one of them in my "cleanup" PR, but they're adding an empty black box at the bottom of the rendered notebook.
external/leveldb/LevelDbTuning.ipynb
Outdated
"logger = create_logger('Optimizing LevelDB', logging_level=logging.WARN)\n", | ||
"def initialize_optimizer():\n", | ||
" parameter_search_space = create_parameter_search_space(parameter_name=\"write_buffer_size\", min_val=1*1024*1024, max_val=128*1024*1024)\n", | ||
" optimization_problem = create_optimization_problem(parameter_search_space, objective_name=\"throughput\", min_val=0, max_val=1000, minimize=False)\n", |
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.
Please add a comment that this max_val
is assumed to be a excessive upperbound for the hardware the notebook was originally tested on, and may need be adjusted for the target platform in question.
Co-authored-by: Brian Kroth <bpkroth@users.noreply.github.com>
@@ -714,20 +279,17 @@ | |||
" latency, throughput = float(stats[0].strip().split(\" \")[0]), float(stats[1].strip().split(\" \")[0])\n", | |||
" return latency, throughput\n", | |||
"\n", | |||
"#optimizer = bayesian_optimizer_factory.create_remote_optimizer(optimization_problem=optimization_problem)\n", |
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.
There's another line wrap issue in result =
line of the the run_workload
def above, but it's outside of this PR's diff so I can't suggest a change directly here.
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.
Fixed this one.
Co-authored-by: Brian Kroth <bpkroth@users.noreply.github.com>
Co-authored-by: Brian Kroth <bpkroth@users.noreply.github.com>
Co-authored-by: Brian Kroth <bpkroth@users.noreply.github.com>
Co-authored-by: Brian Kroth <bpkroth@users.noreply.github.com>
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. I applied just a few more line wrap polishing changes. Thanks!
No description provided.