FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Use PyType_Spec for creating new types in Rust by alex · Pull Request #1132 · PyO3/pyo3 · GitHub

Repository navigation

Use PyType_Spec for creating new types in Rust - #1132

Merged
kngwyu merged 1 commit into
PyO3:abi3from
alex:abi3-class-creation
Sep 5, 2020
Merged

kngwyu merged 1 commit into
PyO3:abi3from
alex:abi3-class-creation

Conversation

alex commented Sep 1, 2020

Copy link
Copy Markdown
Member

This is a proof of concept -- implements just enough of it to prove the idea works! There's still a lot of missing elements of this (look at all the commented out code!)

Before I went too much further and implemented all the different function pointers, I wanted to check in and see if this approach looked ok.

alex mentioned this pull request Sep 1, 2020
alex force-pushed the abi3-class-creation branch from 8f82c49 to b291801 Compare September 1, 2020 00:39

kngwyu commented Sep 1, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

First I'm sorry that the approach I told does not work since we cannot merge this branch to abi3 now. Maybe I push some commits to your branch and then it can cause some conflicts.

I'm for the PyType_Spec approach, but looking https://docs.python.org/3/c-api/type.html#c.PyType_Slot, there are some protocols that PyType_Spec cannot support.
So I think two important designs are:

  • Explicitly separating functionalities that PyType_Spec cannot address. Concretely, I imagine something like:
#[cfg(Py_LIMITED_API)]
fn initialize_tp_obj_additional(type_object: &mut ffi::PyType_Object) {}
#[cfg(not(Py_LIMITED_API))]
fn initialize_tp_obj_additional(type_object: &mut ffi::PyType_Object) { 
    type_object.~ = ...;
}
  • Raise compile errors if those functionalities are used with abi3. It's not so easy but maybe we can do.

And another related thing is:

  • abi3 feature is not good that it is not additive but exclusive. I.e., we should rather make not(Py_LIMITED_API) as a feature flag and set it by default.
    But I'll address this problem myself after we move to abi3 branch.

kngwyu commented Sep 1, 2020

Copy link
Copy Markdown
Member

Ah I forgot an important point: currently we ourselves allocate type objects.
So it is not straightforward to use PyType_FromSpec...

alex commented Sep 1, 2020

Copy link
Copy Markdown
Member Author

Why is it a problem that pyo3 allocates it's own type objects now? This PR switches to PyType_FromSpec and it seems to work ok.

I will go through and finish porting all the other callbacks now.

alex force-pushed the abi3-class-creation branch from b291801 to 024f888 Compare September 1, 2020 03:42

alex commented Sep 1, 2020

Copy link
Copy Markdown
Member Author

Ok, it's now far enough along that test_arithmetics passes!

Comment thread src/pyclass.rs

kngwyu commented Sep 1, 2020

Copy link
Copy Markdown
Member

Why is it a problem that pyo3 allocates it's own type objects now? This PR switches to PyType_FromSpec and it seems to work ok.

Ah I see how you changed get_or_init and it looks OK, except we have to set an additional flag Py_TPFLAGS_HEAPTYPE.

alex commented Sep 1, 2020

Copy link
Copy Markdown
Member Author

It automatically sets that flag for us (very nice of it!) https://github.com/python/cpython/blob/master/Objects/typeobject.c#L2949

alex force-pushed the abi3-class-creation branch from 024f888 to 4f20747 Compare September 1, 2020 04:12

alex commented Sep 1, 2020

Copy link
Copy Markdown
Member Author

Ok, going to bed now. I start working on buffers, but it's not working, and I don't really know why. I suppose next step is gdb :-)

kngwyu commented Sep 1, 2020

Copy link
Copy Markdown
Member

@alex

alex commented Sep 1, 2020 via email

Copy link
Copy Markdown
Member Author

alex force-pushed the abi3-class-creation branch 3 times, most recently from 5ce76e9 to 51ab1d7 Compare September 1, 2020 05:33

alex commented Sep 1, 2020

Copy link
Copy Markdown
Member Author

Here's a new issue I could use some feedback on. https://github.com/PyO3/pyo3/blob/master/tests/test_class_attributes.rs#L54-L60 is currently failing on this branch, because the mutation is allowed.

