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

Update dict.get_item binding to use PyDict_GetItemRef #4355

Merged
merged 18 commits into from
Aug 1, 2024

Conversation

ngoldbaum
Copy link
Contributor

@ngoldbaum ngoldbaum commented Jul 16, 2024

Refs #4265

See the CPython C API docs for this new function. Also see PEP 703 for how this relates to free-threading support.

pyo3-ffi/src/dictobject.rs Outdated Show resolved Hide resolved
src/types/dict.rs Outdated Show resolved Hide resolved
@ngoldbaum
Copy link
Contributor Author

In the interest of testing this better, I wrote a module that makes sure the error path works correctly: https://github.com/ngoldbaum/pyo3-dict-test.

I'd like to integrate this with the rest of the pyo3 tests but have a question. Are there other tests that have little sidecar python files like I used? Or should I just figure out how to write that tiny toy HashErrors class in rust using pyo3?

@davidhewitt
Copy link
Member

Thanks for this! I will try to review tonight although I am struggling with OSS availablity right now due to young kids not sleeping

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Thanks very much for this! Overall looks correct though I have a few suggestions I'd like to see.

In the interest of testing this better, I wrote a module that makes sure the error path works correctly: https://github.com/ngoldbaum/pyo3-dict-test.

I'd like to integrate this with the rest of the pyo3 tests but have a question. Are there other tests that have little sidecar python files like I used? Or should I just figure out how to write that tiny toy HashErrors class in rust using pyo3?

Both options are possible. You can have inline Python inside the Rust source files using e.g. py_run! macro, though that tends to not be fun for longer snippets.

You could write a unit test inside types/dict.rs which uses the #[pyclass] macro to build a type from Rust. See e.g. src/impl_/pyclass.rs and the tests gated on #[cfg(feature = "macros")].

There is also the pytests/ subdirectory which builds a complete Rust extension and then runs some tests under pytest. Though I think in this case a unit test in types/dict.rs seems more fitting personally.

src/types/dict.rs Outdated Show resolved Hide resolved
pyo3-ffi/src/dictobject.rs Outdated Show resolved Hide resolved
src/types/dict.rs Outdated Show resolved Hide resolved
pyo3-ffi/src/dictobject.rs Outdated Show resolved Hide resolved
pyo3-ffi/src/dictobject.rs Outdated Show resolved Hide resolved
Copy link
Contributor Author

@ngoldbaum ngoldbaum left a comment

Choose a reason for hiding this comment

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

Thanks all for the reviews! I think the latest push addresses everything.

pyo3-ffi/src/dictobject.rs Outdated Show resolved Hide resolved
src/types/dict.rs Outdated Show resolved Hide resolved
pyo3-ffi/src/dictobject.rs Outdated Show resolved Hide resolved
Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Thanks, this looks great to me. Just some tiny finishing nits I'd like to have tweaked please 😁

pyo3-ffi/src/compat.rs Outdated Show resolved Hide resolved
pyo3-ffi/src/lib.rs Outdated Show resolved Hide resolved
src/types/dict.rs Outdated Show resolved Hide resolved
@mejrs
Copy link
Member

mejrs commented Jul 26, 2024

@davidhewitt I'm not sure if a compat module is the right way forward here.
Personally I would write

pub unsafe fn PyDict_GetItemRef(
    dp: *mut PyObject,
    key: *mut PyObject,
    result: *mut *mut PyObject,
) -> c_int {
    #[cfg(Py_3_13)]
    {
        extern "C" {
            #[link_name = "PyDict_GetItemRef"]
            pub fn ActualPyDict_GetItemRef(
                dp: *mut PyObject,
                key: *mut PyObject,
                result: *mut *mut PyObject,
            ) -> c_int;
        }
        
        ActualPyDict_GetItemRef(dp, key, result)
    }

    #[cfg(not(Py_3_13))]
    {
        // shim
    }
}

We have other functions that are shimmed in similar ways, like Py_NewRef for example. We should make a decision about how we want to treat these functions going forward.

@davidhewitt
Copy link
Member

davidhewitt commented Jul 26, 2024

@mejrs I take your point and I do agree that the documentation needs to reflect where we actually make these functions available. Also I agree that adding a public compat module is a new idea for pyo3-ffi and not a decision we should take lightly.

What I do feel reasonably strongly about is that we should try as much as possible to keep the existing files in pyo3-ffi be a faithful translation of the (latest) CPython headers. While this is obviously muddied by the existence of PyPy and GraalPy cfgs, my fear is adding code that is just for compatibility with older CPython versions is a new deviation from CPython. I worry it'll be harder for us to reconcile such code later, when we're trying to update these files for future CPython releases.

Isolating the compat code to a separate file seems like a good idea to me to separate out the deviations from the headers, though I am open to not having the new public pyo3_ffi::compat module and instead just have the structure you suggest exposed in dictobject.rs.

I think the question comes down to this: do we want pyo3_ffi to prioritise ease-of-use, or do we want to make it as thin a wrapper as possible around the CPython API?

If we want ease-of-use, I think just having PyDict_GetItemRef defined in dictobject.rs for all versions as you suggest is correct, where // shim in your snippet is replaced by crate::compat::PyDict_GetItemRef(dp, key, result).

If we want to prioritise being a thin wrapper around the CPython API, I think having pyo3_ffi::PyDict_GetItemRef match the CPython behaviour of only being available on 3.13 is correct and I stand by the current design here of having pyo3_ffi::compat as a new submodule which provides functions with a broader support range than the main API, so that users can opt-in to our shimming.

