feat: add PyO3(Hence CPython as a Optional Backend #976

discord9 · 2023-02-13T02:41:03Z

I hereby agree to the terms of the GreptimeDB CLA

What's changed and what's your intention?

Add PyO3 as a Optional Backend
Pros: C API(numpy etc.), faster python execute speed
Cons: GIL might have concurrent problem, i.e. too many parallel python thread may slow script execution down

Please explain IN DETAIL what the changes are in this PR and why they are needed:

Refactor scripts into three folder: ffi_types, rspython and pyo3, where ffi_types store common type defination(Yes those types can be used both in RustPython and PyO3) and other two store respective impl details

Major Changes:

Added cpython PyVector impls & cpython coprocessor impls
Added cpython greptime python modules i.e. import greptime as gt
Paired tests for both RustPython and CPython
Refactor of old RustPython codes, now backend-independ code is in ffi_types folder with RustPython detailed impls in rspython folder and cpython relevant methods in cpython folder
Summarize your change (mandatory)
How does this PR work? Need a brief introduction for the changed logic (optional)
Describe clearly one logical change and avoid lazy messages (optional)
Describe any limitations of the current code (optional)
TODO: Refactor(move into greptime module)&Unit tests in PyO3 for PyDataFrame&SQL query in python&More allow_thread so that it release GIL as much as possible for maximizing parallelism

Checklist

I have written the necessary rustdoc comments.
I have added the necessary unit tests and integration tests.(TODO: PyDataFrame&SQL Query's Unit tests

Refer to a related PR or issue link (optional)

github-actions

license-eye has totally checked 752 files.

Valid	Invalid	Ignored	Fixed
583	5	164	0

Click to see the invalid file list

src/script/src/ffi_types.rs
src/script/src/pyo3.rs
src/script/src/pyo3/vector_impl.rs
src/script/src/rspython.rs
src/script/src/rspython/tests.rs

src/script/src/ffi_types.rs

src/script/src/pyo3.rs

src/script/src/pyo3/vector_impl.rs

src/script/src/rspython.rs

src/script/src/rspython/tests.rs

github-actions

license-eye has totally checked 758 files.

Valid	Invalid	Ignored	Fixed
587	7	164	0

Click to see the invalid file list

src/script/src/ffi_types.rs
src/script/src/ffi_types/vector/tests.rs
src/script/src/pyo3.rs
src/script/src/pyo3/corp_impl.rs
src/script/src/pyo3/vector_impl.rs
src/script/src/rspython.rs
src/script/src/rspython/tests.rs

src/script/src/ffi_types/vector/tests.rs

src/script/src/pyo3/corp_impl.rs

github-actions

license-eye has totally checked 773 files.

Valid	Invalid	Ignored	Fixed
593	8	172	0

Click to see the invalid file list

src/script/src/ffi_types.rs
src/script/src/ffi_types/vector/tests.rs
src/script/src/pyo3.rs
src/script/src/pyo3/copr_impl.rs
src/script/src/pyo3/vector_impl.rs
src/script/src/rspython.rs
src/script/src/rspython/copr_impl.rs
src/script/src/rspython/tests.rs

src/script/src/pyo3/copr_impl.rs

src/script/src/rspython/copr_impl.rs

github-actions

license-eye has totally checked 792 files.

Valid	Invalid	Ignored	Fixed
606	8	178	0

Click to see the invalid file list

src/script/src/python/ffi_types/testcases_builtins.rs
src/script/src/python/ffi_types/utils.rs
src/script/src/python/pyo3/builtins.rs
src/script/src/python/pyo3/copr_impl.rs
src/script/src/python/pyo3/utils.rs
src/script/src/python/pyo3/vector_impl.rs
src/script/src/python/rspython.rs
src/script/src/python/rspython/tests.rs

src/script/src/python/ffi_types/testcases_builtins.rs

src/script/src/python/ffi_types/utils.rs

src/script/src/python/pyo3/builtins.rs

src/script/src/python/pyo3/copr_impl.rs

src/script/src/python/pyo3/utils.rs

src/script/src/python/pyo3/vector_impl.rs

src/script/src/python/rspython.rs

src/script/src/python/rspython/tests.rs

sunng87 · 2023-02-20T03:29:19Z

As we have two backends, I suggest to add feature gate for the optional one and ci to check correctness for both.

killme2008 · 2023-02-20T06:12:21Z

@discord9 A lot of conflicts, please fix them.

github-actions

license-eye has totally checked 795 files.

Valid	Invalid	Ignored	Fixed
615	2	178	0

Click to see the invalid file list

src/script/src/python/ffi_types/pair_tests/sample_testcases.rs
src/script/src/python/pyo3/dataframe_impl.rs

src/script/src/python/ffi_types/pair_tests/sample_testcases.rs

src/script/src/python/pyo3/dataframe_impl.rs

codecov · 2023-02-20T11:28:03Z

Codecov Report

Merging #976 (12fce34) into develop (50d2685) will decrease coverage by 0.39%.
The diff coverage is 69.79%.

@@             Coverage Diff             @@
##           develop     #976      +/-   ##
===========================================
- Coverage    85.33%   84.95%   -0.39%     
===========================================
  Files          459      471      +12     
  Lines        66885    68513    +1628     
===========================================
+ Hits         57078    58202    +1124     
- Misses        9807    10311     +504

killme2008 · 2023-02-22T07:11:00Z

Test failure on mac:

---- python::ffi_types::pair_tests::pyo3_rspy_test_in_pairs stdout ----
thread 'python::ffi_types::pair_tests::pyo3_rspy_test_in_pairs' panicked at '(RsPy)code:
from greptime import *
ret = tan(values)
ret
Real: PyVector { vector: PrimitiveVector { array: PrimitiveArray<Float64>
[
  1.557407724654902,
  -2.185039863261519,
  -0.1425465430742778,
] } }!=Expected: PrimitiveVector { array: PrimitiveArray<Float64>
[
  1.5574077246549023,
  -2.185039863261519,
  -0.1425465430742778,
] }', src/script/src/python/ffi_types/pair_tests.rs:81:13
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace


failures:
    python::ffi_types::pair_tests::pyo3_rspy_test_in_pairs

Reply: fixed, used epsilon for float array comparsion

killme2008

This PR is huge. Plesase let me take a break.

src/script/src/python/engine.rs

src/script/src/python/ffi_types/copr.rs

src/script/Cargo.toml

src/script/src/python/ffi_types/copr.rs

src/script/src/python/pyo3/utils.rs

src/script/src/python/pyo3/vector_impl.rs

waynexia

As we gain the ability to interact with cpython via pyo3, I'm wondering if it's possible to directly forward array compute functions to Arrow's python (which is from C or CPP) native implementation? This may reduce lots of FFI calls and make our code slimmer.

src/script/src/python/ffi_types/vector.rs

discord9 · 2023-02-23T13:59:39Z

As we gain the ability to interact with cpython via pyo3, I'm wondering if it's possible to directly forward array compute functions to Arrow's python (which is from C or CPP) native implementation? This may reduce lots of FFI calls and make our code slimmer.

Wouldn't that complex our dependecy management? Like have to maintance both cargo and python pip or conda to get python arrow?

killme2008 · 2023-02-27T06:55:04Z

@discord9 There is a conflict file.

killme2008

Almost LGTM. But we stil have to refactor this crate, make it more clear and clean.

src/script/src/python/rspython/testcases.ron

waynexia · 2023-02-27T07:11:16Z

Almost LGTM. But we stil have to refactor this crate, make it more clear and clean.

One thing I'm thinking of while reviewing is, the ffi_types folder contains the major structs' definition and its methods of two backends. Do you think it's necessary to also split those implementations into different places like pyo3 and rspython?

discord9 · 2023-02-28T02:39:36Z

Almost LGTM. But we stil have to refactor this crate, make it more clear and clean.

One thing I'm thinking of while reviewing is, the ffi_types folder contains the major structs' definition and its methods of two backends. Do you think it's necessary to also split those implementations into different places like pyo3 and rspython?

Some types&function are way too similar to put in same file(i.e. dataframe&builtins functions), which are basically the same expect error handling and some impl details, so at least them should be put into different folder I guess?

src/script/Cargo.toml

waynexia

Give my thumb up to this brilliant work 👍

* refactor: ffi_types * style: fmt * refactor: use `String` for return when possible * todo: vector_impl * feat: pyobj_try_typed_val * refactor: more backend indep function * feat: +-*/ magic methods * refactor: copr * style: fmt * feat: add paired tests * refactor: more * refactor: move inside `python` folder * refactor: all but test code * feat: builtins for PyO3 * chore: add licenses * chore: remove unused&add todos * refactor: remove old files * chore: mark unused * chore: fmt * chore: license * feat: query in PyO3 * test: paired testcases for rspy&pyo3 * feat: PyDataFrame(Untested) * feat: some allow_threads * style: fmt * style: add license * feat: rebase manually of GreptimeTeam#962 * feat: more `allow_threads` * chore: typo * chore: remove some `TODO` * test: allow margin of epsilon * chore: code review advices * chore: more CR adjust * chore: more adjust * feat: kwargs&its test * chore: remove some `dbg!` * chore: allow params * fix: put `dataframe` into scope * chore: newline * fix: adjust after rebase * fix: test serde skip attr * style: taplo * feat: add `pyo3_backend` feature * doc: update CI&readme

github-actions bot reviewed Feb 13, 2023

View reviewed changes

src/script/src/ffi_types/vector/tests.rs Outdated Show resolved Hide resolved

src/script/src/pyo3/corp_impl.rs Outdated Show resolved Hide resolved

github-actions bot reviewed Feb 14, 2023

View reviewed changes

src/script/src/pyo3/copr_impl.rs Outdated Show resolved Hide resolved

src/script/src/rspython/copr_impl.rs Outdated Show resolved Hide resolved

github-actions bot reviewed Feb 15, 2023

View reviewed changes

discord9 force-pushed the pyo3 branch from 2cce5ca to 05a4ddf Compare February 15, 2023 06:13

discord9 marked this pull request as ready for review February 17, 2023 09:19

killme2008 mentioned this pull request Feb 18, 2023

Create PyDataFrame from SQL #990

Closed

v0y4g3r requested a review from killme2008 February 20, 2023 02:40

discord9 force-pushed the pyo3 branch from 701af69 to 3f5d2ad Compare February 20, 2023 07:24

github-actions bot reviewed Feb 20, 2023

View reviewed changes

src/script/src/python/ffi_types/pair_tests/sample_testcases.rs Show resolved Hide resolved

src/script/src/python/pyo3/dataframe_impl.rs Outdated Show resolved Hide resolved

discord9 force-pushed the pyo3 branch from a11bf71 to 71374bf Compare February 21, 2023 07:40

killme2008 reviewed Feb 22, 2023

View reviewed changes

waynexia reviewed Feb 22, 2023

View reviewed changes

src/script/src/python/ffi_types/vector.rs Outdated Show resolved Hide resolved

src/script/src/python/ffi_types/vector.rs Outdated Show resolved Hide resolved

src/script/src/python/ffi_types/vector.rs Outdated Show resolved Hide resolved

killme2008 approved these changes Feb 27, 2023

View reviewed changes

src/script/src/python/rspython/testcases.ron Outdated Show resolved Hide resolved

killme2008 added the Size: XL label Feb 27, 2023

discord9 added 8 commits February 28, 2023 10:13

refactor: ffi_types

d1d2cbd

style: fmt

0c26c2c

refactor: use String for return when possible

939784f

todo: vector_impl

6fbde79

feat: pyobj_try_typed_val

7e963e3

refactor: more backend indep function

b845069

feat: +-*/ magic methods

c687836

refactor: copr

40d6b2e

discord9 added 17 commits February 28, 2023 10:15

feat: some allow_threads

17b9074

style: fmt

aafb986

style: add license

7b91752

feat: rebase manually of #962

71789e4

feat: more allow_threads

2637af9

chore: typo

22f1330

chore: remove some TODO

eb68399

test: allow margin of epsilon

7c5ece0

chore: code review advices

6d13207

chore: more CR adjust

d44e91c

chore: more adjust

bfa70e3

feat: kwargs&its test

346ef01

chore: remove some dbg!

c241f74

chore: allow params

ec08667

fix: put dataframe into scope

1ba24d4

chore: newline

53de3e9

fix: adjust after rebase

8ff86f7

discord9 force-pushed the pyo3 branch from 0394b71 to 8ff86f7 Compare February 28, 2023 02:35

fix: test serde skip attr

08b1532

discord9 force-pushed the pyo3 branch from 5099fc6 to 08b1532 Compare February 28, 2023 03:08

style: taplo

cbc04d9

waynexia reviewed Feb 28, 2023

View reviewed changes

src/script/Cargo.toml Outdated Show resolved Hide resolved

feat: add pyo3_backend feature

863376f

discord9 force-pushed the pyo3 branch from ab3e1b6 to 863376f Compare February 28, 2023 09:36

doc: update CI&readme

12fce34

waynexia approved these changes Feb 28, 2023

View reviewed changes

killme2008 merged commit c5c6494 into develop Mar 1, 2023

killme2008 deleted the pyo3 branch March 1, 2023 02:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add PyO3(Hence CPython as a Optional Backend #976

feat: add PyO3(Hence CPython as a Optional Backend #976

discord9 commented Feb 13, 2023 •

edited

Loading

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

github-actions bot left a comment

sunng87 commented Feb 20, 2023

killme2008 commented Feb 20, 2023

github-actions bot left a comment

codecov bot commented Feb 20, 2023 •

edited

Loading

killme2008 commented Feb 22, 2023 •

edited by discord9

Loading

killme2008 left a comment

waynexia left a comment

discord9 commented Feb 23, 2023

killme2008 commented Feb 27, 2023

killme2008 left a comment

waynexia commented Feb 27, 2023

discord9 commented Feb 28, 2023

waynexia left a comment

feat: add PyO3(Hence CPython as a Optional Backend #976

feat: add PyO3(Hence CPython as a Optional Backend #976

Conversation

discord9 commented Feb 13, 2023 • edited Loading

What's changed and what's your intention?

Major Changes:

Checklist

Refer to a related PR or issue link (optional)

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

github-actions bot left a comment

Choose a reason for hiding this comment

sunng87 commented Feb 20, 2023

killme2008 commented Feb 20, 2023

github-actions bot left a comment

Choose a reason for hiding this comment

codecov bot commented Feb 20, 2023 • edited Loading

Codecov Report

killme2008 commented Feb 22, 2023 • edited by discord9 Loading

killme2008 left a comment

Choose a reason for hiding this comment

waynexia left a comment

Choose a reason for hiding this comment

discord9 commented Feb 23, 2023

killme2008 commented Feb 27, 2023

killme2008 left a comment

Choose a reason for hiding this comment

waynexia commented Feb 27, 2023

discord9 commented Feb 28, 2023

waynexia left a comment

Choose a reason for hiding this comment

discord9 commented Feb 13, 2023 •

edited

Loading

codecov bot commented Feb 20, 2023 •

edited

Loading

killme2008 commented Feb 22, 2023 •

edited by discord9

Loading