Skip to content

refactor(bindings/python): use declarative pymodule syntax#7812

Merged
chitralverma merged 12 commits into
apache:mainfrom
chitralverma:refactor/python-declarative-pymodule
Jun 24, 2026
Merged

refactor(bindings/python): use declarative pymodule syntax#7812
chitralverma merged 12 commits into
apache:mainfrom
chitralverma:refactor/python-declarative-pymodule

Conversation

@chitralverma

Copy link
Copy Markdown
Contributor

Which issue does this PR close?

Closes #.

Rationale for this change

The Python binding built its submodules (operator, file, capability, services, layers, types, exceptions) imperatively via the add_pymodule! / add_pyexceptions! macros and a fn-based #[pymodule]. PyO3 recommends the declarative #[pymodule] mod ... form with #[pymodule_export] (PyO3 module guide). This PR moves the binding to that style.

What changes are included in this PR?

  • _opendal is now a #[pymodule] mod with one nested #[pymodule] mod per submodule; classes are attached with #[pymodule_export].
  • __version__ is exported as a #[pymodule_export] pub const instead of being set in an init function.
  • Replaced the add_pymodule! / add_pyexceptions! macros with a register_in_sys helper (registers each submodule in sys.modules and sets its __name__) and an add_exceptions! macro.
  • The exceptions submodule adds its members procedurally, since create_exception! types are PyErr subtypes rather than #[pyclass]es and cannot use #[pymodule_export].
  • Refreshed uv.lock.

Note: cargo run --bin stub_gen already fails on main (independent of this change) and is not addressed here; it will be fixed in a future PR.

Are there any user-facing changes?

No API changes. One minor behavioral correction: submodule __name__ is now the fully qualified dotted name (e.g. opendal.operator) instead of the bare name (operator), matching the sys.modules key, the classes' __module__, and standard-library conventions. All public imports (from opendal import Operator, from opendal.exceptions import NotFound, etc.) and opendal.__version__ are unchanged.

AI Usage Statement

Convert _opendal from the imperative add_pymodule!/add_pyexceptions!
macros to PyO3's declarative #[pymodule] mod form with #[pymodule_export].
Export __version__ as a #[pymodule_export] const, and add a register_in_sys
helper plus an add_exceptions! macro. Submodule __name__ is now the
fully qualified dotted name (e.g. opendal.operator).

Refs: https://pyo3.rs/v0.29.0/module.html
Copilot AI review requested due to automatic review settings June 22, 2026 11:50
@dosubot dosubot Bot added size:L This PR changes 100-499 lines, ignoring generated files. releases-note/refactor The PR does a refactor on code or has a title that begins with "refactor" labels Jun 22, 2026

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR refactors the Rust-backed Python extension module (_opendal) to use PyO3’s declarative #[pymodule] mod ... syntax with #[pymodule_export], replacing the prior imperative submodule construction macros. It also centralizes submodule registration to ensure sys.modules and __name__ reflect fully-qualified opendal.* module names.

Changes:

  • Converted _opendal from a fn-based #[pymodule] initializer to a declarative #[pymodule] mod with nested submodules and #[pymodule_export] exports.
  • Replaced add_pymodule! / add_pyexceptions! with a register_in_sys helper and a simplified add_exceptions! macro.
  • Exported __version__ via #[pymodule_export] pub const __version__ instead of setting it during module init.

Reviewed changes

Copilot reviewed 2 out of 3 changed files in this pull request and generated 3 comments.

File Description
bindings/python/src/utils.rs Introduces register_in_sys and add_exceptions! to support the new declarative module layout and qualified sys.modules registration.
bindings/python/src/lib.rs Rewrites _opendal module initialization into declarative nested #[pymodule] submodules and exports __version__ as a module constant.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread bindings/python/src/lib.rs
Comment thread bindings/python/src/lib.rs
Comment thread bindings/python/src/utils.rs Outdated
@chitralverma

chitralverma commented Jun 22, 2026

Copy link
Copy Markdown
Contributor Author

@erickguan @Xuanwo please have a look when you get a chance.

This is in continuation of #7181

Comment thread bindings/python/uv.lock
@chitralverma

Copy link
Copy Markdown
Contributor Author

@PsiACE you wanna check this one as well?

@erickguan erickguan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Generally I like using declarative modules as documentation provided. Perhaps we could stick on one style for majority of the code?

Comment thread bindings/python/src/lib.rs Outdated

#[pymodule_init]
fn init(m: &pyo3::Bound<'_, pyo3::types::PyModule>) -> pyo3::PyResult<()> {
crate::register_in_sys(m, "operator")

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Can we use #[pymodule] for submodules too?

I know packaging might be a problem. Here is what I found.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The 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 register_submodules walk called once from the root #[pymodule_init], replacing the per-submodule registration.

For opendal, we don't need the submodule flag or the PyModuleSubmoduleExt trait as all our submodules are nested inside mod _opendal, so PyO3 auto-marks them as submodules.

Comment thread bindings/python/src/lib.rs
… walk

Replace the per-submodule `register_in_sys` calls (one `#[pymodule_init]`
each) with a single `register_submodules` walk invoked once from the root
module. It recurses over the declarative submodules and inserts them into
`sys.modules` under the public `opendal` name, fixing each `__name__`.

Adapted from the approaches in PyO3 issue apache#759. The data submodules are now
plain `#[pymodule] mod` blocks; only `exceptions` keeps an init for
`add_exceptions!`.
@chitralverma chitralverma requested a review from erickguan June 23, 2026 07:40
…rsive fn

It is only called from the root `#[pymodule_init]`, so fold the thin
`sys.modules`-import wrapper into the recursive function itself.
…dule' into refactor/python-declarative-pymodule

@erickguan erickguan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good. One final question on opendal module

]
)?;
Ok(())
mod _opendal {

Copy link
Copy Markdown
Member

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?

#[pymodule(gil_used = false)]
mod opendal {
}

@chitralverma chitralverma Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes — mod _opendal here is the declarative #[pymodule] mod form (converted from the old fn _opendal).

It has to stay _opendal, not opendal: this is a mixed layout, so module-name = "opendal._opendal" builds opendal/_opendal.*.so and the hand-written opendal/__init__.py is the real package that re-exports from it. The mod name must match the .so's last path component (PyInit__opendal). Naming it opendal would require the native module to be the top-level package, colliding with the opendal/ package dir.

Copy link
Copy Markdown
Member

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:

We are using a mixed Python and native package for python OpenDAL binding.
To avoid conflict of ... 
<And a concise explanation of how it works here should be enough>

Copy link
Copy Markdown
Contributor Author

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 _opendal explaining the mixed-layout naming (ba8627b).

@erickguan erickguan self-requested a review June 24, 2026 14:06
@dosubot dosubot Bot added the lgtm This PR has been approved by a maintainer label Jun 24, 2026
@chitralverma chitralverma merged commit 9d7c68e into apache:main Jun 24, 2026
37 checks passed
@chitralverma chitralverma deleted the refactor/python-declarative-pymodule branch June 24, 2026 15:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lgtm This PR has been approved by a maintainer releases-note/refactor The PR does a refactor on code or has a title that begins with "refactor" size:L This PR changes 100-499 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants