-
Notifications
You must be signed in to change notification settings - Fork 778
refactor(bindings/python): use declarative pymodule syntax #7812
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
Changes from 2 commits
30578d3
c38490e
514abfc
7b6a770
8767759
c943bec
2510236
fc026fa
6c3e861
58e974b
ba8627b
798b86f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -43,72 +43,120 @@ use pyo3_stub_gen::{define_stub_info_gatherer, derive::*}; | |
| pub use services::*; | ||
|
|
||
| #[pymodule(gil_used = false)] | ||
| fn _opendal(py: Python, m: &Bound<'_, PyModule>) -> PyResult<()> { | ||
| // Add version | ||
| m.add("__version__", env!("CARGO_PKG_VERSION"))?; | ||
|
|
||
| // Operator module | ||
| add_pymodule!(py, m, "operator", [Operator, AsyncOperator])?; | ||
|
|
||
| // File module | ||
| add_pymodule!(py, m, "file", [File, AsyncFile])?; | ||
|
|
||
| // Capability module | ||
| add_pymodule!(py, m, "capability", [Capability])?; | ||
|
|
||
| // Services module | ||
| add_pymodule!(py, m, "services", [PyScheme])?; | ||
|
|
||
| // Layers module | ||
| add_pymodule!( | ||
| py, | ||
| m, | ||
| "layers", | ||
| [ | ||
| Layer, | ||
| CapabilityOverrideLayer, | ||
| RetryLayer, | ||
| ConcurrentLimitLayer, | ||
| MimeGuessLayer | ||
| ] | ||
| )?; | ||
|
|
||
| // Types module | ||
| add_pymodule!( | ||
| py, | ||
| m, | ||
| "types", | ||
| [Entry, EntryMode, Metadata, PresignedRequest] | ||
| )?; | ||
|
|
||
| m.add_class::<WriteOptions>()?; | ||
| m.add_class::<ReadOptions>()?; | ||
| m.add_class::<ListOptions>()?; | ||
| m.add_class::<StatOptions>()?; | ||
| m.add_class::<DeleteOptions>()?; | ||
|
|
||
| // Exceptions module | ||
| add_pyexceptions!( | ||
| py, | ||
| m, | ||
| "exceptions", | ||
| [ | ||
| Error, | ||
| Unexpected, | ||
| Unsupported, | ||
| ConfigInvalid, | ||
| NotFound, | ||
| PermissionDenied, | ||
| IsADirectory, | ||
| NotADirectory, | ||
| AlreadyExists, | ||
| IsSameFile, | ||
| ConditionNotMatch, | ||
| RateLimited, | ||
| RangeNotSatisfied, | ||
| ] | ||
| )?; | ||
| Ok(()) | ||
| mod _opendal { | ||
| use pyo3::prelude::*; | ||
|
|
||
|
chitralverma marked this conversation as resolved.
|
||
| #[pymodule] | ||
| mod operator { | ||
| #[pymodule_export] | ||
| use crate::{AsyncOperator, Operator}; | ||
|
|
||
| #[pymodule_init] | ||
| fn init(m: &pyo3::Bound<'_, pyo3::types::PyModule>) -> pyo3::PyResult<()> { | ||
| crate::register_in_sys(m, "operator") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we use I know packaging might be a problem. Here is what I found.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I looked at the linked comment and the alternative below it and modified the current implementation with a single recursive For opendal, we don't need the |
||
| } | ||
| } | ||
|
|
||
| #[pymodule] | ||
| mod file { | ||
| #[pymodule_export] | ||
| use crate::{AsyncFile, File}; | ||
|
|
||
| #[pymodule_init] | ||
| fn init(m: &pyo3::Bound<'_, pyo3::types::PyModule>) -> pyo3::PyResult<()> { | ||
| crate::register_in_sys(m, "file") | ||
| } | ||
| } | ||
|
|
||
| #[pymodule] | ||
| mod capability { | ||
| #[pymodule_export] | ||
| use crate::Capability; | ||
|
|
||
| #[pymodule_init] | ||
| fn init(m: &pyo3::Bound<'_, pyo3::types::PyModule>) -> pyo3::PyResult<()> { | ||
| crate::register_in_sys(m, "capability") | ||
| } | ||
| } | ||
|
|
||
| #[pymodule] | ||
| mod services { | ||
| #[pymodule_export] | ||
| use crate::PyScheme; | ||
|
|
||
| #[pymodule_init] | ||
| fn init(m: &pyo3::Bound<'_, pyo3::types::PyModule>) -> pyo3::PyResult<()> { | ||
| crate::register_in_sys(m, "services") | ||
| } | ||
| } | ||
|
|
||
| #[pymodule] | ||
| mod layers { | ||
| #[pymodule_export] | ||
| use crate::{ | ||
| CapabilityOverrideLayer, ConcurrentLimitLayer, Layer, MimeGuessLayer, RetryLayer, | ||
| }; | ||
|
|
||
| #[pymodule_init] | ||
| fn init(m: &pyo3::Bound<'_, pyo3::types::PyModule>) -> pyo3::PyResult<()> { | ||
| crate::register_in_sys(m, "layers") | ||
| } | ||
| } | ||
|
|
||
| #[pymodule] | ||
| mod types { | ||
| #[pymodule_export] | ||
| use crate::{Entry, EntryMode, Metadata, PresignedRequest}; | ||
|
|
||
| #[pymodule_init] | ||
| fn init(m: &pyo3::Bound<'_, pyo3::types::PyModule>) -> pyo3::PyResult<()> { | ||
| crate::register_in_sys(m, "types") | ||
| } | ||
| } | ||
|
|
||
| // Exceptions are `PyErr` subtypes, not `#[pyclass]`es, so they are added | ||
| // procedurally rather than via `#[pymodule_export]`. | ||
| #[pymodule] | ||
| mod exceptions { | ||
| #[pymodule_init] | ||
| fn init(m: &pyo3::Bound<'_, pyo3::types::PyModule>) -> pyo3::PyResult<()> { | ||
| use pyo3::prelude::*; | ||
|
|
||
|
chitralverma marked this conversation as resolved.
|
||
| use crate::{ | ||
| AlreadyExists, ConditionNotMatch, ConfigInvalid, Error, IsADirectory, IsSameFile, | ||
| NotADirectory, NotFound, PermissionDenied, RangeNotSatisfied, RateLimited, | ||
| Unexpected, Unsupported, add_exceptions, | ||
| }; | ||
|
|
||
| add_exceptions!( | ||
|
chitralverma marked this conversation as resolved.
|
||
| m, | ||
| [ | ||
| Error, | ||
| Unexpected, | ||
| Unsupported, | ||
| ConfigInvalid, | ||
| NotFound, | ||
| PermissionDenied, | ||
| IsADirectory, | ||
| NotADirectory, | ||
| AlreadyExists, | ||
| IsSameFile, | ||
| ConditionNotMatch, | ||
| RateLimited, | ||
| RangeNotSatisfied, | ||
| ] | ||
| )?; | ||
| crate::register_in_sys(m, "exceptions") | ||
| } | ||
| } | ||
|
|
||
| // Option types live directly on the top-level `opendal` module. | ||
| #[pymodule_export] | ||
| use crate::{DeleteOptions, ListOptions, ReadOptions, StatOptions, WriteOptions}; | ||
|
|
||
| #[pymodule_export] | ||
| #[allow(non_upper_case_globals)] | ||
| pub const __version__: &str = env!("CARGO_PKG_VERSION"); | ||
| } | ||
|
|
||
| define_stub_info_gatherer!(stub_info); | ||
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.
Does declarative syntax works for modules?
Uh oh!
There was an error while loading. Please reload this page.
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.
Yes —
mod _opendalhere is the declarative#[pymodule] modform (converted from the oldfn _opendal).It has to stay
_opendal, notopendal: this is a mixed layout, somodule-name = "opendal._opendal"buildsopendal/_opendal.*.soand the hand-writtenopendal/__init__.pyis the real package that re-exports from it. The mod name must match the.so's last path component (PyInit__opendal). Naming itopendalwould require the native module to be the top-level package, colliding with theopendal/package dir.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.
Thanks for your detailed explanation! I would recommend documenting:
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.
Added a comment above
mod _opendalexplaining the mixed-layout naming (ba8627b).