-
Notifications
You must be signed in to change notification settings - Fork 449
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
Use the limited C api #1827
base: main
Are you sure you want to change the base?
Use the limited C api #1827
Conversation
ResultValue shouldn't be in a `shared_ptr`, and `UniqueRef::release()` shouldn't have a period of time where it points at something that might have a refcount of 0.
These are extremely tricky to do in C++, so keeping the exact same log lines is probably not worth the effort. The other issue is that the limited API doesn't specify how to use module state from a function. There are some methods in 3.10+ (like `PyType_FromModuleAndSpec()`) which might help, but then we have the potential issue of subclasses being from a different module.
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
I have read the CLA Document and I hereby sign the CLA |
@@ -321,6 +321,9 @@ jobs: | |||
PYTEST_ADDOPTS: --color=yes --tb=short | |||
TOX_PARALLEL_NO_SPINNER: 1 | |||
shell: bash | |||
- name: Debug - find files |
There was a problem hiding this 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 not necessary anymore
], | ||
language="c++", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think long-term this should be a c file. Then all source files would either be C, or Python
if sys.platform == "win32": | ||
if not any("/std" not in s for s in ext.extra_compile_args): | ||
ext.extra_compile_args.append("/std:c++17") | ||
elif sys.platform == "linux" or sys.platform == "darwin": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
elif sys.platform == "linux" or sys.platform == "darwin": | |
elif sys.platform in ("linux", "darwin"): |
if not any("/std" not in s for s in ext.extra_compile_args): | ||
ext.extra_compile_args.append("/std:c++17") | ||
elif sys.platform == "linux" or sys.platform == "darwin": | ||
if "std=" not in os.environ.get("CXXFLAGS", ""): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are touching this code, we could rework this part so that CXXFLAGS
wouldn't be read out of environment variable multiple times.
# sys.platform for linux used to return with version suffix, (i.e. linux2, linux3) | ||
# After version 3.3, it will always be just 'linux' | ||
# https://docs.python.org/3/library/sys.html#sys.platform |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
# sys.platform for linux used to return with version suffix, (i.e. linux2, linux3) | |
# After version 3.3, it will always be just 'linux' | |
# https://docs.python.org/3/library/sys.html#sys.platform |
return python, abi, plat | ||
|
||
class MyBuildExt(build_ext): | ||
def build_extension(self, ext): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def build_extension(self, ext): | |
def build_extension(self, ext) -> None: |
extensions = get_extensions() | ||
|
||
class MyBuildWheel(bdist_wheel): | ||
def get_tag(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def get_tag(self): | |
def get_tag(self) -> Tuple[str, str, str]: |
This will probably need to be imported.
@@ -32,11 +32,11 @@ extras = | |||
sso: secure-local-storage | |||
install_command = python -m pip install -U {opts} {packages} | |||
external_wheels = |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: We should investigate whether we could retire this plugin
Please answer these questions before submitting your pull requests. Thanks!
What GitHub issue is this PR addressing? Make sure that there is an accompanying issue to your PR.
(TODO)
Fill out the following pre-review checklist:
Please describe how your code solves the related issue.
This migrates
ArrowIterator
from Cython to a pure C++ implementation, using the limited C API.