Port benchmarks from rivulet #459
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Signed-off-by: Jared Webb <[email protected]>
mcember
force-pushed
the
dev/rivulet/port-benchmarks
branch
from
fb801e9 to
fbd238d
Compare
| def timed_step(description: str) -> Generator[BenchmarkStep, None, None]: | ||
| """Convenience for computing elapsed time of a block of code as a metric. | ||
|
|
||
| :param description: description of the step | ||
| :return: a benchmark operation populated with the elapsed time | ||
| """ | ||
| metric = BenchmarkStep(description) | ||
| start_time = time.time() | ||
| yield metric | ||
| end_time = time.time() | ||
| metric.add(BenchmarkMetric("elapsed_time", 1000 * (end_time - start_time), "ms")) |
There was a problem hiding this comment.
Is there an opportunity to replace this with the existing invoke_with_perf_counter https://github.com/ray-project/deltacat/blob/2.0/deltacat/utils/performance.py#L8-L17?
There was a problem hiding this comment.
In general I would like to refactor the whole benchmark framework. I would like to be able to write the tests more like normal client code without wrapping things in BenchmarkEngine/Run/Step objects.
Having said that this CR is more or less a copy-pasta, I don't want to take on scope for refactoring. invoke_with_perf_counter can't be just used as-is because it runs the passed in func whereas benchmark metric is more like a dataclass which gets wrapped in benchmark step
| @contextmanager | ||
| def make_tmpdir(): | ||
| tmpdir = tempfile.mkdtemp() | ||
| try: | ||
| yield tmpdir | ||
| finally: | ||
| shutil.rmtree(tmpdir) | ||
| pass |
There was a problem hiding this comment.
Nit: This is also duplicated at: https://github.com/ray-project/deltacat/blob/2.0/deltacat/tests/storage/conftest.py#L9-L16. Can we make this a fixture, and share it between benchmarks and unit tests from one common location?
There was a problem hiding this comment.
My understanding talking to an LLM is that it is an anti pattern to import fixtures explicitly, and multiple conftest.py files can only be automatically imported hierarchically, not from sibling directories. So unless we moved /benchmarks under /tests, we can't share fixtures
As a compromise I moved this to test utils and imported that from the benchmarks and test fixture
deltacat/storage/rivulet/dataset.py
Outdated
| ALL = "all" | ||
| ALL = "all" |
There was a problem hiding this comment.
Nitpick: looks like this got duplicated.
deltacat/benchmarking/README.md
Outdated
| @@ -0,0 +1,7 @@ | |||
| All the dependences needed to test are in dev-requirements.txt | |||
There was a problem hiding this comment.
Typo: This is going to bug me otherwise.
| All the dependences needed to test are in dev-requirements.txt | |
| All the dependencies needed to test are in dev-requirements.txt |
Summary
Port benchmarks. Modify imports and make refactors for changes to schema and other interfaces when we ported internal repo to deltacat
NOTE: currently this diff is pulling in changes from linter, per #458. We should merge in 458 first then rebase this.