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

auto-initialize: new feature to control initializing Python #1347

Merged
merged 2 commits into from
Jan 4, 2021

Conversation

davidhewitt
Copy link
Member

@davidhewitt davidhewitt commented Dec 28, 2020

This PR add a new feature auto-initialize to control whether Python::with_gil and Python::acquire_gil call prepare_freethreaded_python automatically.

I wanted to add this for a couple of reasons:

  • When building an extension-module then this functionality is not needed.
  • When linking against a static python lib (or PyPy) then this functionality doesn't even work correctly.

The PR checks for the static python and PyPy cases and removes the prepare_freethreaded_python API, and instead emits a compile_error! if the auto-initialize feature is enabled.

Closes #742
Closes #762
Closes #1213


TODO:

It would be nice to make this feature opt-in instead of opt-out, but that's a breaking change so we should delay that until 0.14 release.

I'd like to write some more documentation for this in the guide before merging.

Also needs CHANGELOG entry.

@kngwyu
Copy link
Member

kngwyu commented Dec 29, 2020

Thanks!
I really like the idea to raise a compile error before failing to link, but I need some clarifications:

  • About prepare_freethreaded_python:
    • I agree that this should be disabled for PyPy or static linking. Then, how about introducing some new configuration(e.g., Py_SHARED) and mark #[cfg(not(any(PyPy, Py_SHARED)))] fn prepare_freethreaded_python? I'm not sure this should be a feature flag.
  • About auto-initialize
    • Isn't this a not(extension-module)? Why do we need a new feature?

Again, this is a difficult problem, and thank you for tackling it.

@davidhewitt
Copy link
Member Author

Thanks for the questions. I'm very happy to change the design if we think alternatives are better. Here's my thinking about your proposed alternatives:

  • using Py_SHARED flag, I don't think we can fail compilation in build.rs. This means the error message when building will be that prepare_freethtreaded_python is missing. A nicer error than link failures maybe, especially if we add documentation to that function do that users understand what might have happened.

    The failure will still be quite hard to diagnose for the user compared to a friendly error message from the build script.

  • With auto-initialize being not(extension-module) - I suppose in practice yes, this is true. But I think this would then be another case of The extension-module feature fails to be additive #771 because then extension-module feature is taking away functionality again.

    Also, this would mean that users who want to run cargo test which relies on auto-initialize would have to always use the --no-default-features workaround to disable extension-module, even after Fix cargo test with extension-module feature. #1123 is stable. It seems more idiomatic Rust to me to be able to control this properly with a separate feature flag.

    The feature flag is also more general. For example, in some special cases users might want to embed python and manually initialize it before calling PyO3 code. In those cases they don't need auto-initialize functionality either.

@davidhewitt
Copy link
Member Author

... I'm thinking that maybe we can use Py_SHARED plus the auto-initialize feature. We can use compile_error! to produce a friendly error message, so my argument above for the embedding feature now seems weak.

@davidhewitt davidhewitt force-pushed the embedding branch 7 times, most recently from e336bbe to 1543525 Compare December 31, 2020 01:16
@davidhewitt
Copy link
Member Author

Reworked this PR as discussed but I expect CI to continue to fail until #1350 is merged, because one of the simplifications in that PR fixes how Py_SHARED would be set.

@davidhewitt davidhewitt mentioned this pull request Jan 1, 2021
@davidhewitt davidhewitt force-pushed the embedding branch 3 times, most recently from 9d4a410 to 5eb33f7 Compare January 1, 2021 08:14
@davidhewitt davidhewitt changed the title embedding: new feature to enable embedding Python auto-initialize: new feature to control initializing Python Jan 1, 2021
@davidhewitt
Copy link
Member Author

Rebased and rebuilt this PR as a single commit; hopefully CI is green this time 🤞

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.

LGTM, thanks!
CI failures are because auto-initialise is unintentionally invoked. I'm sorry but I have no idea why this happens.

src/gil.rs Outdated Show resolved Hide resolved
src/gil.rs Outdated
cfg_if::cfg_if! {
if #[cfg(all(feature = "auto-initialize", Py_SHARED, not(PyPy)))] {
prepare_freethreaded_python();
} else if #[cfg(all(feature = "auto-initialize", not(feature = "extension-module"), not(Py_SHARED), not(rustdoc)))] {
Copy link
Member

Choose a reason for hiding this comment

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

I think we can make this simpler by else if all(auto-initialize, PyPy) { ... } else if all(auto_initialize, ...)

