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

Move macros into separate feature. #897

Merged
merged 1 commit into from
May 11, 2020

Conversation

m-ou-se
Copy link
Contributor

@m-ou-se m-ou-se commented May 4, 2020

This makes it possible to opt-out of all the (proc) macros, which reduces the amount of dependencies that need to be downloaded and built.

Together with #895, #896 and #899, this reduces the dependency tree from:

pyo3 v0.9.2
├── indoc v0.3.5
│   ├── indoc-impl v0.3.5
│   │   ├── proc-macro-hack v0.5.15
│   │   ├── proc-macro2 v1.0.10
│   │   │   └── unicode-xid v0.2.0
│   │   ├── quote v1.0.3
│   │   │   └── proc-macro2 v1.0.10 (*)
│   │   ├── syn v1.0.17
│   │   │   ├── proc-macro2 v1.0.10 (*)
│   │   │   ├── quote v1.0.3 (*)
│   │   │   └── unicode-xid v0.2.0
│   │   └── unindent v0.1.5
│   └── proc-macro-hack v0.5.15
├── inventory v0.1.6
│   ├── ctor v0.1.14
│   │   ├── quote v1.0.3 (*)
│   │   └── syn v1.0.17 (*)
│   ├── ghost v0.1.1
│   │   ├── proc-macro2 v1.0.10 (*)
│   │   ├── quote v1.0.3 (*)
│   │   └── syn v1.0.17 (*)
│   └── inventory-impl v0.1.6
│       ├── proc-macro2 v1.0.10 (*)
│       ├── quote v1.0.3 (*)
│       └── syn v1.0.17 (*)
├── libc v0.2.69
├── num-traits v0.2.11
│   [build-dependencies]
│   └── autocfg v1.0.0
├── parking_lot v0.10.2
│   ├── lock_api v0.3.4
│   │   └── scopeguard v1.1.0
│   └── parking_lot_core v0.7.2
│       ├── cfg-if v0.1.10
│       ├── libc v0.2.69
│       └── smallvec v1.4.0
├── paste v0.1.11
│   ├── paste-impl v0.1.11
│   │   ├── proc-macro-hack v0.5.15
│   │   ├── proc-macro2 v1.0.10 (*)
│   │   ├── quote v1.0.3 (*)
│   │   └── syn v1.0.17 (*)
│   └── proc-macro-hack v0.5.15
├── pyo3cls v0.9.2
│   ├── pyo3-derive-backend v0.9.2
│   │   ├── proc-macro2 v1.0.10 (*)
│   │   ├── quote v1.0.3 (*)
│   │   └── syn v1.0.17 (*)
│   ├── quote v1.0.3 (*)
│   └── syn v1.0.17 (*)
└── unindent v0.1.5
[build-dependencies]
├── regex v1.3.7
│   └── regex-syntax v0.6.17
├── serde v1.0.106
│   └── serde_derive v1.0.106
│       ├── proc-macro2 v1.0.10 (*)
│       ├── quote v1.0.3 (*)
│       └── syn v1.0.17 (*)
├── serde_json v1.0.51
│   ├── itoa v0.4.5
│   ├── ryu v1.0.3
│   └── serde v1.0.106 (*)
└── version_check v0.9.1

to:

pyo3 v0.9.2
└── libc v0.2.69
[build-dependencies]
└── version_check v0.9.1

with --no-default-features.

@davidhewitt
Copy link
Member

davidhewitt commented May 4, 2020

Hmmm this is an interesting idea! In principle I am keen to give users an option to have a smaller compile for sure.

I wonder - once the macros are removed then a lot of other code becomes irrelevant I think - e.g. PyCell, and the entirety of src/class. Rather than just drawing the line at macros, should we feature-gate just a little more code? This would get us a long way towards satisfying the recent suggestion in #5 to add a feature to subset pyo3 just to stable code.

@m-ou-se
Copy link
Contributor Author

m-ou-se commented May 4, 2020

Yeah, with feature = "macros" a lot of the code is unused (or should be), but I didn't manage to find a nice line to cut things off at. I tried putting PyClass behind the feature = "macros" gate, but that broke a lot of other things. Also gating the things it broke, broke even more things, and so on. So in the end I gave up and just took the easy way out with a dummy implementation for PyMethodsImpl in the not(feature = "macros") case, and leaving all the PyClass stuff in place.

My main goal was to avoid pulling in unnecessary dependencies, so I'm not too worried about parts of the pyo3 crate that stay unused.

