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

Complete abi3 support #1152

Merged
merged 65 commits into from
Oct 27, 2020
Merged

Complete abi3 support #1152

merged 65 commits into from
Oct 27, 2020

Conversation

kngwyu
Copy link
Member

@kngwyu kngwyu commented Sep 5, 2020

For #1125.
Continuation of #1132.
What I did so far are

  • Prepared the proper feature flag. I named it all-apis but feel free to propose alternative.
    • EDITED: I renamed it unstable-api adviced by konstin.
  • Restructured method tables with cfg.

TODO:

  • Add CI setting for abi3
  • Refactor tp_dict initialization codes (=classattr). It should be changed a lot.
  • __text__signature__.
  • Raise compile errors if some features (e.g., Buffer Protocol) is used without unstable-api feature.
  • Support additional feature flags with the minimum required Python version. See Specifying the minimum supported python version #1195 about the discussion.
  • Documentation
    • Document about abi3 in the README and the guide.

... and so on.

Sorry @alex for the conflict. I'm going to work on tp_dict problem for now and contributions for other areas are welcome.

@kngwyu
Copy link
Member Author

kngwyu commented Sep 5, 2020

Now the command for testing abi3 features is cargo test --no-default-features --features="macros".

@alex
Copy link
Contributor

alex commented Sep 5, 2020

The changes you have here look good to me, once CI is passing of course :-) I think there's still a handful of fixes needed, but we're close!

Cargo.toml Outdated
macros = ["ctor", "indoc", "inventory", "paste", "pyo3cls", "unindent"]
# Enable the use of `not(Py_LIMITED_API)` s.
# This is incompatible with abi3(PEP384). See https://www.python.org/dev/peps/pep-0384/ more.
all-apis = []
Copy link
Member

@konstin konstin Sep 5, 2020

Choose a reason for hiding this comment

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

I'd prefer the (c)python terminology of "stable abi"/"limited api"/"abi3" or an inverted term such as "unstable abi" or "unlimited api".

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, I choose unstable-api

@konstin
Copy link
Member

konstin commented Sep 5, 2020

This looks great, thanks to both of you for implementing this!

I'll update maturin to support building abi3 packages, and have two questions regarding coordination.

I'd like to automatically detect whether the project only needs a single abi3 wheel or the current one wheel per version, so it would be great to have a single feature to check for. Ideally this would not be checking for the absence for a feature, as checking for presence makes backwards compatibility easier. (That you only need single wheel is imho one of the biggest advantages of abi3)

As far as I've understood, abi3 grows with each python version, so imho it would be a good idea if we had some way of communicating the lowest supported python version. This ensures that users don't accidentally rely on features that are only in more recent versions. Imho it would be ideal if pyo3 could set the python version cfg flags based on that version, so maturin can directly build cp3<minor version>-abi3 wheels, no matter what python version there is. (As far as I can tell we don't even need to have an actual python for the build script since the sysconfig shouldn't matter with the stable abi, but that's a bit out of scope for this pull request)

@kngwyu
Copy link
Member Author

kngwyu commented Sep 6, 2020

once CI is passing of course

Looks I accidentally removed Py_GetSetDef_INIT, which caused SIGSEGV.
Now CI is red again since my fixes for weakref/dict offset are not working 😓 (EDITED: reverted the change)

abi3 grows with each python version, so imho it would be a good idea if we had some way of communicating the lowest supported python version

Thanks, do you know some good documents for that? Or do we need to check each version precisely?

@davidhewitt
Copy link
Member

davidhewitt commented Sep 6, 2020

@kngwyu do you want this to merge for the 0.12 release? I think that 0.12 is now amost ready, with all docs waiting in PRs. So I'd like to release 0.12 this week.

I'm a little wary of merging this just before 0.12 releases, because the move to heap types seems like it can change runtime behaviour in ways that we might not discover immediately. So I vote we release 0.12 asap, and then merge this so that we can have some time to gain confidence while we work towards 0.13.

@konstin
Copy link
Member

konstin commented Sep 6, 2020

I've tested this branch with one of my projects and it works, however I'm now getting DeprecationWarning: builtin type MyBuiltinType has no __module__ attribute from importlib for each of my builtin types (python 3.7).

@kngwyu
Copy link
Member Author

kngwyu commented Sep 6, 2020

@davidhewitt

do you want this to merge for the 0.12 release?

There are some remaining problems and I don't think we can address all these problems immediately.
So 👍 for including this in 0.13 release, not 0.12.

src/class/number.rs Outdated Show resolved Hide resolved
alex and others added 10 commits September 6, 2020 17:35
Fixed a few compilation errors on the abi3 branch
Properly mark a function as limited API only
Complete the process of disabling buffers with Py_LIMITED_API
I think this might technically be backwards incompatible if you had a custom metaclass with fancy behavior, but pyo3 doesn't seem to have any native support for those.
Fill tp_dict on types in an abi3-friendly way
The implementation is more complex, because there's no equivalent to tp_name in the limited API
Make PyType::name abi3 compatible
@alex
Copy link
Contributor

alex commented Oct 12, 2020

:flips table: another merge conflict, this time in CHANGELOG.

I'm happy to keep resolving these as they come up in PRs -- but let me know if that's not how y'all would prefer to handle this.(It's my hope that we're close to merge ready, and keeping it conflict free will help.)

@davidhewitt
Copy link
Member

😬 sorry - hopefully now that 0.12.3 is released and I'm going to let changes stack up for 0.13 the CHANGELOG conflicts might settle down.

Alternatively don't worry about fixing CHANGELOG conflicts for the moment? We can tidy that up just before merge.

@alex
Copy link
Contributor

alex commented Oct 12, 2020 via email

@kngwyu
Copy link
Member Author

kngwyu commented Oct 27, 2020

I think we can merge this after adding some notes about abi3 support of maturin in the guide.

@alex
Copy link
Contributor

alex commented Oct 27, 2020

I don't know anything about maturin, so I can't really help there. Are you able to do that?

@kngwyu
Copy link
Member Author

kngwyu commented Oct 27, 2020

I don't know anything about maturin, so I can't really help there. Are you able to do that?

It is already done by @konstin so what we need is just to note the minimum required version (and confirm it is correct).


By default, Python extension modules can only be used with the same Python version they were compiled against -- if you build an extension module with Python 3.5, you can't import it using Python 3.8. [PEP 384](https://www.python.org/dev/peps/pep-0384/) introduced the idea of the limited Python API, which would have a stable ABI enabling extension modules built with it to be used against multiple Python versions. This is also known as `abi3`.

Note that [maturin] >= 0.9.0 or [setuptools-rust] >= 0.12.0 is going to support `abi3` wheels.
Copy link
Member Author

Choose a reason for hiding this comment

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

@konstin
Is it OK to say this?

Copy link
Member

Choose a reason for hiding this comment

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

👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks!

@kngwyu
Copy link
Member Author

kngwyu commented Oct 27, 2020

@alex
Thank you for lots of works!

@kngwyu kngwyu merged commit 3b3ba4e into master Oct 27, 2020
@alex
Copy link
Contributor

alex commented Oct 27, 2020

Thank you for all your help getting this merged. This is very exciting!

@alex alex mentioned this pull request Oct 27, 2020
@messense messense deleted the abi3 branch March 18, 2021 02:23
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.

4 participants