Copy link
Member Author

@davidhewitt davidhewitt Jan 2, 2021

Choose a reason for hiding this comment

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

not(extension-module) is unfortunately necessary to avoid a breaking change - otherwise all extension modules would suddenly start failing to build PyPy wheels unless users changed their Cargo.toml to remove default features. We could make it as you suggest in 0.14.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, thanks. That makes sense.
BTW, do you think that we should keep auto-initialize in default-features?

Copy link
Member Author

Choose a reason for hiding this comment

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

I think in the long-run it's better to remove auto-initialize from default-features, but that's also a breaking change, so let's remove it in 0.14.

I'll create an issue to track these cleanups and add it to the 0.14 milestone.

Copy link
Member

Choose a reason for hiding this comment

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

👍🏼


- name: Build without default features
- name: Build (no features)
run: cargo build --no-default-features --verbose --target ${{ matrix.platform.rust-target }}
Copy link
Member

Choose a reason for hiding this comment

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

It looks like that auto-initialize and macros are enabled in this build, though I don't know why 😕

Copy link
Member Author

Choose a reason for hiding this comment

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

It's because of the Cargo.toml changes - see other comment.


# Run tests (except on PyPy, because no embedding API).
- if: matrix.python-version != 'pypy-3.6'
name: Test
run: cargo test --features "num-bigint num-complex hashbrown" --target ${{ matrix.platform.rust-target }}
run: cargo test --no-default-features --features "macros num-bigint num-complex hashbrown" --target ${{ matrix.platform.rust-target }}
Copy link
Member

Choose a reason for hiding this comment

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

Why not using auto-initialize?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's automatically included in tests because I put this in [dev-dependencies]:

# features needed to run the PyO3 test suite
pyo3 = { path = ".", default-features = false, features = ["macros", "auto-initialize"] }

However, that's also the reason why auto-initialize and macros are included in the cargo build command above. Interestingly cargo is about to stabilize a fix for this as a new resolver = "2" option in Cargo.toml. rust-lang/cargo#8997 (I tested on nightly and the features are no longer included in cargo build.)

Because in the future what I've done here will work correctly in the future, I'm going to keep the code mostly as-is for the moment and introduce a hack to avoid the immediate issue with a TODO to tidy it up once we bump MSRV in the future (probably to Rust 1.52).

@davidhewitt davidhewitt force-pushed the embedding branch 2 times, most recently from 02e27c1 to c2cc186 Compare January 2, 2021 16:05
@davidhewitt
Copy link
Member Author

@kngwyu I pushed a CHANGELOG entry and a new guide page guide/src/features.md to document this functionality.

I also added a tiny "hack" with cfg(__pyo3_ci) to suppress the error message in our CI until we are able to bump MSRV to a Rust version which includes the cargo resolver = "2". Might be nearly a year though - I put TODO on the location in ci.yml and build.rs where I added the hack.

If you are happy with this, I think this is ready to merge! 🚀

@kngwyu
Copy link
Member

kngwyu commented Jan 4, 2021

I also added a tiny "hack" with cfg(__pyo3_ci) to suppress the error message in our CI until we are able to bump MSRV to a Rust version which includes the cargo resolver = "2". Might be nearly a year though - I put TODO on the location in ci.yml and build.rs where I added the hack.

Is it much simpler to remove the dev-dependency for now?

@davidhewitt
Copy link
Member Author

If we remove the dev-dependency and also remove auto-initialize from default features then to run the PyO3 test suite we'll have to write cargo test --features auto-initialize.

I'm sure I'll forget to add the feature a lot and it's also annoying to type something so long.

I think it's better to leave the very small hack in so that we can make exactly the API we want for users. Especially as we know the fix which removes the need for the hack is going to be stabilised very soon.

(Because we only need the hack to fix the pypy CI jobs, we only need to wait until it's in the most recent stable rust version.)

@kngwyu
Copy link
Member

kngwyu commented Jan 4, 2021

If we remove the dev-dependency and also remove auto-initialize from default features

OK, if you are to release 0.14 before the next stable release, let's go in this way 👍🏼

@davidhewitt
Copy link
Member Author

(I force-pushed to fix the commit message. Will merge shortly.)

@davidhewitt davidhewitt merged commit 8e37d37 into PyO3:master Jan 4, 2021
@davidhewitt davidhewitt deleted the embedding branch January 4, 2021 21:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants