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

Adjust cl imports #1248

Merged
merged 13 commits into from
May 25, 2023
Merged

Adjust cl imports #1248

merged 13 commits into from
May 25, 2023

Conversation

ThomasHepworth
Copy link
Contributor

As discussed, here's a slightly cleaner layout for our comparisons so that we obscure some of the imports/exports to the users.

Some background:
At present, when you use:

import duckdb.duckdb_comparison_level_library as cll

all of our base classes are imported, alongside the classes we wish end users to take advantage of.

This means it’s possible for people to use:

cll.ElseLevelBase

by accident, should they not read the docs or know what they’re doing.

This used to be more of an issue than it is now, where the _level functions were imported and used in duckdb_comparison_library.py and would cause issues if you weren’t paying attention.

@github-actions
Copy link
Contributor

github-actions bot commented May 22, 2023

Test: test_2_rounds_1k_duckdb

Percentage change: -13.4%

date time stats_mean stats_min commit_info_branch commit_info_id machine_info_cpu_brand_raw machine_info_cpu_hz_actual_friendly commit_hash
849 2022-07-12 18:40:05 1.89098 1.87463 splink3 c334bb9 Intel(R) Xeon(R) Platinum 8370C CPU @ 2.80GHz 2.7934 GHz c334bb9
1659 2023-05-25 17:13:40 1.64783 1.62271 (detached head) 84bf606 Intel(R) Xeon(R) CPU E5-2673 v4 @ 2.30GHz 2.2947 GHz 84bf606

Test: test_2_rounds_1k_sqlite

Percentage change: -6.0%

date time stats_mean stats_min commit_info_branch commit_info_id machine_info_cpu_brand_raw machine_info_cpu_hz_actual_friendly commit_hash
851 2022-07-12 18:40:05 4.32179 4.25898 splink3 c334bb9 Intel(R) Xeon(R) Platinum 8370C CPU @ 2.80GHz 2.7934 GHz c334bb9
1661 2023-05-25 17:13:40 4.07406 4.00453 (detached head) 84bf606 Intel(R) Xeon(R) CPU E5-2673 v4 @ 2.30GHz 2.2947 GHz 84bf606

Click here for vega lite time series charts

@ThomasHepworth
Copy link
Contributor Author

ThomasHepworth commented May 22, 2023

Another potential layout - if you'd prefer that _comparison_imports.py was cleaned up:

.
{linker}_linker.py
├── {linker}_helpers
│   └── comparison_level_library.py
│   └── comparison_template_library.py
│   └── comparison_library.py

and then adjust the import paths accordingly.

@RossKen
Copy link
Contributor

RossKen commented May 23, 2023

I think I prefer the current structure rather than your alternative ☝️

Copy link
Contributor

@RossKen RossKen left a comment

Choose a reason for hiding this comment

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

I really like this change, and all seems to work as expected.

Just one thing to change - can you update the comparison and comparison level dev guides to reflect the changes of the folder structure? There are only a couple of references, then I am happy to approve.

Copy link
Contributor

@RossKen RossKen left a comment

Choose a reason for hiding this comment

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

Sorry didn't mean to approve - there are tests now failing

@ThomasHepworth ThomasHepworth merged commit af84cce into master May 25, 2023
@ThomasHepworth ThomasHepworth deleted the adjust_cl_imports branch May 25, 2023 23:18
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