tools: perf: introduce report_figures.py compare mode

[osalyk]

Continuation #1298

• Add deepdiff to PYTHON_DEPS #1321

---

This change is 

[janekmi]

Reviewed 29 of 29 files at r2, all commit messages.
Reviewable status: 29 of 34 files reviewed, 2 unresolved discussions (waiting on @janekmi and @osalyk)

---

utils/docker/images/Dockerfile.archlinux-latest, line 51 at r2 (raw file):

	matplotlib \
	pandas \
	deepdiff"

Has to be added also to BENCHMARKING.md.

---

utils/docker/images/Dockerfile.centos-7, line 93 at r2 (raw file):

# install python modules for the default user
RUN pip3 install --user $PYTHON_DEPS	deepdiff
deepdiff

What it is for?

[osalyk]

Reviewable status: 0 of 34 files reviewed, 2 unresolved discussions (waiting on @janekmi)

---

utils/docker/images/Dockerfile.archlinux-latest, line 51 at r2 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Has to be added also to BENCHMARKING.md.

Maybe in a separate PR?

---

utils/docker/images/Dockerfile.centos-7, line 93 at r2 (raw file):

Previously, janekmi (Jan Michalski) wrote…

What it is for?

Mistake.
Thanks

[ldorau]

Reviewed 28 of 29 files at r4, all commit messages.
Reviewable status: 28 of 34 files reviewed, 4 unresolved discussions (waiting on @janekmi, @ldorau, and @osalyk)

---

utils/docker/images/Dockerfile.archlinux-latest, line 51 at r4 (raw file):

	matplotlib \
	pandas \
	deepdiff"

I think that all changes to Docker files should be made in a separate PR.

---

utils/docker/images/Dockerfile.archlinux-latest, line 51 at r4 (raw file):

	matplotlib \
	pandas \
	deepdiff"

Alphabetical order please

[osalyk]

Reviewable status: 28 of 34 files reviewed, 4 unresolved discussions (waiting on @janekmi and @ldorau)

---

utils/docker/images/Dockerfile.archlinux-latest, line 51 at r4 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

Alphabetical order please

Done.

---

utils/docker/images/Dockerfile.archlinux-latest, line 51 at r4 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

I think that all changes to Docker files should be made in a separate PR.

#1321

[janekmi]

Reviewed 28 of 29 files at r4, all commit messages.
Reviewable status: 28 of 34 files reviewed, 2 unresolved discussions (waiting on @janekmi and @ldorau)

[ldorau]

Reviewable status: 28 of 34 files reviewed, all discussions resolved (waiting on @janekmi and @ldorau)

---

utils/docker/images/Dockerfile.archlinux-latest, line 51 at r4 (raw file):

Previously, osalyk (Oksana Sałyk) wrote…

#1321

OK, so remove it from the current one, please

[ldorau]

Reviewable status: 28 of 34 files reviewed, 2 unresolved discussions (waiting on @janekmi, @ldorau, and @osalyk)

---

utils/docker/images/Dockerfile.archlinux-latest, line 51 at r4 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

OK, so remove it from the current one, please

Rebase, please

[ldorau]

Reviewable status: 28 of 34 files reviewed, 2 unresolved discussions (waiting on @janekmi, @ldorau, and @osalyk)

---

utils/docker/images/Dockerfile.archlinux-latest, line 51 at r4 (raw file):

Previously, osalyk (Oksana Sałyk) wrote…

Done.

Not in this PR.

[osalyk]

Reviewable status: 0 of 34 files reviewed, 2 unresolved discussions (waiting on @janekmi and @ldorau)

---

utils/docker/images/Dockerfile.archlinux-latest, line 51 at r4 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

Not in this PR.

Done.

---

utils/docker/images/Dockerfile.archlinux-latest, line 51 at r4 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

Rebase, please

Done.

[janekmi]

Reviewed 28 of 29 files at r6, all commit messages.
Reviewable status: 28 of 34 files reviewed, 2 unresolved discussions (waiting on @janekmi and @ldorau)

[janekmi]

Reviewed 1 of 29 files at r6.
Reviewable status: 29 of 34 files reviewed, 2 unresolved discussions (waiting on @janekmi and @ldorau)

[ldorau]

Reviewed 1 of 29 files at r6, all commit messages.
Reviewable status: 29 of 34 files reviewed, all discussions resolved (waiting on @janekmi and @ldorau)

[janekmi]

