Make rusty-rlp optional #127
Conversation
tox.ini
Outdated
| @@ -14,10 +15,13 @@ usedevelop=True | |||
| commands= | |||
| core: py.test {posargs:tests/} | |||
| doctest: py.test --doctest-glob='docs/*.rst' --doctest-modules docs rlp | |||
| core-fallback: pip uninstall -y rusty-rlp | |||
There was a problem hiding this comment.
That's probably not the supposed way of handling it but it works. Naturally I first tried pip uninstall -y rusty-rlp && py.test {posargs:tests/} but tox doesn't allow that. Then I noticed that the uninstall is already enough because (for some reason) it still derives the py.test {posargs:tests/} from core so it does what it is supposed to. I'm very open to learn about cleaner alternatives :D
There was a problem hiding this comment.
Yeah, the prefixes match on substrings, so the core: py.test {posargs:tests/} line still matches core-fallback environments. I'm not clear on how ordering works, if it's even stable. My read of the config file is that pytest would run before the pip uninstall, but that's based on intuition, not a careful reading of the tox.ini spec.
There was a problem hiding this comment.
Ah that explains it. Regarding ordering. I don't know either but it appeared to work (I tested it with intentional making it fail and ultimately also with the log message that I introduced). I could also try to make it fail if the log message isn't in the output so that we can be sure it fails loudly if there actually is an issue with that approach.
cburgdorf
force-pushed
the
christoph/feat/fallback
branch
3 times, most recently
from
55d5fe6
to
fb00f1e
Compare
| logger.warning( | ||
| "Rust backend not supported. Falling back to slow RLP implementation. " | ||
| "If this is a 64-bit system, you may have a 32-bit Python installed." | ||
| ) |
There was a problem hiding this comment.
The warning can be observed by running tox -e py38-core-fallback -- --log-cli-level WARNING
I think it is especially useful to avoid situations where people are on a 64-bit system but have installed a 32-bit Python.
setup.py
Outdated
| @@ -53,7 +55,7 @@ | |||
| setup_requires=['setuptools-markdown'], | |||
| install_requires=[ | |||
| "eth-utils>=1.0.2,<2", | |||
| "rusty-rlp>=0.1.15, <0.2", | |||
| "rusty-rlp>=0.1.15, <0.2" if is_64_bit else "" | |||
There was a problem hiding this comment.
My spider sense says that logic like this doesn't work at installation time in certain contexts. For example, I think that when installing with a wheel this file may not be evaluated?!??? but I'm really not sure about this.
There was a problem hiding this comment.
I'm not sure I understand. You mean the fact of having the ternary expression at this particular place or evaluating the is_64_bit in general? If the former, then would moving the install_requires out of the setup function change that? We certainly build it dynamically in Trinity, too
If you mean the whole evaluation of the is_64_bit would not work then I wouldn't really know how to provide a proper fallback at all.
There was a problem hiding this comment.
@pipermerriam Any update on this? I got two more requests asking for ARM builds (which I can not provide right now, too). The 64 bit check may not be sufficient to exclude these (since there are 64 bit ARM chips) but the principle for how to handle the fallback should be more or less the same.
I'd like to cut a second alpha soon to ensure people installing web3.py and other libs won't get that error and just fallback to the slower implementation.
There was a problem hiding this comment.
I meant relying on having a python expression in the setup.py file be reliably evaluated at installation time. I think that it is possible that for wheels, it may be evaluated at the time the wheel is created, but not during installation... but I'm not sure about this.
There was a problem hiding this comment.
Avoids arbitrary code execution for installation. (Avoids setup.py)
Mmh..sounds like it. Then, maybe the best option would be to not depend on rusty-rlp at all but have pyrlp use it if it is importable. And then Trinity would list it as a dependency separately, hence activate it that way. Web3.py and friends would not use it unless it is explicitly installed by the user.
There was a problem hiding this comment.
@carver @pipermerriam Alright, I made the changes to make this optional. Trinity can add a dependency rlp[rust-backend] to get the performance benefit. Waiting for re-approval. If we merge this today or tomorrow I could potentially still cut a new release this week.
cburgdorf
force-pushed
the
christoph/feat/fallback
branch
from
fb00f1e
to
08b06d4
Compare
cburgdorf
changed the title
cburgdorf
force-pushed
the
christoph/feat/fallback
branch
from
08b06d4
to
593d4ca
Compare
| @@ -28,6 +27,9 @@ | |||
| "ipython", | |||
| "twine", | |||
| ], | |||
| 'rust-backend': [ | |||
| "rusty-rlp>=0.1.15, <0.2" | |||
| ] | |||
There was a problem hiding this comment.
Following the example of eth-hash
No, but with this change and as soon as we cut a new release nobody will end up using the rusty-rlp backend by default. So everyone installing web3.py on whatever system will get the old pyrlp that they used to have. Except for Trinity where we add a dependency to |
|
If no one objects, I will cut version |
|
Lol at the |
What is wrong
We seem to have the following problem. Currently, we do not have 32-bit binaries for
rusty-rlp. The reason for that is that Github Actions does currently not support 32 bit platformsI had several people running into that problem. People will also run into this if they do have a 64 bit machine but installed a 32 bit Python as it was the case here (solved).
How was it fixed
Only use
rusty-rlpwhen it is explicitly installed e.g.rlp[rust-backend]Add Python 3.8 support in CI
Add a job to test functionality with the rust backend