Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Implement Python wrapper for parity-clib and example #10443

Closed
wants to merge 3 commits into from

Conversation

mjkoo
Copy link

@mjkoo mjkoo commented Mar 2, 2019

Follows jni example, uses pyo3 to build an extension module exposing the low-level API and includes a Python module which exports a more pythonic interface.

Ref: #9339 and #8604

Interested in also tackling #9338 (Golang example) if this is well-received.

@parity-cla-bot
Copy link

It looks like @mjkoo signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

@jam10o-new jam10o-new added the A0-pleasereview 🤓 Pull request needs code review. label Mar 2, 2019
@soc1c soc1c modified the milestones: 2.6, 2.5 Mar 4, 2019
@soc1c soc1c added the M5-dependencies 🖇 Dependencies. label Mar 4, 2019
@niklasad1 niklasad1 self-requested a review March 4, 2019 09:16
@@ -14,6 +14,7 @@ futures = "0.1.6"
jni = { version = "0.11", optional = true }
panic_hook = { path = "../util/panic-hook" }
parity-ethereum = { path = "../", default-features = false }
pyo3 = { version = "0.6.0-alpha.4", optional = true, features = ["extension-module"] }
Copy link
Collaborator

Choose a reason for hiding this comment

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

pyo3 will require the nightly toolchain and currently we only use stable features

"""
cb = _CallbackGenerator()
self.rpc_query_cb(query, cb, timeout_ms)
return next(cb)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will call the iterator of the Callback class?

Copy link
Author

Choose a reason for hiding this comment

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

of the _CallbackGenerator class, yes

Copy link
Collaborator

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

The code looks very good and clean thank you. Added some minor comments/questions that needs to be addressed.

However, because we don't allow unstable features we need to wait until version 0.6 is available, PyO3/pyo3#210.

The latest nightly don't work on parity due to ICE so I couldn't test it

@niklasad1 niklasad1 added A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. and removed A0-pleasereview 🤓 Pull request needs code review. labels Mar 11, 2019
@mjkoo
Copy link
Author

mjkoo commented Mar 13, 2019

@niklasad1 thank you for review, partially addressed comments, had some follow up questions asked above.

Also experienced ICE when compiling with latest nightly, had to revert to nightly-2019-02-07 which is what I've been testing with.

Ack re: pyo3 and stable features, especially since this is triggering bug with current nightly toolchain. Will follow stabilization efforts over there, seems fairly close.

@niklasad1
Copy link
Collaborator

FYI, the ICE is fixed now rust-lang/rust#58634

@soc1c soc1c modified the milestones: 2.5, 2.6 Apr 2, 2019
@ordian ordian modified the milestones: 2.6, 2.7 Jul 12, 2019
@debris
Copy link
Collaborator

debris commented Jul 29, 2019

I would love to get it in. What is the status of this pr? @niklasad1 do you still have some suggestions regarding this pr?

@debris
Copy link
Collaborator

debris commented Jul 29, 2019

I would love to get it in. What is the status of this pr? @niklasad1 do you still have some suggestions regarding this pr?

@niklasad1
Copy link
Collaborator

@debris it is currently blocked on PyO3/pyo3#210 but I agree would be nice to get it in. We shall maybe look how hard it to fix that issue IMHO.

@niklasad1
Copy link
Collaborator

niklasad1 commented Jan 7, 2020

We have removed the parity-clib, thus closing this one.

@niklasad1 niklasad1 closed this Jan 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. M5-dependencies 🖇 Dependencies.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants