search

ethereum / eth-account

Account abstraction library for web3.py
http://eth-account.readthedocs.io/
MIT License
269 stars 156 forks source link

replace first bunch of cytoolz helper usage #216

Closed oberstet closed 7 months ago

oberstet commented 1 year ago

fixes https://github.com/ethereum/eth-account/issues/215

oberstet commented 1 year ago

the CI is currently failing with the following - which seems unrelated to this PR:

## Confirming "jammy" is supported...

+ curl -sLf -o /dev/null 'https://deb.nodesource.com/node_12.x/dists/jammy/Release'

## Your distribution, identified as "jammy", is not currently supported, please contact NodeSource at https://github.com/nodesource/distributions/issues if you think this is incorrect or would like your distribution to be considered for support

grafik

oberstet commented 1 year ago

You can actually import the methods from eth-utils

Yeah, I could, but why more complicated? Using toolz works fine for me on both CPython and PyPy.

I have spent countless hours with the Ethereum foundation Python packages fighting dependency issues.

All the Ethereum foundation Python packages unfortunately specify upper bounds on package dependencies.

Since Pip/Setuptools is unable to cope with the resulting confluent dependencies in the dependency graph of apps, this results in lots of pain and hours wasted in downstream projects.

As soon as your app (or any direct or indirect dependency) depends on 2 deps which both depend on 1 eth package - but insist on different versions of that shared dep (confluence in dependency graph) - you are up for lots of fun;)

And I am talking about apps which use a properly isolated, app-specific venv! For distros shipping Python packages as system-wide distro packages, making this work for users and apps will become simply impossible.

IMO, the Ethereum Python packages, their dependencies policy that is, is severely broken, and I have to use forked versions of the libraries anyways, with upper bounds removed. End of rant. Sorry.

oberstet commented 1 year ago

Yeah, I could, but why more complicated?

just noticed, this library depends on eth-utils anyways already ... with upper bound: means, using it shouldn't create additional problems;)

https://github.com/ethereum/eth-account/blob/c5552a56b349e4c205089f50ddf54e942d4d9bc9/setup.py#L73

are you interested in merging this PR? then I would invest more work into the PR, continue use my fork only for killing the upper bounds ..

kclowes commented 1 year ago

Yeah, I could, but why more complicated? Using toolz works fine for me on both CPython and PyPy.

Cytoolz is more performant so we like to give people the option if possible. And like you said eth-utils is already a dependency, so it shouldn't be more complicated.

All the Ethereum foundation Python packages unfortunately specify upper bounds on package dependencies

We have been exploring removing the upper bounds on our dependencies across our libraries. Web3.py v6b8 has that change in already if you want to test it out. Pinning upper bounds and not pinning upper bounds both have pros and cons. But your feedback has been heard.

are you interested in merging this PR?

Yes. One of us can pick it up in a bit if you don't want to put any more time in but we'd appreciate the contribution. We're focused on getting web3.py v6 stable out now so it will probably take us some time to get to this.

oberstet commented 1 year ago

Cytoolz is more performant

On which Python implementation? Without measuring, I would expect it to be faster on CPython and slower on PyPy. The tracing JIT compiler of PyPy spills out native machine code. PyPy is usually faster with pure Python JIT'ed than a C compiler, unless using vectorized machine code (SIMD). In which case, Numba (also a JIT compiler, but non-tracing .. I think) will do the shootout with the C compilers.

Anyways: if eth-utils does the right job, that is not using cytoolz if not available or when running on PyPy, and falls back to toolz, cool!!! Doesn't harm then.

Pinning upper bounds and not pinning upper bounds both have pros and cons. But your feedback has been heard.

Thanks a lot! Yeah, there are different opinions. FWIW, my opinion (which is what I try to follow wherever I can):

but we'd appreciate the contribution

ok, I'll get back to this hopefully tomorrow and do the changes as indicated by you.

if you have a fix for the Node version thing, I'd be glad to incorporate! as it is more fun to have proper "fully green" PR in the end;)

pacrob commented 1 year ago

if you have a fix for the Node version thing, I'd be glad to incorporate! as it is more fun to have proper "fully green" PR in the end;)

Node concerns should be handled between #217 and #223. Thanks for your help!