I am open to all parties' opinions on this question!


Py_NewRef is a slightly different case because the way we currently have it implemented matches how CPython does it with an override for non-abi3, however it does capture the same problem about cfgs and it looks like our documentation currently has this wrong (claims it's not available on the limited API where we define the extern function).

Probably we should tackle that case in a separate PR.

@mejrs
Copy link
Member

mejrs commented Jul 26, 2024

I am concerned that people end up writing code like

pub mod pyo3_ffi{
    pub mod compat {
        pub fn PyDict_GetItemRef(){}
    }
    mod dictobject{
        pub fn PyDict_GetItemRef(){}
    }
    pub use dictobject::*;
}

use pyo3_ffi::*;
use pyo3_ffi::compat::*;

fn main(){
    PyDict_GetItemRef();
}

which would raise ambiguous import errors if both are defined.

@davidhewitt
Copy link
Member

That's a strong point against a compat namespace for sure. Though I think most users would agree glob imports are problematic, it's not unthinkable that users would want to glob import like that to get a C-like experience of #include.

@mejrs
Copy link
Member

mejrs commented Jul 27, 2024

I think we can make a compat module work by just re-exporting the item if it does not need shimming with the current cfgs:

#![cfg_attr(docsrs, feature(doc_cfg))]

pub mod pyo3_ffi{
    pub mod compat {
        #[cfg_attr(docsrs, doc(cfg()))]
        #[cfg(Py_3_13)]
        pub use super::dictobject::PyDict_GetItemRef;
    
        // shim
        #[cfg_attr(docsrs, doc(cfg()))]
        #[cfg(not(Py_3_13))]
        pub fn PyDict_GetItemRef(){}
    }
    mod dictobject{
        #[cfg(Py_3_13)]
        pub fn PyDict_GetItemRef(){}
    }
    pub use dictobject::*;
}

use pyo3_ffi::*;
use pyo3_ffi::compat::*;

fn main(){
    PyDict_GetItemRef();
}

@ngoldbaum
Copy link
Contributor Author

I think I correctly applied the suggestion from #4355 (comment) in the last commit.

Let me know if I messed that up, it took me about 20 minutes of searching and then finding this discussion thread before I understood what the suggestion was.

@ngoldbaum
Copy link
Contributor Author

Hmm, nope, that broke the docs build. I think this has to do with pyo3-ffi being a workspace dependency, so the rustdoc configuration to set --cfg docsrs isn't being set for pyo3-ffi. Replicating the configuration to do from the top-level Cargo.toml to the pyo3-ffi Cargo.toml doesn't seem to help.

@davidhewitt
Copy link
Member

Try adding #![cfg_attr(docsrs, feature(doc_cfg, doc_auto_cfg))] to the top of pyo3-ffi/src/lib.rs (similar to how it is applied in src/lib.rs).

@ngoldbaum

This comment was marked as outdated.

@ngoldbaum
Copy link
Contributor Author

Finally got it, you need to declare features in the top-level lib.rs file. I think I knew that but I needed to relearn it I guess...

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

This looks working to me! @mejrs are you happy with the compat namespace now it is set up in this way?

pyo3-ffi/src/compat.rs Outdated Show resolved Hide resolved
@ngoldbaum

This comment was marked as outdated.

@ngoldbaum

This comment was marked as outdated.

@davidhewitt
Copy link
Member

I think hopefully #4397 will resolve.

@ngoldbaum

This comment was marked as outdated.

@davidhewitt davidhewitt added this pull request to the merge queue Aug 1, 2024
Merged via the queue into PyO3:main with commit b50fd81 Aug 1, 2024
42 of 43 checks passed
davidhewitt pushed a commit that referenced this pull request Sep 3, 2024
* Update dict.get_item binding to use PyDict_GetItemRef

Refs #4265

* test: add test for dict.get_item error path

* test: add test for dict.get_item error path

* test: add test for dict.get_item error path

* fix: fix logic error in dict.get_item bindings

* update: apply david's review suggestions for dict.get_item bindings

* update: create ffi::compat to store compatibility shims

* update: move PyDict_GetItemRef bindings to spot in order from dictobject.h

* build: fix build warning with --no-default-features

* doc: expand release note fragments

* fix: fix clippy warnings

* respond to review comments

* Apply suggestion from @mejrs

* refactor so cfg is applied to functions

* properly set cfgs

* fix clippy lints

* Apply @davidhewitt's suggestion

* deal with upstream deprecation of new_bound
davidhewitt pushed a commit that referenced this pull request Sep 15, 2024
* Update dict.get_item binding to use PyDict_GetItemRef

Refs #4265

* test: add test for dict.get_item error path

* test: add test for dict.get_item error path

* test: add test for dict.get_item error path

* fix: fix logic error in dict.get_item bindings

* update: apply david's review suggestions for dict.get_item bindings

* update: create ffi::compat to store compatibility shims

* update: move PyDict_GetItemRef bindings to spot in order from dictobject.h

* build: fix build warning with --no-default-features

* doc: expand release note fragments

* fix: fix clippy warnings

* respond to review comments

* Apply suggestion from @mejrs

* refactor so cfg is applied to functions

* properly set cfgs

* fix clippy lints

* Apply @davidhewitt's suggestion

* deal with upstream deprecation of new_bound
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