Currently on master, it passes because an exception is raised here: https://github.com/python/cpython/blob/master/Objects/typeobject.c#L3399-L3409

However, on this branch, the type now has the Py_TPFLAGS_HEAPTYPE flag! I'm not sure what the right fix is. One option is just removing the test.

Copy link
Copy Markdown
Member

🤔 Class immutability is a nice property, but I'm not sure we can preserve the existing behavior without doing something like creating a metaclass.

That's probably overly complicated for now, so I'd be cautiously ok for this feature to go. (At least in the short term, maybe with an issue to track restoring it?) Might want to wait to see what @kngwyu says too.

alex commented Sep 1, 2020

Copy link
Copy Markdown
Member Author

Ok, then we're on to the next failing test: https://github.com/PyO3/pyo3/blob/master/tests/test_class_basics.rs#L122-L141

This fails because it expects that __module__ on a type whose tp_name doesn't have '.' in it to be "module". However, this is another behavior difference with heap types: https://github.com/python/cpython/blob/master/Objects/typeobject.c#L544-L573

Personally I think this is a bit more correct, builtins was not particularly accurate anyways.

alex force-pushed the abi3-class-creation branch from 51ab1d7 to b6eb8b8 Compare September 1, 2020 13:49

kngwyu commented Sep 1, 2020

Copy link
Copy Markdown
Member

However, on this branch, the type now has the Py_TPFLAGS_HEAPTYPE flag! I'm not sure what the right fix is.

I think the only possible fix is having two functions:

#[cfg(Py_LIMITED_API)]
pub(crate) fn initialize_type_object<T>(...) {}
#[cfg(not(Py_LIMITED_API))]
pub(crate) fn initialize_type_object<T>(...) {}

Ok, then we're on to the next failing test: https://github.com/PyO3/pyo3/blob/master/tests/test_class_basics.rs#L122-L141

This test is not so important and you can either opt-in it by Py_LIMITED_API or just removing it.

alex commented Sep 1, 2020

Copy link
Copy Markdown
Member Author

I'm trying very hard to as little duplicate code paths as well, so i"ll remove the test. Here's the next one:

https://github.com/PyO3/pyo3/blob/master/tests/test_class_basics.rs#L10-L19

Yet again, CPython has completely different behavior for heap types: https://github.com/python/cpython/blob/v3.8.2/Objects/typeobject.c#L4972-L4988 they appear to always get a tp_new no matter what, which makes them callable. Does this seem ok, or does it break something?

Copy link
Copy Markdown
Member

I think the only possible fix is having two functions:

#[cfg(Py_LIMITED_API)]
pub(crate) fn initialize_type_object<T>(...) {}
#[cfg(not(Py_LIMITED_API))]
pub(crate) fn initialize_type_object<T>(...) {}

I wondered about this but would vote against splitting the #[pyclass] implementation on such a big way. I think if we duplicate any code, we should be super careful to ensure that the runtime behaviour is identical. I worry lots of people will get caught out if a package has different behaviour silently because it was installed from an abi3 wheel instead of a platform-specific one.

Copy link
Copy Markdown
Member

Yet again, CPython has completely different behavior for heap types: https://github.com/python/cpython/blob/v3.8.2/Objects/typeobject.c#L4972-L4988 they appear to always get a tp_new no matter what, which makes them callable. Does this seem ok, or does it break something?

Default tp_new seems scary - it probably allocates the memory but doesn't initialize it? Can we add a default tp_new which raises TypeError?

alex commented Sep 1, 2020

Copy link
Copy Markdown
Member Author

yeah, that works -- is there an example of a raw callback raising an exception that I can use?

davidhewitt commented Sep 1, 2020 •
edited
Loading

Copy link
Copy Markdown
Member

The safest way might be to do something like

callback_body!(py { Err(exceptions::PyTypeError::py_err("No constructor defined")) })

and trust the optimizer to remove all the gubbins :)

alex commented Sep 1, 2020

Copy link
Copy Markdown
Member Author

that'll work.

Copy link
Copy Markdown
Member

Just as a quick thought, once we've got this ready, should we potentially avoid merging it until after the 0.12 release? (#1104).

