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

Update pybind #66

Merged
merged 9 commits into from
Sep 21, 2023
Merged

Update pybind #66

merged 9 commits into from
Sep 21, 2023

Conversation

ErikPoppleton
Copy link
Collaborator

Updated the included pybind version from 2.2 to 2.11 so that oxpy and oat work with Python 3.11. Pybind11 2.11 has a minimum Python version of 3.8, but because of oat's requirement of 3.9 I changed all requirements to match that.

Copy link
Owner

@lorenzo-rovigatti lorenzo-rovigatti left a comment

Choose a reason for hiding this comment

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

I think this is fine as it is, but I'm thinking about including something to help a user to make it work with older python versions. Maybe we can add some info on how to use older pybind versions and add some cmake flags? I believe it's possible that some people don't have access to new python versions on older clusters/computers, and python 3.11 is really recent. What do you think?

@@ -46,7 +46,7 @@ def check(to_check:List[str]=["python", "numpy", "matplotlib", "sklearn", "oxpy"
print("INFO: Python version: {}".format('.'.join([str(i) for i in version_info[0:3]])), file=stderr)
if version_info < (3, 9):
flag = True
print("WARNING: Some scripts will not run with Python versions earler than 3.8. You have {}, please update your environment", file=stderr)
print("WARNING: Some scripts will not run with Python versions earler than 3.9. You have {}, please update your environment", file=stderr)

Choose a reason for hiding this comment

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

I think there is a version_info missing in the print call

Choose a reason for hiding this comment

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

And there is also a typo (earler should be earlier)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As far as version requirements go, the oxpy/oat code in this pull request will work with Python 3.9-3.11 (haven't checked the bleeding edge 3.12). Because of changes to the CPython api, the current version of Pybind11 (2.11) only works with Python 3.8+. Because of some data structures and type hints in oat, it needs 3.9+.

I'm not familiar with CMake/make enough to do this easily, but is there an easy way to decouple oxpy from oat (since right now they're just installed in a single pip command, I think)? If so, we could give instructions on how to install just oxpy with older versions of Python by downloading an older version of Pybind11 and replacing the oxpy/pybind11 folder.

Thank you for noticing the errors. I will fix.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I added a section to the Installation page of the documentation on how to replace the pybind11 folder and remove oat, which will allow oxpy to be installed with older Python versions.

@lorenzo-rovigatti lorenzo-rovigatti merged commit 2c17f37 into master Sep 21, 2023
3 checks passed
@lorenzo-rovigatti lorenzo-rovigatti deleted the update_pybind branch September 21, 2023 14:16
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