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

refactor!: drop RustPython support #3749

Closed
wants to merge 10 commits into from

Conversation

tisonkun
Copy link
Collaborator

I hereby agree to the terms of the GreptimeDB CLA.

Refer to a related PR or issue link (optional)

What's changed and what's your intention?

This supersedes and thus closes #3723.

This closes #3063.

Drop RustPython backend for scripting and always build with PyO3. This is an attempt to simplify our scripting functions.

This should involve a lot of docs changes, but we haven't decided yet. Open this patch as a preview.

Checklist

  • I have written the necessary rustdoc comments.
  • I have added the necessary unit tests and integration tests.
  • This PR does not require documentation updates.

tisonkun and others added 5 commits April 19, 2024 15:59
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Co-authored-by: Jeong YunWon <jeong@youknowone.org>
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
@tisonkun tisonkun requested a review from a team as a code owner April 19, 2024 08:51
@tisonkun tisonkun changed the title Drop rustpython refactor!: drop RustPython support Apr 19, 2024
@github-actions github-actions bot added docs-not-required This change does not impact docs. Invalid PR Title breaking-change This pull request contains breaking changes. labels Apr 19, 2024
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
Copy link

codecov bot commented Apr 19, 2024

Codecov Report

Attention: Patch coverage is 56.11111% with 79 lines in your changes are missing coverage. Please review.

Project coverage is 85.25%. Comparing base (cfed466) to head (46d0194).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3749      +/-   ##
==========================================
- Coverage   85.63%   85.25%   -0.38%     
==========================================
  Files         944      937       -7     
  Lines      158776   156542    -2234     
==========================================
- Hits       135966   133464    -2502     
- Misses      22810    23078     +268     

Signed-off-by: tison <wander4096@gmail.com>
@@ -149,6 +149,10 @@ jobs:
run: |
sudo apt update && sudo apt install -y libfuzzer-14-dev
cargo install cargo-fuzz
- uses: actions/setup-python@v5
with:
python-version: '3.8'
Copy link
Collaborator Author

@tisonkun tisonkun Apr 19, 2024

Choose a reason for hiding this comment

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

@davidhewitt @messense

We are trying to ship the Python script function with PyO3 primarily here. And since statically link to libpython can be complicated, I'm investigating the way to dynamically link to libpython.

So far, it works well and we can see the CI all passed. But this requires a libpython3.8.so can be linked in the system.

Thus, I have two questions:

  1. How can I make this binary work well with all the libpython >= 3.8?
  2. How can I disable the related functionalities if libpython3.so is not found?

This refers to:

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

cc @discord9 any thoughts here?

Copy link
Contributor

Choose a reason for hiding this comment

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

cc @discord9 any thoughts here?

Maybe we can use bindgen to create a shared library(call it libpython_finder maybe?) with the same symbols as limited python abi, and forward those calls to actual libpython3.* at runtime with libloading, we would also need to export extra functions to check if libpython is found, and config pyo3 to use our shared library, the shared library would not need much effect to mantain after we are done developing it as limited api doesn't change that much.

@killme2008 killme2008 requested a review from discord9 April 22, 2024 09:23
Copy link
Contributor

@discord9 discord9 left a comment

Choose a reason for hiding this comment

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

Rest LGTM

"backend" => {
let value = py_str_to_string(&kw.node.value)?;
let value = py_str_to_string(&kw.value)?;
match value.as_str() {
// although this is default option to use RustPython for interpreter
// but that could change in the future
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we change this comment?

@@ -73,6 +58,8 @@ pub(crate) fn arrow_rtruediv(arr: &dyn Datum, val: &dyn Datum) -> Result<ArrayRe
}

/// Performs `val / arr`, but cast to i64.
/// TODO: figure out whether pyo3 backend can support this
#[allow(dead_code)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Pyo3 support rich cmp, see more in this link, and the interface is quite similiar:
https://pyo3.rs/v0.14.5/class/protocols.html?highlight=compare#comparison-operators
Edit: maybe change this todo to TODO(discord9): impl SequenceProtocol and rich cmp?

@tisonkun tisonkun mentioned this pull request Apr 23, 2024
3 tasks
@tisonkun
Copy link
Collaborator Author

Closed as undetermined yet. This patch can be picked up since it touches only the Python script code path.

Before we make the decision to lift PyO3 as the default backend and distribute it in every artifact, we should resolve the issue described in #3777, namely how to support multiple Python versions, or at least disable when Python environment not found, instead of panic.

@tisonkun tisonkun closed this Apr 23, 2024
@davidhewitt
Copy link

Sorry for the late reply, I am somewhat behind on OSS so please expect some delay from me at the moment.

Thus, I have two questions:

  1. How can I make this binary work well with all the libpython >= 3.8?
  2. How can I disable the related functionalities if libpython3.so is not found?

In principle you can link to libpython3.so of any version of 3.8 or higher using the abi3-py38 feature. But my previous experience is that distribution packagers are frequently omitting this file - e.g. PyO3/pyo3#2901 (comment)

Probably you are safer to avoid depending on the system Python and distribute the Python version you want to link with. This means either you:

  • statically link (the easiest way I know to do this is with PyOxidizer)
  • dynamically link and ship the linked libpython along with your binary. I believe you can use a rpath to ensure that the linked libpython is preferred for loading (but this may affect your other dependencies too).

@tisonkun
Copy link
Collaborator Author

@davidhewitt Thanks a lot! According to your comment, here are my two questions:

libpython3.so of any version of 3.8 or higher using the abi3-py38 feature

How does it work? We currently add the abi3-py37 feature, but it still failed to load shared lib as in https://github.com/GreptimeTeam/greptimedb/actions/runs/8751166135/job/24016840142:

./bins/greptime: error while loading shared libraries: libpython3.8.so.1.0: cannot open shared object file: No such file or directory

You can see the commits above to fight with this issue.

dynamically link and ship the linked libpython along with your binary. I believe you can use a rpath to ensure that the linked libpython is preferred for loading (but this may affect your other dependencies too).

It still has two challenges:

  1. Is it possible to opt-out such Python scripting support on missing dylib? Currently, it panics on binary starts up.
  2. Either static linking or dynamic linking with bundled dylib, we may meet challenge to ship 3rdparty dependencies like numpy. So it's still desired to leverage the Python installation on user environment. But GreptimeDB wants a way to be compatible with as many as Python versions and gracefully disable the related features if non dylib found.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change This pull request contains breaking changes. docs-not-required This change does not impact docs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrade rustpython
3 participants