It looks like it might touch a fair bit of core code, and given the test coverage on PyO3 is... improving (😄)... it might be wiser to avoid merging until we've released 0.12. Which hopefully will be very soon!

alex commented Sep 1, 2020

Copy link
Copy Markdown
Member Author

That sounds reasonable to me.

alex commented Sep 1, 2020

Copy link
Copy Markdown
Member Author

Good news/bad news! The good news is that PyType_FromSpec seems to result in things being more consistent between Python/Rust... the bad news is that obviously it accomplishes this by being backwards incompatible...

https://github.com/python/cpython/blob/master/Objects/typeobject.c#L2891-L2986 this enforcement is pretty core to PyType_FromSpec...

alex force-pushed the abi3-class-creation branch 4 times, most recently from 677cc63 to 9a42221 Compare September 1, 2020 22:39

alex commented Sep 1, 2020

Copy link
Copy Markdown
Member Author

https://bugs.python.org/issue38140 seems to suggest that there's no way to set tp_dictoffset from this API until 3.9 is out... I'm not sure how to proceed.

alex force-pushed the abi3-class-creation branch 2 times, most recently from 7d770b7 to 95975b5 Compare September 1, 2020 23:56

alex commented Sep 1, 2020

Copy link
Copy Markdown
Member Author

I found a workaround... just set tp_dictoffset on the PyTypeObject* after it's created... it seems to work

alex force-pushed the abi3-class-creation branch 2 times, most recently from a6fd81e to 7622744 Compare September 2, 2020 00:07

alex commented Sep 2, 2020

Copy link
Copy Markdown
Member Author

Ok, __text_signature__ is now behaving differently... I know nothing about this, so time to investigate I suppose!

alex force-pushed the abi3-class-creation branch from 7622744 to 8592e01 Compare September 2, 2020 04:45

alex commented Sep 2, 2020

Copy link
Copy Markdown
Member Author

At this point all of cargo test is passing (the example tests still aren't). It required more changes to the tests than I like, but unfortunately heap types just seem to be slightly different in a bunch of ways.

alex commented Sep 2, 2020

Copy link
Copy Markdown
Member Author

locally on CPython the examples tests pass, so it might be a pypy bug :-/

kngwyu commented Sep 2, 2020

Copy link
Copy Markdown
Member

I cannot understand why you removed some tests.
I was going to support abi3 with some limited features, but are you going to remove some features of PyO3, even with not(Py_LIMITED_API)?

I feel we're not agreed with that point.

alex commented Sep 2, 2020

Copy link
Copy Markdown
Member Author

I definitely agree it's not an appropriate solution, and I think we need a better one before this could be merged. Here's what happened for each of the tests I changed/removed:

The thing all of these have in common is that CPython treats a heap type different than a static type.

kngwyu commented Sep 2, 2020

Copy link
Copy Markdown
Member

Thank you for listing them.
So, if you're going to retrieve some tests at last, could you please just mark them #[ignore] and add some comments?
If I have some time this weekend I'm going to push some changes to abi3 branch for some backward compatibility, and just #[ignore] is easier to retrieve then.

alex force-pushed the abi3-class-creation branch from 8592e01 to f0bbff5 Compare September 2, 2020 20:49

alex commented Sep 2, 2020

Copy link
Copy Markdown
Member Author

I added #[ignore] with comments.

alex force-pushed the abi3-class-creation branch from f0bbff5 to 4004620 Compare September 2, 2020 20:54
alex marked this pull request as ready for review September 2, 2020 21:08
alex changed the title Proof of concept of using PEP384s PyType_Spec Use PyType_Spec for creating new types in Rust Sep 2, 2020

alex commented Sep 2, 2020

Copy link
Copy Markdown
Member Author

Tests are now green :-)

kngwyu commented Sep 5, 2020

Copy link
Copy Markdown
Member

Thank you, let me use some time to create a continuation PR

kngwyu merged commit 62a175e into PyO3:abi3 Sep 5, 2020
kngwyu mentioned this pull request Sep 5, 2020
5 of 6 tasks
alex deleted the abi3-class-creation branch April 2, 2026 12:48
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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.

3 participants


Back | FazBrowse Home | New Git URL