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

Introduce capsule API #308

Merged
merged 16 commits into from
Nov 21, 2022
Merged

Introduce capsule API #308

merged 16 commits into from
Nov 21, 2022

Conversation

fangerer
Copy link
Contributor

@fangerer fangerer commented Mar 19, 2022

This introduces an HPyCapsule API similar to PyCapsule.
While the capsule API isn't use a lot in the top4000 packages, it's needed for our NumPy migration efforts.

Main goals to achieve were:

  • HPyCapsule should be interchangeable with PyCapsule. So, one can create an HPyCapsule, store into some attribute and a completely unrelated C API extension could use it via the PyCapsule API (and vice versa).
  • Map HPyCapsule_* functions to PyCapsule_* functions in CPython ABI mode.
  • Use just one context function to get and one to set all fields (to keep number of context functions low).

The implementation for CPython is mostly straight forward except of the destructor. This was the main problem is should probably be the main discussion point here.
The capsule destructor function has signature void (*PyCapsule_Destructor)(PyObject *) and is also exposed to the user with PyCapsule_Get/SetDestrutor.
I've introduced signature void (*HPyCapsule_Destructor)(const char *name, void *pointer, void *context) that will receive the contents of the capsule but not the object itself (and no HPy context, ofc).
However, we now cannot directly map the HPy destructor to a CPython one, we need a trampoline. This one needs to know the function pointer of the HPyCapsule_Destructor but PyCapsule does not give you the possibility to store this pointer somewhere. I've considered:

  • subclassing and using a different C struct for the capsule object but the type is not base type.
  • over-allocating the name field and storing the pointer before the name but the name is optional.

The only reliable solution I could think of was to have a global registry somewhere. I decided to put it into the interpreter state's dict (Python >= 3.8) or into the sys module (using PySys_Get/SetObject for Python < 3.8).

@mattip
Copy link
Contributor

mattip commented Oct 25, 2022

It seems this is required for the graal-team/hpy branch of numpy. Were there any more thoughts about how to do global state for the desctuctor?

@mattip
Copy link
Contributor

mattip commented Oct 25, 2022

Not connected to this PR, but I also see h_ComplexType in the numpy branch. Is there a PR or issue to add that to the HPyContext?

@mattip
Copy link
Contributor

mattip commented Oct 25, 2022

... and HPyTupleBuilder_IsNull as well...

@fangerer
Copy link
Contributor Author

It seems this is required for the graal-team/hpy branch of numpy.

Yes.

Were there any more thoughts about how to do global state for the desctuctor?

Not sure if I understand your question.
In this PR, the problem was that I wanted HPyCapsule to be compatible with PyCapsule. The main problem is PyCapsule's destructor that receives the full capsule object (i.e. as PyObject *). We clearly cannot do that in HPy. So, we install a trampoline as PyCapsule destructor function that calls the HPy destructor function. Now, the tricky part is to store the pointer of the HPy destructor function somewhere for the trampoline.
The solution in this PR is to do that in the interpreter state's dict.

Anyway, I didn't think about other solutions since then. Ideas are welcome.

@mattip mattip added this to the Version 0.9 milestone Nov 3, 2022
@mattip
Copy link
Contributor

mattip commented Nov 3, 2022

Adding this to the 0.9 milestone since it is needed for the NumPy port.

@mattip
Copy link
Contributor

mattip commented Nov 10, 2022

Needs a rebase

@fangerer
Copy link
Contributor Author

sure, I'll do that

@mattip
Copy link
Contributor

mattip commented Nov 16, 2022

@fangerer waiting for a rebase to merge this

@fangerer
Copy link
Contributor Author

@fangerer waiting for a rebase to merge this

Yes, still on my list. I'm sorry for the delay. I got sick (I'm still on leave) but will return tomorrow.

@mattip
Copy link
Contributor

mattip commented Nov 19, 2022

C++ is not happy with the capsule code:

/home/runner/work/hpy/hpy/hpy/devel/src/runtime/ctx_capsule.c:105:1: error: jump to label ‘error’
[214](https://github.com/hpyproject/hpy/actions/runs/3497785299/jobs/5857316290#step:7:215)
  105 | error:
[215](https://github.com/hpyproject/hpy/actions/runs/3497785299/jobs/5857316290#step:7:216)
      | ^~~~~

@mattip
Copy link
Contributor

mattip commented Nov 21, 2022

LGTM. I will merge once CI passes, if no-one objects.

@fangerer fangerer requested a review from mattip November 21, 2022 08:16
@mattip mattip merged commit 414be8b into hpyproject:master Nov 21, 2022
@fangerer
Copy link
Contributor Author

🥳 thank you, Matti

@fangerer fangerer deleted the fa/capsule branch November 21, 2022 08:57
@antocuni
Copy link
Collaborator

(Sorry to chime in just few hours after the PR has been merged 🤦‍♂️ )

The only reliable solution I could think of was to have a global registry somewhere.

I might be missing something here, but I think that we could use the same technique that we used for HPyDef_METH:

typedef struct {
const char *name; // The name of the built-in function/method
HPyCFunction impl; // Function pointer to the implementation
cpy_PyCFunction cpy_trampoline; // Used by CPython to call impl
HPyFunc_Signature signature; // Indicates impl's expected the signature
const char *doc; // The __doc__ attribute, or NULL
} HPyMeth;

i.e., instead of passing a pointer to a function, we could pass a pointer to a struct which contains two function pointers (one to the user-provided code, and one to the autogenerated cpy trampoline). Something like this:

typedef void (*HPyFunc_capsule_dtor)(const char *name, void *pointer, void *context);

typedef struct {
    cpy_PyCapsule_Destructor cpy_trampoline;
    HPyFunc_capsule_dtor impl;
} HPyCapsule_Destructor;

Then the user will have to use a macro to generate the struct:

/* this automatically generate the trampoline and fills a statically allocated 
    struct called 'my_dtor' */
HPyCapsule_DESTRUCTOR(my_dtor, my_dtor_impl);
void my_dtor_impl(const char *name, void *pointer, void *context) {
    ...
}

void init() {
    ...
    HPyCapsule c = HPyCapsule_New(ctx, "foo.bar", myptr, &my_dtor);
}

@mattip
Copy link
Contributor

mattip commented Nov 21, 2022

It's not too late, this could be an add-on PR to enhance the current code. Would you want to make a PR?

@fangerer
Copy link
Contributor Author

No, it's not too late. That's a good suggestion. Sorry, the PR is so old that I didn't spent too much thoughts on it recently.

@antocuni
Copy link
Collaborator

ok, I've opened an issue for it to ensure that we don't forget

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