It would definitely be nice to split this better (maybe even separate crates?), but that's probably a lot of work.

@davidhewitt
Copy link
Member

👍 maybe I will have a quick play with this later to see if I can find something which feels good.

I'm in split opinion about separate crates. It helps with some big projects but it does separate the documentation out into multiple places, which can make it a little bit harder for users to find the docs they need...

@ijl
Copy link
Contributor

ijl commented May 4, 2020

@m-ou-se how much of pyo3 outside of the FFI bindings does your use case exercise if it doesn't use pymodule, pyfunction, etc.? Would moving the FFI bindings into a pyo3ffi crate be useful?

Copy link
Member

@kngwyu kngwyu left a comment

Choose a reason for hiding this comment

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

Thank you!

It's a bit surprising for me that we can separate features by such a simple trick.

@@ -142,3 +144,11 @@ pub trait PyMethodsImpl {
.collect()
}
}

#[doc(hidden)]
#[cfg(not(feature = "macros"))]
Copy link
Member

Choose a reason for hiding this comment

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

Nice 👍

@kngwyu
Copy link
Member

kngwyu commented May 5, 2020

This would get us a long way towards satisfying the recent suggestion in #5 to add a feature to subset pyo3 just to stable code.

You mean we should separate PyClass? If so, please wait until my next trial to remove specialization fails.

Would moving the FFI bindings into a pyo3ffi crate be useful?

@ijl
It's OK to separate FFI parts if you need them. For orjson?
My only concern is how the build script should be.

@kngwyu kngwyu closed this May 5, 2020
@kngwyu kngwyu reopened this May 5, 2020
@m-ou-se
Copy link
Contributor Author

m-ou-se commented May 5, 2020

@ijl

@m-ou-se how much of pyo3 outside of the FFI bindings does your use case exercise if it doesn't use pymodule, pyfunction, etc.? Would moving the FFI bindings into a pyo3ffi crate be useful?

The inline-python crate requires PyDict and ToPyObject and some other stuff, but none of the macros.

Another crate I'm about to publish converts objects to and frorm PyO3's PyObject using serde. It uses PyDict, PyList, PyTuple and more, but again no macros.

All the macros like #[pymodule] are mostly used when using Rust from Python. But when using Python from Rust, they're often not needed (in simple cases).

@@ -280,6 +278,7 @@ macro_rules! wrap_pymodule {
/// If you need to handle failures, please use [Python::run] directly.
///
#[macro_export]
#[cfg(feature = "macros")]
Copy link
Member

Choose a reason for hiding this comment

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

I'm sorry for my lazy review but could you please add this definition?

#[macro_export]
#[cfg(not(feature = "macros"))]
macro_rules! py_run {
    ($py:expr, $($val:ident)+, $code:expr) => {{
        pyo3::py_run_impl!($py, $($val)+, &pyo3::unindent::unindent($code))
    }};
}

unindent is a really small dependency (see its Cargo.toml) and I think it's OK.

Copy link
Member

Choose a reason for hiding this comment

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

then py_run would behave differently depending on the feature? that seems confusing...

Copy link
Contributor Author

@m-ou-se m-ou-se May 5, 2020

Choose a reason for hiding this comment

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

That would mean that code like

let code = "asdf";
py_run!(py, a b, code);

would compile fine with feature = "macros" disabled, but will no longer compile when it gets enabled.

Copy link
Member

@kngwyu kngwyu May 5, 2020

Choose a reason for hiding this comment

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

then py_run would behave differently depending on the feature?

No, it behaves the same.
But

  1. with feature="macros" and
  2. string literal is passed like py_run!(py, a, "print(a)")

it works faster thanks to indoc.

@ijl
Copy link
Contributor

ijl commented May 5, 2020

I don't have a need to separate the FFI right now. It is simple to do, though. The build.rs in each should be duplicated, but this is rarely touched and I think pretty mild for how difficult some build processes can be, so not that bad. So if it helps #904 or something else it's simple to do (I have a branch if needed).

@kngwyu
Copy link
Member

kngwyu commented May 9, 2020

Could you please rebase on the master?
Github doesn't allow maintainers to edit an organization branch,

It's enabled by default to avoid breakage, but this allows compiling
pyo3 with a lot less dependencies in case the macros are not needed.
@kngwyu
Copy link
Member

kngwyu commented May 11, 2020

Thanks!

@kngwyu kngwyu merged commit da22eec into PyO3:master May 11, 2020
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.

5 participants