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

Optimise Python unit test runtime (~7x speedup) #3032

Merged
merged 3 commits into from
Mar 13, 2024

Conversation

alexander-beedie
Copy link
Contributor

@alexander-beedie alexander-beedie commented Mar 12, 2024

Update

  • Each of the 88 tests was recreating a fresh Database from scratch, reloading/parsing all data, etc.
  • Optimised this by creating as a read-only DB fixture once, and caching it:
    • conn_db_readonly.
  • Only the 3 tests that require a writable DB get a different database; there are two writable fixtures, one of which is also cached (and this is the one that gets used, as the current set of tests requiring write-access don't step on each other):
    • conn_db_writable_cached
    • conn_db_writable_new.

Speedup

(These timings were generated locally on Apple Silicon M2)

Before: ~37 secs
 After: ~5½ secs 🚀 

Note: not worth adding xdist parallelism, as the initial setup cost leads to a higher total runtime (8-9 secs).

@alexander-beedie alexander-beedie changed the title Optimise Python unit test runtime (~6x speedup) Optimise Python unit test runtime (~7x speedup) Mar 12, 2024
@alexander-beedie alexander-beedie force-pushed the optimise-python-unit-tests branch 2 times, most recently from dc3b19b to 21de857 Compare March 12, 2024 20:13
@mewim
Copy link
Member

mewim commented Mar 13, 2024

@alexander-beedie Thanks for your contribution. I think it makes a lot of sense to cache database for testing instead of create a new one per test. For write tests though, I think there might be some risks in caching the database. It is not easy to tell whether a write test would conflict with other ones or not and may cause confusing errors when modifications on a test causes other tests to fail. So I changed the code a bit to only keep conn_db_readonly and conn_db_readwrite (which always create a new database for writing). I also added a minor workaround and notes as read only mode is not currently supported properly on Windows. I think this is now ready to be merged.

@mewim mewim merged commit 7da8a62 into kuzudb:master Mar 13, 2024
8 of 14 checks passed
@alexander-beedie
Copy link
Contributor Author

alexander-beedie commented Mar 13, 2024

@alexander-beedie I changed the code a bit to only keep conn_db_readonly and conn_db_readwrite (which always create a new database for writing).

Good call; as there are only three tests that need it the performance difference is marginal, and it's better to always do the right thing rather than chase a few % more speed in the tests ;)

@alexander-beedie alexander-beedie deleted the optimise-python-unit-tests branch March 13, 2024 09:09
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