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

Performance degraded by 8x #253

Closed
jvacek opened this issue Oct 21, 2024 · 13 comments
Closed

Performance degraded by 8x #253

jvacek opened this issue Oct 21, 2024 · 13 comments

Comments

@jvacek
Copy link

jvacek commented Oct 21, 2024

Between 6.5.2 and 6.5.3 we noticed an ~800% increase in time taken to complete the same loop.

After profiling, it seems most of the time goes to pt_in_poly_python.

Let me know if there's any other info you need to help troubleshoot this.

@jvacek
Copy link
Author

jvacek commented Oct 21, 2024

Looks like with 6.5.2 we were using the clang version, not sure why we'd be switching to the python one but it's coming from here

437113f#diff-004ace7ca04755ff5d00d7eeb3045b910a59d9a6af99c7ecd7af0acd53297d65R143-R148

@jannikmi
Copy link
Owner

Thanks for bringing this up. Unfortunately I don't have the capacity to look into this. Can anyone help out?

The tests (CI/CD) should be expanded to test both the C implementation and without (similar to the Numba alternative).

As a temporary workaround you could try enabling the Numba implementation or downgrading for a Speedup.

@jvacek
Copy link
Author

jvacek commented Oct 21, 2024

Thanks for the prompt reply, we'll downgrade for the time being.

try enabling the Numba implementation

Is there documentation for this?

@jannikmi
Copy link
Owner

@jvacek could you please test if you can locally import and use the c lang helpers (see the respective python module and tests).

@jvacek
Copy link
Author

jvacek commented Oct 21, 2024

On 6.5.3,
image

@jannikmi
Copy link
Owner

For documentation, please check:

https://timezonefinder.readthedocs.io/en/latest/7_performance.html#numba

@jannikmi
Copy link
Owner

Could you please run the tests of this package (Pytest) locally? These includes running the clang version of the point in polygon implementation.

@jvacek
Copy link
Author

jvacek commented Oct 21, 2024

TimezoneFinder.using_clang_pip() returns True for 6.5.2, and False on 6.5.3.

@jannikmi
Copy link
Owner

Could you please navigate to the directory where timezonefinder is installed and run pytest.

What is the value of timezonefinder.utils_clang.clang_extension_loaded?

@jannikmi
Copy link
Owner

I noticed that the tests are failing because the C extension is not being loaded when I run the tests locally on MacOS: #255

@jvacek
Copy link
Author

jvacek commented Oct 22, 2024

Will run the pytest later, but FYI this also happened in a cronjob on our k8s cluster, which is running python 3.10.3 on I think a buster-slim image

@jannikmi
Copy link
Owner

Could you please try again with the latest version 6.5.4. Thank you

@jvacek
Copy link
Author

jvacek commented Oct 23, 2024

6.5.4 works well, and it's using clang again. thanks for the super fast fix! Cheers

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

No branches or pull requests

2 participants