[internal] Switch from Rust-Cpython to PyO3 #13526
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
stuhood
reviewed
Nov 8, 2021
Eric-Arellano
force-pushed
the
pyo3
branch
4 times, most recently
from
9e29e72 to
0544e75
Compare
Eric-Arellano
added a commit
that referenced
this pull request
Nov 11, 2021
…3586) Prework for #13526. Before, we were parsing the Python objects (which requires the GIL) in the same iterator that created async blocks. That won't work when we port to PyO3 - iterating over the `file_items` collection uses `&'py PyAny` values, where the `'py` lifetime corresponds to how long we are holding the GIL (as represented by the `pyo3::Python` type). The `Python` token is not safe in async blocks because it does not implement `Send`, so we can't create the futures in this iterator. This fix makes the code much more understandable as well. In the future, we are unblocked from defining `CreateDigest` in Rust with `PyO3` because parsing of the code is now completely decoupled from using it to create async futures. [ci skip-build-wheels]
[ci skip-build-wheels]
[ci skip-build-wheels]
This will allow us to more clearly start porting things to PyO3 without confusing types. # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
[ci skip-build-wheels]
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
[ci skip-build-wheels]
[ci skip-build-wheels]
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
Don't pass the GIL over a future. Also stop using the obfuscation of `Value` Down to 7 errors! # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
stuhood
approved these changes
Nov 13, 2021
Comment on lines
+167
to
+169
| def __eq__(self, other: PySnapshot | Any) -> bool: ... | ||
| def __hash__(self) -> int: ... | ||
| def __repr__(self) -> str: ... |
There was a problem hiding this comment.
When is it necessary to declare these?
There was a problem hiding this comment.
Not strictly necessary. MyPy was not complaining about anything, I was being proactive so users of this API know what is expected to work without having to look at Rust code.
I believe __repr__ and __eq__ are always meant to be implemented, just with defaults though that is the object instance.
Wdyt? Too noisy?
There was a problem hiding this comment.
If not necessary, would leave them off probably. But doesn't really matter.
src/rust/engine/src/python.rs
Outdated
Comment on lines
272
to
273
| /// count, but that type only works when we are holding the GIL. While we could clone the | ||
| /// `PyObject` without the GIL, that would be an expensive deep clone. |
There was a problem hiding this comment.
Yea, possibly. Sounds like it is effectively already equivalent to an Arc.
Comment on lines
-95
to
+89
| pub fn getattr<T>(value: &PyObject, field: &str) -> Result<T, String> | ||
| pub fn getattr<'py, T>(value: &'py PyAny, field: &str) -> Result<T, String> | ||
| where | ||
| for<'a> T: FromPyObject<'a>, | ||
| T: FromPyObject<'py>, |
There was a problem hiding this comment.
Yea, makes sense. Before, T was not going to have its own lifetime (it was implicitly 'static), but now it does.
|
Others have the review covered so I'll step away. Thanks for checking coarse perf is not worse. |
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
Eric-Arellano
added a commit
that referenced
this pull request
Nov 14, 2021
Prework for #13526. We were reacquiring the GIL directly inside `py.allow_threads(||)`. Rust-cpython allows that, but PyO3 does not. (PyO3 has the benefit of instilling much stronger confidence in reasoning when the GIL is held or not.) We can easily work around this by having `WorkunitStore.with_latest_workunits` return `Vec<Workunit>` instead of `&[Workunit]`. No new clones are happening! We already were making this Vec. Only now we move the value out, rather than referencing it via a closure passed in as a callback. That change allows for operating on the `Vec<Workunit>` safely outside of the `py.allow_threads(||)` to convert it into the FFI boundary. ## Benchmark These benchmarks use the Toolchain BuildSense plugin + anonymous telemetrics streaming workunit handlers. ### No pantsd Before: ``` ❯ hyperfine -w 1 -r 10 './pants -ldebug --no-pantsd list ::' Time (mean ± σ): 4.611 s ± 0.206 s [User: 2.940 s, System: 0.474 s] Range (min … max): 4.419 s … 5.153 s 10 runs ``` After: ``` ❯ hyperfine -w 1 -r 10 './pants -ldebug --no-pantsd list ::' Time (mean ± σ): 4.655 s ± 0.113 s [User: 2.975 s, System: 0.509 s] Range (min … max): 4.430 s … 4.827 s 10 runs ``` ### Pantsd Before: ``` ❯ hyperfine -w 1 -r 10 './pants -ldebug --concurrent --pantsd list ::' Time (mean ± σ): 2.641 s ± 0.021 s [User: 2.026 s, System: 0.323 s] Range (min … max): 2.602 s … 2.670 s 10 runs ``` After: ``` ❯ hyperfine -w 1 -r 10 './pants -ldebug --concurrent --pantsd list ::' Time (mean ± σ): 2.651 s ± 0.023 s [User: 2.036 s, System: 0.334 s] Range (min … max): 2.611 s … 2.693 s 10 runs ``` [ci skip-build-wheels]
# Conflicts: # src/rust/engine/src/externs/interface.rs # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
|
I can still get Pants to crash on Linux by running This was happening in CI and locally on Linux (not macOS) this morning, and the core dump isolated it to line 380 from https://github.com/PyO3/pyo3/blob/47747444c76c9fc6bdd7800c55f3d6dad28a3c9b/src/python.rs#L365-L380. I tried fixing that with #13609, so that we don't reacquire the GIL inside The most recent core dump is this, which I am not sure what to make sense of: This is almost certainly an issue with our "streaming workunit handler" being async, as indicated by the core dump. I ran @davidhewitt @mejrs, do you expect it to be safe to use Edit: I have an idea how to isolate this failure to something more specific. Edit 2: tentatively looks like the problem is after exiting |
|
Hmm, as far as I'm aware you're not doing something fundamentally impossible. A quick search for "exception not rethrown" reaches this thread, which is "resolved", but it looks like the fix requires the nightly-only I suspect that some C code is forcing unwinding (maybe The difference between PyO3 and Rust-CPython on that line you mark is that Rust-CPython doesn't use I wonder if I change to a guard pattern (which should be enough) it will fix your issue. |
mejrs
reviewed
Nov 14, 2021
src/rust/engine/src/python.rs
Outdated
Comment on lines
272
to
273
| /// count, but that type only works when we are holding the GIL. While we could clone the | ||
| /// `PyObject` without the GIL, that would be an expensive deep clone. |
There was a problem hiding this comment.
That entire comment block is more confusing/irrelevant than it is helpful, I think. I'd probably omit it entirely or, as you say, do away with the entire newtype.
So I'm honestly not sure what the value of wrapping in an Arc<> is?
In this context there is no value, I think.
It's already cheap to clone
&PyAnyas it simply increases Python's refcount, but that type only works when we are holding the GIL.
Copying and passing around &PyAny doesn't involve reference counts, it's a borrowed reference. Having it bound to the gil lifetime allows for cheap and safe working with borrowed references. But this struct needs to own a reference to it, which is what Py<T> represents.
While we could clone the
PyObjectwithout the GIL, that would be an expensive deep clone.
Cloning a Py<T> without the GIL just increases a reference count (deferred into the future, when the gil is next acquired). No "deep copying" takes place.
Seems like we're adding unnecessary locking.
Arc doesn't involve locking, just some math on atomic integers. Or I'm misunderstanding your comment.
# Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
Eric-Arellano
added a commit
that referenced
this pull request
Nov 14, 2021
No need for two distinct extensions anymore post #13526.
tdyas
pushed a commit
to tdyas/pants
that referenced
this pull request
Nov 15, 2021
This reverts commit 021c3c0.
Merged
jsirois
added a commit
to jsirois/pants
that referenced
this pull request
Nov 28, 2021
This includes a fix we were waiting on in pantsbuild#13526: PyO3/pyo3#1990 Changelog here: https://pyo3.rs/v0.15.1/changelog.html # Building wheels and fs_util will be skipped. Delete if not intended. [ci skip-build-wheels]
jsirois
added a commit
that referenced
this pull request
Nov 28, 2021
This includes a fix we were waiting on in #13526: PyO3/pyo3#1990 Changelog here: https://pyo3.rs/v0.15.1/changelog.html
Eric-Arellano
added a commit
that referenced
this pull request
Dec 4, 2021
`PyObject` (aka `Py<PyAny>`) already provides a GIL-independent reference to a Python object. Wrapping it in `Arc` is not necessary, as described at #13526 (comment). Using `PyObject` directly removes a layer of indirection.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See #12110 for the original motivation.
Why migrate
Beyond the APIs requiring much less boilerplate and being more ergonomic (e.g. the
.call0()andcall1()variants of.call()), I think the biggest benefit is clearer modeling for when the GIL is held.With PyO3, the two core types are
&PyAnyandPyObject(akaPy<PyAny).&PyAnyis always a reference with the same lifetime as thePythontoken, which represents when the GIL is used. Whenever you want to do Python operations in Rust, like.getattr(), you use&PyAny.PyObjectand anyPy<T>are GIL-independent.In contrast, rust-cpython only had
PyObjectand&PyObject. It passed aroundPythonas an argument to any Python functions like.getattr().PyO3 is more disciplined about when we should have the GIL and when we should not. For example, #13586 and #13609 were necessary to get PyO3 working. This allows us to better reason about performance because we can know when the GIL is held or not.
--
An additional benefit is that using proc macros instead of declarative macros means PyCharm no longer hangs for me when working in the
engine/crate! PyCharm does much better w/ proc macros, and things feel zippy now like autocomplete working.Migration strategy
I tried originally doing an incremental strategy, most recently with #12451, which proved technically infeasible.
This PR completely swaps Rust-Cpython with PyO3, but tries to minimize the diff. It only makes changes when it was too difficult to get working with PyO3. For example,
interface.rsis far too big, but this keeps the same organization as before.For now, we still have the
native_engine_pyo3extension along withnative_engine. That will be consolidated in a followup.Benchmark
Before:
After: