-
Notifications
You must be signed in to change notification settings - Fork 760
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
RFC: Use a capsule-based API with a stable ABI for global, cross-extension functionality. #3073
Conversation
@@ -220,7 +220,7 @@ where | |||
let py_err = match panic_result { | |||
Ok(Ok(value)) => return value, | |||
Ok(Err(py_err)) => py_err, | |||
Err(payload) => PanicException::from_panic_payload(payload), | |||
Err(payload) => PanicException::from_panic_payload(py, payload), |
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.
The types do not work out at all for now, but I wanted to discuss the concrete approach first before trying plough through the trampolines.
…nsion functionality. [skip ci]
871657e
to
6d854fe
Compare
(I had intended to include |
Currently, the only reason I would see for a 0.18.3 is the |
Any comments w.r.t. the proposed direction? Do want this solution for cross-extension functionality? Are there alternatives? Are we okay with a |
There are the following questions / alternatives in my mind:
|
Yes, I think this is the case. If we have a separately installable pyo3 module, it would mainly give it the responsibility to install the capsule API instead of lazily installing it by whichever PyO3-built extension is loaded first. The only alternative I see to provide a pure Python API which we use internally, but that would still have the same versioning issues and might have additional performance issues on top of that.
The idea is that the capsule API stays backwards compatible, i.e. whenever an older than current version is installed, this might mean degraded functionality, but not imply crashes. (The check could stay Of course this implies additional effort to implement this graceful degradation as the API evolves and it makes the path dependency worse (i.e. the available functionality depends on which extension is loaded first). Only if we need to add new but universally required functionality will there be a hard incompatibility. (And the check would change to e.g. I think working hard on a minimal and highly compatible capsule API is preferable to for example provide multiple namespaced API as this complicates an already complex design further but still could not rule incompatibilities like multiple I think the observation that other foundational projects like NumPy were able to evolve their capsule API in a compatible manner suggests that this is possible. There is the danger of ossification for which NumPy is probably an example as well, but to me this mainly implies that we must put a lot of consideration into adding anything to the cross-extension API, i.e. try to keep as much functionality as possible extension-private. So in summary, I think providing a separately installable PyO3-built If we do go for this, the next step would be to setup a Python package implemented using PyO3 which we can publish to PyPI in the repository here? Or do we want a separate GitHub project? (More self contained, but harder to evolve in parallel. But then again coupling should be loose.) |
One thing I noticed that if we provide a separate pyo3 package which installs the capsule API, we have to be very very careful to not do something that would call into that capsule API until it is installed when that pyo3 package is initialized. Or we would need to not use PyO3 to implement the pyo3 package? |
Making the pyo3 package pure Python and caching a reference to the constructor of a PanicException defined in that Python package and just calling that via the normal Python calling convention would have a certain simplicity going for it, wouldn't it? The performance should also not be such a problem as long as we talk about raising exceptions? |
Definitely would be simpler. Would that rule out replacing it with a capsule-based version later? Any older PyO3 versions which just do |
Can you provide an example of how things might go wrong? I haven't thought super hard about this, however it's not readily apparent to me what the kind of failure you're anticipating here looks like. |
Well let's say initialization of the
I think we could always upgrade to a capsule API later and hence think that starting with a pure Python |
Ah right I see, yes. There are options like aborting or falling back silently but they're all quite messy, so something it would be nice to learn about first. I'm in favour of starting with a pure-Python package and we can learn from there. So I think just a couple questions to consider:
|
... I think I quite like going for |
I am not really familiar with calendar version. From what you describe, is there a difference between Personally, I would probably prefer to version this separately from PyO3 (the library), e.g. we just start at |
The difference between If we go with this package being Regardless of which combination we go for, I've observed that one nice thing about documenting lower bounds is that we only need to care about backwards-compatibility of the PyPI package with older PyO3 versions. We don't need to gracefully degrade PyO3 to use older versions of the PyPI package, because we can always document "extensions built with PyO3 0.20 require ... Just trying to write that I realise how confusing it is to write
|
Not invested in naming the PyPI package I definitely think that being able to break
I think this less an effect of lower bounds, but rather of Python accepting only one version of a package per dependency tree. Because the downgrade is required here because different versions of PyO3 could be combined to built extensions loaded into a single Python interpreter and each would have to be able to cope with the capsule API installed by the others. With any Python package like |
So I think we're converging to the decision that the next step will be to push a
My gut feeling is that even if we reserve the capability to do so, we'll never want to break the package in practice, because then ecosystem of packages using PyO3 is forcibly split by the runtime version they need. I can imagine we'd be really popular for putting the whole ecosystem in dependency hell 🙃
Yes, this is essentially what I meant. As long as the bounds are set correctly for |
Yes, I think the ease of changing this together with the usage sites would be worth it. I'll close this issue then because this not how we will proceed. I still a pure-Python |
Agreed. (I'm up for us including this in 0.20, shall we track it in an issue?) |
I just came across https://docs.python.org/3/c-api/init.html#c.PyInterpreterState_GetDict, which looks like it might be useful for storing panic exception which is shared across extensions. I don't think it would offer importable symbols for downstream code though. |
Indeed, I still think a |
This does not work (meaning build) due to multiple issues but I wanted to propose something concrete here so we can discuss whether this roundabout way is something we would want to pursue.
As this it is proposed, this should mean that the first extension built using PyO3 that calls
ensure_global_api
creates the module containing itsPanicException
type object and the corresponding capsule API. All other PyO3-based extensions would then use this capsule to create panic exceptions based on the same internals as first one.Downstream code should then be able to
import pyo3.PanicException
to catch all panic exceptions raised by an PyO3-based extension. (At least if it is new enough to use the capsule API.)