Refactor benchmark CLI #273

popematt · 2023-07-12T01:03:29Z

Issue #, if available:

Description of changes:

This is still a WIP, but the CLI is usable with BenchmarkSpecs.

Still to-do are some more documentation, updating the benchmark cli tests, and some general double-checking of things, but you can start taking a look at it or even try it out yourself right now!

Example:

❯ python3 ./amazon/ionbenchmark/ion_benchmark_cli.py run tests/benchmark_sample_data/sample_spec/spec.ion
╒═══════════════════════════════════════════════════════╤════════════════╤════════════════╤═════════════════╤════════════════════════╕
│ name                                                  │   file_size(B) │   time_min(ns) │   time_mean(ns) │   memory_usage_peak(B) │
╞═══════════════════════════════════════════════════════╪════════════════╪════════════════╪═════════════════╪════════════════════════╡
│ (cbor,loads,cat.cbor)                                 │            131 │            458 │             505 │                   1200 │
├───────────────────────────────────────────────────────┼────────────────┼────────────────┼─────────────────┼────────────────────────┤
│ (cbor2,loads,cat.cbor)                                │            131 │           1416 │            1504 │                   1467 │
├───────────────────────────────────────────────────────┼────────────────┼────────────────┼─────────────────┼────────────────────────┤
│ (json,loads,cat.json)                                 │            178 │           1750 │            1810 │                   3157 │
├───────────────────────────────────────────────────────┼────────────────┼────────────────┼─────────────────┼────────────────────────┤
│ (ujson,loads,cat.json)                                │            178 │            541 │             571 │                    736 │
├───────────────────────────────────────────────────────┼────────────────┼────────────────┼─────────────────┼────────────────────────┤
│ (ion_text,loads,cat.ion)                              │            161 │          16333 │           16773 │                  41225 │
├───────────────────────────────────────────────────────┼────────────────┼────────────────┼─────────────────┼────────────────────────┤
│ (ion_binary,loads,cat.ion)                            │            137 │          16667 │           17191 │                  36853 │
├───────────────────────────────────────────────────────┼────────────────┼────────────────┼─────────────────┼────────────────────────┤
│ Ion Binary, pure Python impl                          │            137 │         150334 │          156203 │                  48926 │
├───────────────────────────────────────────────────────┼────────────────┼────────────────┼─────────────────┼────────────────────────┤
│ Ion Text, pure Python impl                            │            161 │         320625 │          332027 │                  66276 │
├───────────────────────────────────────────────────────┼────────────────┼────────────────┼─────────────────┼────────────────────────┤
│ (self_describing_protobuf,loads,cat.sd_protobuf_data) │            374 │           2500 │            2905 │                  13568 │
├───────────────────────────────────────────────────────┼────────────────┼────────────────┼─────────────────┼────────────────────────┤
│ (protobuf,loads,cat.protobuf_data)                    │             81 │            291 │             361 │                    888 │
╘═══════════════════════════════════════════════════════╧════════════════╧════════════════╧═════════════════╧════════════════════════╛

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

rmarrowstone

On the whole this looks like a good factoring and decreases a lot of repetitive code.

amazon/ionbenchmark/report.py

rmarrowstone · 2023-07-12T18:33:32Z

amazon/ionbenchmark/benchmark_runner.py

+
+from amazon.ionbenchmark.benchmark_spec import BenchmarkSpec
+
+_pypy = platform.python_implementation() == 'PyPy'


I will say again: it's not clear to me that anyone is actually using this with PyPy. For the benchmarking I would comfortable saying "we don't benchmark or profile on pypy"

popematt · 2023-07-25T00:03:08Z

@rmarrowstone What are your thoughts on testing the benchmark CLI? Can you recommend a good overall testing strategy, or shall I just try to write unit tests for the individual components and rely on the regression workflow to catch larger problems?

rmarrowstone · 2023-07-25T15:53:04Z

What are your thoughts on testing the benchmark CLI? Can you recommend a good overall testing strategy, or shall I just try to write unit tests for the individual components and rely on the regression workflow to catch larger problems?

Latter sounds reasonable. For CLIs I tend to just run them and not worry too much about automated testing of the actual executable: the ROI of such isn't great imo.

…of python

This reverts commit 90aead5.

This reverts commit 5988aca.

This reverts commit 4e961bc.

popematt · 2023-07-26T23:37:00Z

Alright, I've got all tests passing now, including on PyPy. The performance workflow will fail, but I'd like to address that in a followup PR to prevent the scope of this one from growing too large.

rmarrowstone

Some minor-ish comments. I can't say I've read every line thoroughly. I can say that this is such an improvement over what's there that I'm inclined to merge and iterate/fix-up as needed.

Great work!!!

amazon/ionbenchmark/Format.py

amazon/ionbenchmark/benchmark_spec.py

rmarrowstone · 2023-07-27T17:03:59Z

amazon/ionbenchmark/benchmark_spec.py

+        if 'name' not in self:
+            self['name'] = f'({self.get_format()},{self.get_operation_name()},{path.basename(self.get_input_file())})'
+
+    def __missing__(self, key):


fwiw i think you could inherit from defaultdict to do this. not sure it's any better.

rmarrowstone · 2023-07-27T17:05:07Z

amazon/ionbenchmark/benchmark_spec.py

+        """
+        return self["name"]
+
+    def get_format(self):


consider using @property annotation on methods without get_ e.g:

@property def format(self): ...

So... I just tried this out, and found that I was getting inconsistent behavior between PyPy and CPython, so I'd like to leave it as is.

sorry to hear that. agree not to sweat it for now.

rmarrowstone · 2023-07-27T17:07:35Z

amazon/ionbenchmark/benchmark_spec.py

+            self._loader_dumper = self._get_loader_dumper()
+        return self._loader_dumper
+
+    def _get_loader_dumper(self):


Nice way to abstract the specific format. 👍

amazon/ionbenchmark/ion_benchmark_cli.py

amazon/ionbenchmark/ion_load_dump.py

amazon/ionbenchmark/ion_benchmark_cli.py

Co-authored-by: Rob Marrowstone <[email protected]>

Refactor benchmark CLI

fd2f289

rmarrowstone reviewed Jul 12, 2023

View reviewed changes

amazon/ionbenchmark/report.py Outdated Show resolved Hide resolved

rmarrowstone reviewed Jul 12, 2023

View reviewed changes

popematt added 2 commits July 24, 2023 15:36

Adds changes based on PR feedback and other cleanup

3a4c30d

Adds more documentation to BenchmarkSpec

4b7838e

Updated testing for refactored benchmark cli

ba1f06b

popematt marked this pull request as ready for review July 25, 2023 23:06

popematt and others added 17 commits July 25, 2023 16:26

Merge branch 'master' into cli-refactor

6991049

Replace match with if/elif/else

fb28068

Fix whitespace

380da01

Fix _another_ match/case that I missed before.

7e08959

Remove use of | operator for dicts to support older python versions

d95fc3c

Fix _another_ match/case that I missed before.

0fe046a

Remove type subscripts to support older python versions

4a02a8b

Change matrix strategy to fail-fast: false

50ee514

Remove type unions from function signature to support older versions …

fe119fb

…of python

Remove use of removeprefix() function to support older python versions

2fa41b8

Try to fix c-extension conditional to make tests run on PyPy

4e961bc

Try another hack to get the tests to run on PyPy

5988aca

Skip some tests on PyPy

90aead5

Revert "Skip some tests on PyPy"

5feed27

This reverts commit 90aead5.

Revert "Try another hack to get the tests to run on PyPy"

04851ff

This reverts commit 5988aca.

Revert "Try to fix c-extension conditional to make tests run on PyPy"

20c3991

This reverts commit 4e961bc.

Fix faulty c_ext logic in ion_load_dump

c464c97

popematt requested a review from rmarrowstone July 26, 2023 23:36

rmarrowstone approved these changes Jul 27, 2023

View reviewed changes

Update amazon/ionbenchmark/Format.py

4ad60bf

Co-authored-by: Rob Marrowstone <[email protected]>

popematt added 2 commits July 27, 2023 20:52

PR feedback changes

26a317a

Fix test that was failing with CPython

6115add

popematt merged commit f3f1b3c into amazon-ion:master Jul 28, 2023
9 of 13 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor benchmark CLI #273

Refactor benchmark CLI #273

popematt commented Jul 12, 2023 •

edited

Loading

rmarrowstone left a comment

rmarrowstone Jul 12, 2023

popematt commented Jul 25, 2023

rmarrowstone commented Jul 25, 2023

popematt commented Jul 26, 2023

rmarrowstone left a comment

rmarrowstone Jul 27, 2023

rmarrowstone Jul 27, 2023

popematt Jul 28, 2023

rmarrowstone Jul 28, 2023

rmarrowstone Jul 27, 2023


		from amazon.ionbenchmark.benchmark_spec import BenchmarkSpec

		_pypy = platform.python_implementation() == 'PyPy'

Refactor benchmark CLI #273

Refactor benchmark CLI #273

Conversation

popematt commented Jul 12, 2023 • edited Loading

rmarrowstone left a comment

Choose a reason for hiding this comment

rmarrowstone Jul 12, 2023

Choose a reason for hiding this comment

popematt commented Jul 25, 2023

rmarrowstone commented Jul 25, 2023

popematt commented Jul 26, 2023

rmarrowstone left a comment

Choose a reason for hiding this comment

rmarrowstone Jul 27, 2023

Choose a reason for hiding this comment

rmarrowstone Jul 27, 2023

Choose a reason for hiding this comment

popematt Jul 28, 2023

Choose a reason for hiding this comment

rmarrowstone Jul 28, 2023

Choose a reason for hiding this comment

rmarrowstone Jul 27, 2023

Choose a reason for hiding this comment

popematt commented Jul 12, 2023 •

edited

Loading