Reviewed 2 of 5 files at r1.
Reviewable status: 30 of 34 files reviewed, 12 unresolved discussions (waiting on @janekmi, @ldorau, and @osalyk)

---

tools/perf/report_figures.py, line 23 at r6 (raw file):

# manually.
SUBPARSERS.required = True
PARSER_R = SUBPARSERS.add_parser('report',

This change requires an update to BENCHMARKING.md

Ref: https://github.com/pmem/rpma/blob/master/tools/perf/BENCHMARKING.md#3-generate-figures

---

tools/perf/report_figures.py, line 36 at r6 (raw file):

PARSER_C.add_argument('--benches', type=json_from_file, required=True,
                      nargs='+',
                      help='bench.json files of a completed benchmark')

of completed benchmarks

---

tools/perf/lib/compare.py, line 29 at r6 (raw file):

    @staticmethod
    def _figure_id(figure):
        """generate an identifier of the figure"""

an -> the? 🤔

---

tools/perf/lib/compare.py, line 33 at r6 (raw file):

    def prepare_series(self):
        """generate all comparisons required"""

... (both JSON and PNG files)

---

tools/perf/lib/compare.py, line 48 at r6 (raw file):

    def cache(self):
        """store the comparison to a JSON file"""

... to bench.json file

---

tools/perf/lib/compare.py, line 49 at r6 (raw file):

    def cache(self):
        """store the comparison to a JSON file"""
        _, bench = list(self._benches.items())[0]

# XXX assuming the first benchmark covers all figures of the comparison which may not always be true

---

tools/perf/lib/compare.py, line 57 at r6 (raw file):

            'requirements': {
                id: r.cache()
                for id, r in bench.requirements.items()}

Please indent this line.

---

tools/perf/lib/compare.py, line 71 at r6 (raw file):

        self._figure = figure # the sample figure
        self._figures = {}
        # pick all figures matching the sample one

Please put this comment above the self._figures = {}. I think it will be easier to follow that way.

---

tools/perf/lib/compare.py, line 78 at r6 (raw file):
A docstring is missing. :-)

e.g.

Combine series from all figures involved into a single figure

---

tools/perf/lib/compare.py, line 94 at r6 (raw file):

                'y': self._figure.argy,
                'xscale': self._figure.xscale,
                'series': benchlines}    

Unnecessary spaces at the end of the line.

---

tools/perf/lib/compare.py, line 108 at r6 (raw file):

            output = {}
        # for oneseries in self._figure.series:
            # keycontent.append(self._prepare_benchlines(oneseries['label']))

Commented out. Please remove.

---

tools/perf/lib/compare.py, line 128 at r6 (raw file):

        Figure.draw_png(keycontent['x'], keycontent['y'], keycontent['series'],
                        keycontent['xscale'], output_path, None, None,
                        None)

This None should fit into the previous line.

[janekmi]

Reviewable status: 30 of 34 files reviewed, 13 unresolved discussions (waiting on @janekmi, @ldorau, and @osalyk)

---

tools/perf/lib/compare.py, line 125 at r8 (raw file):

        keycontent = data.get(self._figure.key)
        output_path = self.png_path()
        # XXX - add setters to yaxis_max for bw and lat

- seems redundant.

[osalyk]

Reviewable status: 28 of 35 files reviewed, 12 unresolved discussions (waiting on @janekmi and @ldorau)

---

tools/perf/report_figures.py, line 23 at r6 (raw file):

Previously, janekmi (Jan Michalski) wrote…

This change requires an update to BENCHMARKING.md

Ref: https://github.com/pmem/rpma/blob/master/tools/perf/BENCHMARKING.md#3-generate-figures

Done.

---

tools/perf/report_figures.py, line 36 at r6 (raw file):

Previously, janekmi (Jan Michalski) wrote…

of completed benchmarks

Done.

---

tools/perf/lib/compare.py, line 33 at r6 (raw file):

Previously, janekmi (Jan Michalski) wrote…

... (both JSON and PNG files)

Done.

---

tools/perf/lib/compare.py, line 48 at r6 (raw file):

Previously, janekmi (Jan Michalski) wrote…

... to bench.json file

Done.

---

tools/perf/lib/compare.py, line 49 at r6 (raw file):

Previously, janekmi (Jan Michalski) wrote…

# XXX assuming the first benchmark covers all figures of the comparison which may not always be true

Done.

---

