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.

Sign up for GitHub

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

Jump to bottom

Issue 83: Enable Python bindings for StreamManager and EventWriter. #104

Merged
merged 19 commits into from
Jun 3, 2020
Merged

Issue 83: Enable Python bindings for StreamManager and EventWriter. #104

merged 19 commits into from
Jun 3, 2020

Conversation

shrids
Copy link
Contributor

@shrids shrids commented May 26, 2020 •
edited
Loading

Note: rustc nightly version is required to generate the bindings.

Change log description
Enable Python bindings for the native client. StreamManager and EventWriter apis have been enabled in this Pull request.
Purpose of the change
Fixes #83

What the code does
This PR uses https://github.com/PyO3/PyO3 to generate Python bindings.
https://github.com/PyO3/maturin has been used to generate the python packages and can also be used to upload it https://pypi.org/

This PR does not generate async bindings

How to verify it
This has been verified against Pravega-standalone.

shrids added 9 commits May 22, 2020 19:05
@tkaitchuck tkaitchuck requested review from Tristan1900, tkaitchuck and zwx14700 and removed request for Tristan1900 and tkaitchuck May 26, 2020 23:13
tkaitchuck
tkaitchuck reviewed May 26, 2020
View reviewed changes
bindings/Cargo.toml Outdated Show resolved Hide resolved
bindings/src/python_binding/mod.rs Outdated Show resolved Hide resolved
bindings/src/python_binding/stream_writer.rs Outdated Show resolved Hide resolved
controller-client/src/lib.rs
@@ -229,7 +229,7 @@ pub struct ControllerClientImpl {
channel: RwLock<ControllerServiceClient<Channel>>,
}

fn get_channel(config: &ClientConfig) -> Channel {
async fn get_channel(config: &ClientConfig) -> Channel {
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be async? I don't see any IO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pravega-client-rust/controller-client/src/lib.rs

Line 247 in fa5239a

Channel::balance_list(iterable_endpoints)

this internally spawns a tokio task. This mandates the use of Tokio runtime. Hence marking it under an async block will ensure the user invokes it in a Tokio runtime.

We have a similar issue with

pravega-client-rust/src/event_stream_writer.rs

Lines 60 to 64 in fa5239a

tokio::spawn(Processor::run(processor));
EventStreamWriter {
writer_id: Uuid::new_v4(),
sender: tx,
}

Perhaps we can use the same approach here too.

Copy link
Member

Choose a reason for hiding this comment

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

I created #107 as a way to resolve this.

Copy link
Contributor

@zwx14700 zwx14700 Jun 2, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Does this mean our current creating controller client in the wrong way?

rust-toolchain Show resolved Hide resolved
@tkaitchuck tkaitchuck mentioned this pull request May 29, 2020
Closed
@shrids shrids marked this pull request as ready for review June 1, 2020 02:14
shrids
shrids commented Jun 1, 2020
View reviewed changes
bindings/Cargo.toml Outdated
#javascript_dev = ["wasm-bindgen"]

# python binding is enabled by default
# This causes cargo test to break ref: https://github.com/PyO3/pyo3/issues/341
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ref: https://github.com/PyO3/pyo3/blob/master/guide/src/advanced.md#testing

Copy link
Member

Choose a reason for hiding this comment

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

Do we need it enabled by default? It seems reasonable that you should specify the "feature" of the language you want to link. Perhaps that can act as a flag to enable this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have updaetd the default feature to include pyo3. maturin develop command with enabling a specific feature.

tkaitchuck
tkaitchuck approved these changes Jun 1, 2020
View reviewed changes
Copy link
Member

@tkaitchuck tkaitchuck left a comment

Choose a reason for hiding this comment

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

Change looks good, but the build issues need to be resolved before merging.
Ideally we could do #107 first or right after because it would make this a lot nicer.

bindings/src/lib.rs
// http://www.apache.org/licenses/LICENSE-2.0
//

#[macro_use]
Copy link
Member

Choose a reason for hiding this comment

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

We can probably add a dummy main to keep the test suite happy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

tried with a dummy main, it too fails. We need to use the workaround mentioined @ https://pyo3.rs/master/advanced.html#testing

bindings/Cargo.toml Outdated
#javascript_dev = ["wasm-bindgen"]

# python binding is enabled by default
# This causes cargo test to break ref: https://github.com/PyO3/pyo3/issues/341
Copy link
Member

Choose a reason for hiding this comment

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

Do we need it enabled by default? It seems reasonable that you should specify the "feature" of the language you want to link. Perhaps that can act as a flag to enable this?

rust-toolchain Outdated
@@ -1,2 +1,3 @@
stable
# Change to stable once issue https://github.com/PyO3/pyo3/issues/5 is resolved.
Copy link
Member

Choose a reason for hiding this comment

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

Note, progress is being tracked here:
PyO3/pyo3#210

shrids added 4 commits June 2, 2020 18:00
@shrids
CR Changes.
36f7428 
Signed-off-by: Sandeep <[email protected]>
@shrids
remove redundant main
224a617 
Signed-off-by: Sandeep <[email protected]>
@shrids
fix clippy.
9398fb8 
Signed-off-by: Sandeep <[email protected]>
@shrids
Fix clippy errors.
8c3a680 
Signed-off-by: Sandeep <[email protected]>
Tristan1900
Tristan1900 approved these changes Jun 2, 2020
View reviewed changes
bindings/src/stream_manager.rs Outdated Show resolved Hide resolved
@shrids
Remove comment.
259e4ab 
Signed-off-by: Sandeep <[email protected]>
@Tristan1900 Tristan1900 merged commit b10c15a into master Jun 3, 2020
@Tristan1900 Tristan1900 deleted the python-binding-poc-2 branch June 3, 2020 18:56
@tkaitchuck tkaitchuck mentioned this pull request Jun 9, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zwx14700 zwx14700 zwx14700 approved these changes

@Tristan1900 Tristan1900 Tristan1900 approved these changes

@tkaitchuck tkaitchuck tkaitchuck approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Python bindings
4 participants
@shrids @tkaitchuck @Tristan1900 @zwx14700