Skip to content
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

Update benchmarks for Python 3.12 #148

Merged
merged 3 commits into from
Oct 16, 2023

Conversation

movermeyer
Copy link
Collaborator

What are you trying to accomplish?

Updating the benchmarking metrics in the tables.

What approach did you choose and why?

Ran the benchmarking scripts.

I had to exclude pendulum and maya (which uses pendulum) since pendulum won't build on Python 3.12 until v3.0.0 comes out (any day now???): sdispater/pendulum#696

tox no longer supports Python < 3.7, so it isn't useful for that anymore.
This means that the Python 2.7 data hasn't been updated. 😞

What should reviewers focus on?

There should be no changes to the Python 2.7 column. I had to manually insert this data, so that's the spot that's most likely to have broken, though I've double-checked.

The impact of these changes

Users can see our Python 3.12 performance numbers and feel that we are keeping up with modern Python.

Comment on lines +131 to +132
writer.headers = ["Module"] + (["Call"] if include_call else []) + formatted_python_versions + [f"Relative slowdown (versus {baseline_module}, latest Python)"]
writer.type_hints = [pytablewriter.String] * len(writer.headers)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

pytablewriter v1.0.0 dropped these, so I replaced them with the new names: thombashi/pytablewriter@7777033

@movermeyer movermeyer marked this pull request as ready for review October 16, 2023 19:00
Copy link
Collaborator

@AlecRosenbaum AlecRosenbaum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a huge deal, but on the second table the extra column makes the table wider than the page will go (and looks cut-off):

image

The relative speeds are probably most important to at a glance, so maybe those should be the first column before the python versions? (Absolutely not a strong opinion here, just figured I'd mention it)

@movermeyer
Copy link
Collaborator Author

movermeyer commented Oct 16, 2023

Not a huge deal, but on the second table the extra column makes the table wider than the page will go (and looks cut-off):

@AlecRosenbaum Very true. I've switched the order of the columns so that the relative speed-up is the second column.


Aside: for my future reference:

As for making the columns thinner, I thought about changing Incorrect Result to and having a tooltip to explain what the symbol meant. The only way that I found to get GitHub to render it was using the ..raw: html block in the cell:

.. table::

  +--------------------------------------------------------+
  |             Result                                     |
  +========================================================+
  |.. raw:: html                                           |
  |                                                        |
  |   <span title="Incorrect Result (``None``)">❌</span>  |
  +--------------------------------------------------------+

Which works, but but pytablewriter doesn't support multi-line values in table cells.. 🤷

@movermeyer movermeyer force-pushed the movermeyer/update_benchmarks_for_312 branch from d5ab26d to 90543e8 Compare October 16, 2023 22:20
@movermeyer
Copy link
Collaborator Author

Alright, instead of changing the column order (I was partial to what we had), I've done the simple change of changing the Incorrect Result to , without a tooltip. The columns fit once again:

image

@@ -23,7 +23,7 @@ def __str__(self):
if self.exception:
return f"Raised ``{self.exception}`` Exception"
elif not self.matched_expected:
return f"**Incorrect Result** (``{self.parsed_value}``)"
return "❌"
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reduced width so that the columns all fit without a scroll bar.

@movermeyer movermeyer merged commit 8fed62b into master Oct 16, 2023
17 checks passed
@movermeyer
Copy link
Collaborator Author

movermeyer commented Oct 16, 2023

But of course, the table width shown in PR review is not the same as shown on the project main page 🤦:

image

so the column re-ordering will be necessary (unless we drop some columns)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants