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

Expose LTS_CPU in a public API #19109

Closed
alonme opened this issue Oct 5, 2024 · 6 comments · Fixed by #19193
Closed

Expose LTS_CPU in a public API #19109

alonme opened this issue Oct 5, 2024 · 6 comments · Fixed by #19193
Labels
enhancement New feature or an improvement of an existing feature

Comments

@alonme
Copy link
Contributor

alonme commented Oct 5, 2024

Description

I think we should enable users to check if the installed version of polars is LTS_CPU or not programmatically.

I suggest adding this information to pl.show_versions()

Would be happy to implement this if maintainers agree to the idea

@alonme alonme added the enhancement New feature or an improvement of an existing feature label Oct 5, 2024
@ritchie46
Copy link
Member

Yes, I think we can add that show_versions.

@alonme
Copy link
Contributor Author

alonme commented Oct 5, 2024

Ok so i believe i found a couple of issues while working on that

  1. The _POLARS_LTS_CPU isn't actually set in the build process!
    This means that the check_cpu_flags doesn't exit early - however because the _POLARS_FEATURE_FLAGS is changed to match the lts-cpu mode - everything works?

Reproduction:

> pip install polars-lts-cpu --force-reinstall --quiet
> cat .venv2/lib/python3.11/site-packages/polars/_cpu_check.py | grep "LTS_CPU = "
_POLARS_LTS_CPU = False

The problem is that the sed command replaces the "old" name of the variable _LTS_CPU and not _POLARS_LTS_CPU

sed $SED_INPLACE 's/^_LTS_CPU = False$/_LTS_CPU = True/g' $CPU_CHECK_MODULE

  1. Minor:
    the wheel seems to contain a redundant file called _cpu_check.py''
> ls .venv/lib/python3.11/site-packages/polars/_cpu_*
.venv2/lib/python3.11/site-packages/polars/_cpu_check.py   .venv2/lib/python3.11/site-packages/polars/_cpu_check.py''

@ritchie46 - I think i'll create a PR to fix issue 1, and then expose the variable in another PR - sounds alright?
regarding issue 2 - i'll open another issue?

@orlp
Copy link
Collaborator

orlp commented Oct 7, 2024

I think we can remove the LTS CPU check bypass. The comment justifying it is:

# Set to True during the build process if we are building a LTS CPU version.
# The risk of the CPU check failing is then higher than a CPU not being supported.
_POLARS_LTS_CPU = False

This was initially done as a conservatism when the CPU check was introduced. However, I messed up and the code actually replaces the wrong variable as you noticed, so we were never conservative in the first place, without any issues. But now that it's been in Polars for quite a while I think we can safely say it works on polars-lts-cpu as well, and has actually diagnosed for people with CPUs that are too old even for LTS: #18956.

So at this point I would suggest making a PR that does set _POLARS_LTS_CPU correctly but removes the bypass and always runs the CPU check also on lts-cpu.

@alonme
Copy link
Contributor Author

alonme commented Oct 7, 2024

@orlp sounds good - i'll do that

@alonme
Copy link
Contributor Author

alonme commented Oct 12, 2024

@ritchie46 any chance you can build macos wheels so i can verify that the github actions change works? (there was a miss there originally so i would like to make sure all works now)

@ritchie46
Copy link
Member

ally so i would like to make sure all works now)

You can test that on your fork.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or an improvement of an existing feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants