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.

Sign up for GitHub

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

Jump to bottom

test(python): Reorganize benchmark test folder #6695

Merged
merged 7 commits into from
Feb 9, 2023
Merged

test(python): Reorganize benchmark test folder #6695

merged 7 commits into from
Feb 9, 2023

Conversation

stinodego
Copy link
Member

@stinodego stinodego commented Feb 5, 2023 •
edited
Loading

Related to #6364

Changes:

  • Added a threshold for the H2O AI benchmark - strings test.
    • I figured, if there is no threshold set, there is not much use in running the benchmark at all.
  • Made other benchmark tests into proper pytest tests and marked them as benchmark tests.
    • These are now ran as part of the benchmark workflow, and their run durations are reported in the logs.
  • Rename the folder db-benchmark to benchmark and add some documentation to the benchmark script.
  • Rename docs/run_doc_examples.py to docs/run_doctest.py
  • Added a README.md to the py-polars/tests folder, and linked to this in the CONTRIBUTING guide. Feels good to make a bunch of this knowledge explicit to future contributors ☺️

@github-actions github-actions bot added python Related to Python Polars test Related to the test suite labels Feb 5, 2023
stinodego
stinodego commented Feb 5, 2023
View reviewed changes
py-polars/tests/unit/test_series.py Outdated
Comment on lines 2058 to 2060
def test_mean_overflow() -> None:
arr = np.array([255] * (1 << 17), dtype="int16")
assert arr.mean() == 255.0
Copy link
Member Author

Choose a reason for hiding this comment

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

This doesn't test anything Polars-related, so I removed it.

Copy link
Member

Choose a reason for hiding this comment

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

It does! It was testing if it doesn't overflow the mean. A naive sum/count would overflow the i16

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, but it's just numpy code though? 🤔 We don't rely on this to calculate our mean, right?

Reverted this removal, but I still don't understand why this test is in there!

ritchie46
ritchie46 reviewed Feb 6, 2023
View reviewed changes
Copy link
Member

@ritchie46 ritchie46 left a comment

Choose a reason for hiding this comment

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

The slow tests were explicitly run in benchmark folder because the polars had a release build. So we can test really slow code.

py-polars/tests/db-benchmark/lazy_vs_eager.py Show resolved Hide resolved
py-polars/tests/db-benchmark/various.py Show resolved Hide resolved
@stinodego
Copy link
Member Author

Ah interesting, learned a lot from your comments 😄 I'm gonna revert a lot of this stuff and add a bit of documentation in that folder on what's going on there.

@stinodego stinodego marked this pull request as draft February 6, 2023 11:35
@ritchie46
Copy link
Member

Yeah, maybe most ideal would be having those loose files also concerted to pytest functions and running those "release-build" tests only in CI once the benchmark has finished. That same binary is then used for those tests.

@stinodego stinodego marked this pull request as ready for review February 8, 2023 21:05
@stinodego
Copy link
Member Author

Yeah, maybe most ideal would be having those loose files also concerted to pytest functions and running those "release-build" tests only in CI once the benchmark has finished. That same binary is then used for those tests.

Done, and documented in the new Test Suite README 😄

ritchie46
ritchie46 approved these changes Feb 9, 2023
View reviewed changes
Copy link
Member

@ritchie46 ritchie46 left a comment

Choose a reason for hiding this comment

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

Nice cleanup. Especially formalizing the release tests is a good one.

I hope we can add the full TPCH benchmarks in the future.

py-polars/tests/benchmark/test_release.py
@@ -0,0 +1,148 @@
"""
Copy link
Member

Choose a reason for hiding this comment

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

Nice, that's cleaner indeed. :)

@ritchie46 ritchie46 merged commit 0a1c1bc into pola-rs:master Feb 9, 2023
Vincenthays pushed a commit to Vincenthays/polars that referenced this pull request Feb 9, 2023
@stinodego
test(python): Reorganize benchmark test folder (pola-rs#6695)
2dee8fa
@alexander-beedie
Copy link
Collaborator

Really nice.

@stinodego stinodego deleted the test-organization branch February 22, 2023 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ritchie46 ritchie46 ritchie46 approved these changes

Assignees
No one assigned
Labels
python Related to Python Polars test Related to the test suite
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@stinodego @ritchie46 @alexander-beedie