Update for Pyo3 0.23 by ngoldbaum · Pull Request #1289 · Instagram/LibCST

ngoldbaum · 2025-01-28T22:35:21Z

Summary

Updates the native library to use PyO3 0.23. This is the first step towards declaring support for the free-threaded interpreter.

I could have gone further in ripping out TryIntoPy in favor of IntoPyObject. If you'd prefer not to leave this project in a state with an unnecessary wrapper trait I'm happy to implement that, I just wanted to make the smallest possible working diff.

Test Plan

This passes the tests for me locally. Let's see if CI spots and issues.

facebook-github-bot · 2025-01-28T22:35:27Z

Hi @ngoldbaum!

Thank you for your pull request.

We require contributors to sign our Contributor License Agreement, and yours needs attention.

You currently have a record in our system, but the CLA is no longer valid, and will need to be resubmitted.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at cla@meta.com. Thanks!

ngoldbaum · 2025-01-28T22:39:18Z

I signed the CLA.

facebook-github-bot · 2025-01-28T23:11:02Z

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

ngoldbaum · 2025-01-28T23:16:29Z

native/libcst/src/nodes/traits.rs

-        }
-    }
-
-    impl TryIntoPy<PyObject> for PyTuple {


I found that with IntoPyObject that some of these impls aren't needed at all. I didn't look too closely to try to understand why though so there could be something subtle happening.

Great, I was hoping to get rid of these.

ngoldbaum · 2025-01-28T23:20:29Z

native/libcst/src/parser/errors.rs

+                }() {
+                    Ok(py_err_value) => PyErr::from_value(py_err_value),
+                    Err(e) => e,
+                }


Since ultimately we want a PyErr in both the success and error cases, we can make use of a closure that returns a result and ? to avoid unwrap or expect in the new into_pyobject() calls that can return errors and at the same time clean up some unnecessary pre-existing expect use to turn some panics into errors. This is a neat Rust trick, unfortuntely it's only convenient in error handling code.

ngoldbaum · 2025-01-28T23:20:56Z

native/libcst/src/parser/errors.rs

        }
    }
-
-    impl<'a> PyErrArguments for Details {


Both this and Details are unused and the compiler warns about them.

zsol

LGTM in general, thanks for this. I'd appreciate a followup PR that rips out TryIntoPy completely, but happy to land this as is if we can figure out a way around that unwrap() call

zsol · 2025-02-21T22:17:32Z

native/libcst/src/nodes/traits.rs

-        }
-    }
-
-    impl TryIntoPy<PyObject> for PyTuple {


Great, I was hoping to get rid of these.

zsol · 2025-02-21T22:19:40Z

native/libcst/src/nodes/traits.rs

-    impl TryIntoPy<PyObject> for PyTuple {
-        fn try_into_py(self, py: Python) -> PyResult<PyObject> {
-            Ok(self.into_py(py))
+            PyTuple::new(py, converted).unwrap().into_py_any(py)


Anything we can do to avoid the unwrap() and potential panic here?

This PR: 1. marks the `libcst.native` module as free-threading-compatible 2. replaces the use of ProcessPoolExecutor with ThreadPoolExecutor if free-threaded CPython is detected at runtime This depends on #1294 and #1289.

zsol · 2025-02-22T20:04:47Z

Ugh, apologies for the churn, my tools messed with the PR - I did my best to revert my changes

ngoldbaum · 2025-03-03T16:14:50Z

Sorry for the delay here - I accidentally marked the TODO item I had for this as "done". Will try to deal with the unwrap() calls today.

zsol · 2025-03-07T15:35:28Z

🎉 thanks!

This PR: 1. marks the `libcst.native` module as free-threading-compatible 2. replaces the use of ProcessPoolExecutor with ThreadPoolExecutor if free-threaded CPython is detected at runtime This depends on #1294 and #1289.

This PR: 1. marks the `libcst.native` module as free-threading-compatible 2. replaces the use of ProcessPoolExecutor with ThreadPoolExecutor if free-threaded CPython is detected at runtime This depends on Instagram#1294 and Instagram#1289.

This PR: 1. marks the `libcst.native` module as free-threading-compatible 2. replaces the use of ProcessPoolExecutor with ThreadPoolExecutor if free-threaded CPython is detected at runtime This depends on #1294 and #1289.

* Update Cargo.lock and Cargo.toml for PyO3 0.23 support * Replace deprecated _bound methods with their new undeprecated names * Update TryIntoPy trait to use IntoPyObject * Update ParserError wrapper to use IntoPyObject * replace unwrap with early return

ngoldbaum added 4 commits January 28, 2025 15:28

Update Cargo.lock and Cargo.toml for PyO3 0.23 support

467cd12

Replace deprecated _bound methods with their new undeprecated names

0ddd59a

Update TryIntoPy trait to use IntoPyObject

efe3fbd

Update ParserError wrapper to use IntoPyObject

c2e8673

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jan 28, 2025

ngoldbaum commented Jan 28, 2025

View reviewed changes

zsol approved these changes Feb 21, 2025

View reviewed changes

zsol mentioned this pull request Feb 22, 2025

Enable support for free-threading #1295

Merged

zsol changed the title ~~Update for Pyo3 0.23~~ Update ParserError wrapper to use IntoPyObject Feb 22, 2025

zsol changed the title ~~Update ParserError wrapper to use IntoPyObject~~ Update for Pyo3 0.23 Feb 22, 2025

replace unwrap with early return

b038df7

ngoldbaum mentioned this pull request Mar 7, 2025

Bump pyo3 from 0.22.2 to 0.23.5 in /native #1300

Closed

zsol merged commit 727e433 into Instagram:main Mar 7, 2025
25 checks passed

ngoldbaum mentioned this pull request Mar 7, 2025

Publish cp313t (free-threaded) wheels #1242

Closed

Conversation

ngoldbaum commented Jan 28, 2025 • edited by zsol Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test Plan

Uh oh!

facebook-github-bot commented Jan 28, 2025

Process

Uh oh!

ngoldbaum commented Jan 28, 2025

Uh oh!

facebook-github-bot commented Jan 28, 2025

Uh oh!

ngoldbaum Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

zsol Feb 21, 2025

Choose a reason for hiding this comment

Uh oh!

ngoldbaum Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ngoldbaum Jan 28, 2025

Choose a reason for hiding this comment

Uh oh!

zsol left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zsol Feb 21, 2025

Choose a reason for hiding this comment

Uh oh!

zsol Feb 21, 2025

Choose a reason for hiding this comment

Uh oh!

zsol commented Feb 22, 2025

Uh oh!

ngoldbaum commented Mar 3, 2025

Uh oh!

Uh oh!

zsol commented Mar 7, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ngoldbaum commented Jan 28, 2025 •

edited by zsol

Loading

ngoldbaum Jan 28, 2025 •

edited

Loading

zsol left a comment •

edited

Loading