tools/perf/lib/compare.py, line 57 at r6 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Please indent this line.

Done.

---

tools/perf/lib/compare.py, line 71 at r6 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Please put this comment above the self._figures = {}. I think it will be easier to follow that way.

Done.

---

tools/perf/lib/compare.py, line 78 at r6 (raw file):

Previously, janekmi (Jan Michalski) wrote…

A docstring is missing. :-)

e.g.

Combine series from all figures involved into a single figure

Done.

---

tools/perf/lib/compare.py, line 94 at r6 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Unnecessary spaces at the end of the line.

Done.

---

tools/perf/lib/compare.py, line 108 at r6 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Commented out. Please remove.

Done.

---

tools/perf/lib/compare.py, line 128 at r6 (raw file):

Previously, janekmi (Jan Michalski) wrote…

This None should fit into the previous line.

Done.

---

tools/perf/lib/compare.py, line 125 at r8 (raw file):

Previously, janekmi (Jan Michalski) wrote…

- seems redundant.

Done.

[janekmi]

Reviewed 1 of 2 files at r7, 2 of 4 files at r9.
Reviewable status: 31 of 35 files reviewed, all discussions resolved (waiting on @janekmi and @ldorau)

[ldorau]

Reviewed 1 of 5 files at r1, 1 of 4 files at r9, 1 of 1 files at r10, all commit messages.
Reviewable status: 34 of 35 files reviewed, 2 unresolved discussions (waiting on @janekmi, @ldorau, and @osalyk)

---

tools/perf/BENCHMARKING.md, line 151 at r10 (raw file):

report

Maybe create ?

---

tools/perf/BENCHMARKING.md, line 157 at r10 (raw file):

report

.

[janekmi]

Reviewed 1 of 4 files at r9.
Reviewable status: 34 of 35 files reviewed, 3 unresolved discussions (waiting on @janekmi, @ldorau, and @osalyk)

---

tools/perf/BENCHMARKING.md, line 151 at r10 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

report

Maybe create ?

I agree report is unfortunate. But I'm not sure having a choice between create and compare... I'm not sure.

---

tools/perf/BENCHMARKING.md, line 171 at r10 (raw file):

```sh
$ ./report_figures.py compare -h

What you have called step number 4 is not yet another step in this process. It is a separate thing to do. If you will I recommend calling it a separate section e.g.

## Comparing

and add it at the end of this file.

[janekmi]

Reviewed 1 of 5 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ldorau and @osalyk)

[janekmi]

Reviewed 1 of 5 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @ldorau and @osalyk)

---

tools/perf/tests/lib/report/test_init.py, line 46 at r10 (raw file):

class BenchMock:
    """a lib.Bench.Bench mock"""
    config = CONFIG_DUMMY

Why it is needed?

[janekmi]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @ldorau and @osalyk)

a discussion (no related file):
I think all the commits should be squashed into a single commit:

• most of them overwrites each other
• the change to pylint.rc "fixes" the build "broken" by all of the changes you do which obviously has to go in the same commit otherwise bisection won't be possible.

---

[janekmi]

Reviewed 1 of 1 files at r10, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @ldorau and @osalyk)

[osalyk]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @janekmi, @ldorau, and @osalyk)

a discussion (no related file):

Previously, janekmi (Jan Michalski) wrote…

I think all the commits should be squashed into a single commit:

• most of them overwrites each other
• the change to pylint.rc "fixes" the build "broken" by all of the changes you do which obviously has to go in the same commit otherwise bisection won't be possible.

Done.

---

---

tools/perf/BENCHMARKING.md, line 151 at r10 (raw file):

Previously, janekmi (Jan Michalski) wrote…

I agree report is unfortunate. But I'm not sure having a choice between create and compare... I'm not sure.

Done.

[osalyk]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @janekmi, @ldorau, and @osalyk)

---

tools/perf/BENCHMARKING.md, line 157 at r10 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

report

.

Done.

---

tools/perf/BENCHMARKING.md, line 171 at r10 (raw file):

Previously, janekmi (Jan Michalski) wrote…

What you have called step number 4 is not yet another step in this process. It is a separate thing to do. If you will I recommend calling it a separate section e.g.

## Comparing

and add it at the end of this file.

Done.

[osalyk]

Reviewable status: 32 of 35 files reviewed, 5 unresolved discussions (waiting on @janekmi and @ldorau)

---

tools/perf/tests/lib/report/test_init.py, line 46 at r10 (raw file):

Previously, janekmi (Jan Michalski) wrote…

Why it is needed?

def _load_parts(self, loader, env, bench):
    self.parts = []
    if not bench.config.get('compare', False):

[osalyk]

Reviewable status: 33 of 35 files reviewed, 5 unresolved discussions (waiting on @janekmi and @ldorau)

---

tools/perf/BENCHMARKING.md, line 151 at r10 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

I'm for generate

Done.

[janekmi]

Reviewed 1 of 2 files at r13, 2 of 2 files at r14, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ldorau and @osalyk)

---

tools/perf/BENCHMARKING.md, line 189 at r12 (raw file):

Previously, osalyk (Oksana Sałyk) wrote…

Done.

## Comparing

🤣

---

tools/perf/tests/lib/report/test_init.py, line 46 at r10 (raw file):

Previously, osalyk (Oksana Sałyk) wrote…

Done.

CONFIG_DUMMY is sooooo not needed.

[osalyk]

Reviewable status: 33 of 35 files reviewed, 3 unresolved discussions (waiting on @janekmi and @ldorau)

---

tools/perf/BENCHMARKING.md, line 189 at r12 (raw file):

Previously, janekmi (Jan Michalski) wrote…

## Comparing

🤣

Done.

---

tools/perf/tests/lib/report/test_init.py, line 46 at r10 (raw file):

Previously, janekmi (Jan Michalski) wrote…

CONFIG_DUMMY is sooooo not needed.

Done.

[ldorau]

Reviewed 1 of 5 files at r1, 10 of 29 files at r6, 1 of 2 files at r15, all commit messages.
Reviewable status: 34 of 35 files reviewed, 3 unresolved discussions (waiting on @janekmi, @ldorau, and @osalyk)

a discussion (no related file):
Please rebase

---

[osalyk]

Reviewable status: 33 of 35 files reviewed, 3 unresolved discussions (waiting on @janekmi and @ldorau)

a discussion (no related file):

Previously, ldorau (Lukasz Dorau) wrote…

Please rebase

Done.

---

[ldorau]

Reviewed 1 of 2 files at r15, 1 of 1 files at r16, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @janekmi)

[janekmi]

Reviewed 2 of 2 files at r15, 1 of 1 files at r16, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @osalyk)

[ldorau]

Reviewed 1 of 4 files at r9, 1 of 2 files at r14.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @osalyk)

---

tools/perf/report_figures.py, line 63 at r16 (raw file):

generate a comparison figures

generate comparative figures

---

tools/perf/lib/compare.py, line 63 at r16 (raw file):

"utf-8"

'utf-8'

---

tools/perf/lib/compare.py, line 67 at r16 (raw file):

a comparison among

a comparison between

---

tools/perf/lib/compare.py, line 112 at r16 (raw file):

"utf-8"

.

---

tools/perf/lib/figure.py, line 34 at r16 (raw file):

'pad': 2

Are you sure the padding equal 2 will be good?
We have decreased it from 6 to 1 - see: 2bdacc1

---

tools/perf/lib/figure.py, line 300 at r16 (raw file):

                line_style = group_to_line_styles[group]
            else:
                if len(line_styles) > 0:

/rpma/tools/perf/lib/figure.py:300:19: C1801: Do not use `len(SEQUENCE)` to determine if a sequence is empty (len-as-condition)

See https://github.com/ldorau/rpma/runs/4239593859

[ldorau]

Reviewed 1 of 2 files at r7.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @osalyk)

[osalyk]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @ldorau)

---

tools/perf/report_figures.py, line 63 at r16 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

generate a comparison figures

generate comparative figures

Done.

---

tools/perf/lib/compare.py, line 63 at r16 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

"utf-8"

'utf-8'

Done.

---

tools/perf/lib/compare.py, line 67 at r16 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

a comparison among

a comparison between

Done.

---

tools/perf/lib/compare.py, line 112 at r16 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

"utf-8"

.

Done.

---

tools/perf/lib/figure.py, line 34 at r16 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

'pad': 2

Are you sure the padding equal 2 will be good?
We have decreased it from 6 to 1 - see: 2bdacc1

It looks good in my opinion.

---

tools/perf/lib/figure.py, line 300 at r16 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

/rpma/tools/perf/lib/figure.py:300:19: C1801: Do not use `len(SEQUENCE)` to determine if a sequence is empty (len-as-condition)

See https://github.com/ldorau/rpma/runs/4239593859

This is a bug in Pylint (pylint-dev/pylint#2684), fixed as of Pylint 2.4 (pylint-dev/pylint#2815)

[osalyk]

Reviewable status: 31 of 35 files reviewed, 6 unresolved discussions (waiting on @janekmi and @ldorau)

---

tools/perf/lib/figure.py, line 34 at r16 (raw file):

Previously, osalyk (Oksana Sałyk) wrote…

It looks good in my opinion.

Sorry, wrong image ;)

[janekmi]

Reviewed 4 of 4 files at r17, all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @ldorau)

[ldorau]

Reviewed 14 of 29 files at r6, 4 of 4 files at r17, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @osalyk)

---

tools/perf/lib/figure.py, line 34 at r16 (raw file):

Previously, osalyk (Oksana Sałyk) wrote…

Sorry, wrong image ;)

It has to be checked how it looks in the report, since the padding was decreased, because the distance between a text and a picture was too huge.

---

tools/perf/lib/figure.py, line 300 at r16 (raw file):

Previously, osalyk (Oksana Sałyk) wrote…

This is a bug in Pylint (pylint-dev/pylint#2684), fixed as of Pylint 2.4 (pylint-dev/pylint#2815)

So we have to use a workaround here, because those builds should not fail.

[ldorau]

Reviewed 1 of 29 files at r6.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @osalyk)

[ldorau]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @osalyk)

---

tools/perf/lib/figure.py, line 300 at r16 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

So we have to use a workaround here, because those builds should not fail.

So we have to use a workaround here, because those builds should not fail:
https://stackoverflow.com/questions/43121340/why-is-the-use-of-lensequence-in-condition-values-considered-incorrect-by-pyli

For sequences, (strings, lists, tuples), use the fact that empty sequences are false.

Yes: if not seq:
     if seq:

No:  if len(seq):
     if not len(seq):

[osalyk]

Reviewable status: 33 of 35 files reviewed, 2 unresolved discussions (waiting on @janekmi and @ldorau)

---

tools/perf/lib/figure.py, line 34 at r16 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

It has to be checked how it looks in the report, since the padding was decreased, because the distance between a text and a picture was too huge.

I've deleted this change.

---

tools/perf/lib/figure.py, line 300 at r16 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

So we have to use a workaround here, because those builds should not fail:
https://stackoverflow.com/questions/43121340/why-is-the-use-of-lensequence-in-condition-values-considered-incorrect-by-pyli

For sequences, (strings, lists, tuples), use the fact that empty sequences are false.

Yes: if not seq:
     if seq:

No:  if len(seq):
     if not len(seq):

I turned that check off.

[ldorau]

Reviewed 2 of 2 files at r18, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @osalyk)

---

tools/perf/lib/figure.py, line 300 at r16 (raw file):

Previously, osalyk (Oksana Sałyk) wrote…

I turned that check off.

And I have submitted the issue: #1329

I will run again all our nightly builds now...

[ldorau]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @osalyk)

---

tools/perf/lib/figure.py, line 291 at r18 (raw file):

group_to_line_styles.keys():

There is another error here:

/rpma/tools/perf/lib/figure.py:291:24: C0201: Consider iterating the dictionary directly instead of calling .keys() (consider-iterating-dictionary)

See: https://github.com/ldorau/rpma/runs/4250502093?check_suite_focus=true

[osalyk]

Reviewable status: 34 of 35 files reviewed, 2 unresolved discussions (waiting on @ldorau)

---

tools/perf/lib/figure.py, line 300 at r16 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

And I have submitted the issue: #1329

I will run again all our nightly builds now...

Done.

---

tools/perf/lib/figure.py, line 291 at r18 (raw file):

Previously, ldorau (Lukasz Dorau) wrote…

group_to_line_styles.keys():

There is another error here:

/rpma/tools/perf/lib/figure.py:291:24: C0201: Consider iterating the dictionary directly instead of calling .keys() (consider-iterating-dictionary)

See: https://github.com/ldorau/rpma/runs/4250502093?check_suite_focus=true

Done.

[ldorau]

Reviewed 1 of 1 files at r19, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @ldorau)

[ldorau]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @osalyk)

---

tools/perf/lib/figure.py, line 300 at r16 (raw file):

Previously, osalyk (Oksana Sałyk) wrote…

Done.

It is OK now

[ldorau]

Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @osalyk)