Refactor dependency handling #548

cburgdorf · 2018-04-11T10:47:44Z

What was wrong?

If one follows the current development instructions, one is left with a system that doesn't have all needed dependencies installed (See #538 )

How was it fixed?

This PR is based on #546 because it also updates the documentation which I couldn't get to work without #546.

Basically this PR follows the pattern that @carver already uses for eth-account. Maybe I implemented it with a tiny twist but it seemed most logical to me.

The gist of it goes like this.

requirements-dev.txt and requirements-doc.txt are no more
in setup.py there's a dictionary of dependencies with dependencies grouped as evm, p2p, trinity, tests, docs and dev.
the dev group automatically includes all others which means there is nothing else to run apart from pip install -e .[dev] to get all dependencies that are needed to run, develop, test and release anything related to this repository.

Let me finish with my usual disclaimer that I'm still quite new to Python and have no idea what I'm doing so please be nice in case this turns out as utter nonsense. 😅

Cute Animal Picture

pipermerriam · 2018-04-11T13:42:29Z

docs/contributing.rst

-You can run the tests with:
+A great way to explore the code base is to run the tests.
+
+We can run all tests with:


Probably worth an admonition here that naively running the full test suite is likely to take multiple hours to run.

Added:

However, running the entire test suite does take a very long time so often we just want to run a subset instead, like:

pipermerriam · 2018-04-11T13:44:07Z

setup.py

        "aiohttp>=2.3.1,<3.0.0",
        "async_lru>=0.1.0,<1.0.0",
+        "coincurve>=7.0.0,<8.0.0",


this should not be part of the default py-evm install. It requires a system dependency and we are working to avoid as many system dependencies for the base library.

Ah, so that explains the existing grouping that I couldn't make sense of 👍

However, I think I haven't fully grokked that yet.

It requires a system dependency and we are working to avoid as many system dependencies for the base library

So, if I get you right you want that the install_requires (the ones taken into account for anyone running pip install py-evm) dependencies to not include coincurve, eth-hash and plyvel but still want these to be included for everyone running a pip install -e .[dev]. Is that correct?

Do you mind elaborating a bit on that? I understand that we want to avoid system dependencies for the base library but if not installing these dependencies means that the evm doesn't work how would that help?

but still want these to be included for everyone running a pip install -e .[dev]. Is that correct?

Yes.

The reasoning for having pip install py-evm install an absolute minimum number of requirements (and absolute minimal system requirements) is to make the base library very easy to install and use as a library with minimal assumptions about underlying things like what crypto implementations they want to use, what database backend they want to use, etc.

if not installing these dependencies means that the evm doesn't work how would that help?

This is not the case. eth-keys ships with a pure python backend for the ecc operations. coincurve is just faster. Same with plyvel. py-evm doesn't care about what database backend it uses and it does just fine with a minimal MemoryDB.

So while trinity doesn't work (or is slower) without these dependencies, the base library py-evm does.

Ah py-evm has existing fallbacks for these libraries in case these are missing. That clears it up! 👍

pipermerriam · 2018-04-11T13:44:23Z

setup.py

        "cryptography>=2.0.3,<3.0.0",
        "cytoolz>=0.9.0,<1.0.0",
        "eth-bloom>=1.0.0,<2.0.0",
+        "eth-hash[pycryptodome]",


same, should not be part of the base py-evm install.

Moved it into new evm-extras section (see longer comment below)

pipermerriam · 2018-04-11T13:44:37Z

setup.py

        "eth-utils>=1.0.1,<2.0.0",
+        "plyvel==1.0.4",


same, should not be part of the base py-evm install.

Moved it into new evm-extras section (see longer comment below)

pipermerriam · 2018-04-11T13:45:33Z

setup.py

+        "pytest-logging>=2015.11.4",
+        "pytest-xdist==1.18.1",
+        "pytest-watch>=4.1.0,<5",
+        "tox==2.7.0",


This should probably be part of the dev requirements as the test requirements would be what tox installs.

Yep, moved it to dev. 👍

pipermerriam · 2018-04-11T13:46:27Z

setup.py

+    ],
+    'test': [
+        "flake8==3.5.0",
+        "mypy>=0.580",


this should probably be under a separate lint section so that in tox we can reduce the installation size for the linting and mypy runs

We can do that but considering that we cache all packages between runs anyway I wouldn't expect it to have a big impact on CI performance.

Less about performance and more about isolation. Not a big deal but it matches the convention we've used elsewhere and I think I like the distinction.

pipermerriam · 2018-04-11T13:46:31Z

setup.py

+            "web3>=4.0.0b11,<5.0.0",
+    ],
+    'test': [
+        "flake8==3.5.0",


this should probably be under a separate lint section so that in tox we can reduce the installation size for the linting and mypy runs

Yep, it's now separated and the linting jobs also only require lint as extras.

pipermerriam · 2018-04-11T13:47:53Z

tox.ini

-    p2p: p2p
-deps = -r{toxinidir}/requirements-dev.txt
+
+deps = .[dev]


Yous can just change this to extras=dev which is the slightly more semantically correct way to do this.

What was wrong with the extras as the were? They had a clean isolation of which dependencies to install for which tests.

In the ideal setup, we shouldn't need to install the "developer" extra inside the test suite at all: https://github.com/ethereum/eth-account/blob/master/tox.ini#L32-L35

Specifically:

extras = + test database: leveldb trinity: trinity, p2p rpc: trinity, p2p p2p: p2p -deps = -r{toxinidir}/requirements-dev.txt

Where the test extra includes any backends that are needed by core libraries, I'm guessing: coincurve, pycryptodome for eth-hash, and plyvel

One quirk that I never found an answer for: I'd like to specify eth-hash as a primary dependency in a library like py-evm and then specify an extra, such that py-evm[test] depends on eth-hash[pycryptodome]. The first few ways I've tried that didn't work as I expected, so I was stuck doing it as an explicit dependency outside the test extra, like:

https://github.com/ethereum/eth-utils/blob/03eabede3d55e2d00686d2b14bda0cb2148c63af/tox.ini#L14-L16

deps = eth-hash[pycryptodome] extras = test

This is a tradeoff that seems like the better option, but does have a downside:

Explicit eth-hash[pycryptodome] in tox.ini

Bad: We have to specify the backend of choice twice: once in the tox file and once in setup.py. Changing the preferred backend means changing it twice (or more realistically: changing it once by accident, and hunting down a confusing bug from two slightly different environments)

Good: We get to specify which major versions of pycryptodome that eth-hash supports inside eth-hash, so any changes in supported versions auto-propagate to all libraries that use eth-hash

Specify pycryptodome version explicitly in py-evm[test]

Bad: if we need to change the supported versions of pycryptodome (say, they make a non-semver-compliant release, and we have to explicitly exclude some minor version), then all libraries that depend on eth-hash will be broken until we go tweak the lib.

Good: the backend is only specified once in py-evm so we eliminate the class of bugs from the first approach

Since a breakage in the first one only affects this one library (py-evm), and the second one affects all libraries depending on eth-hash, the first one seems the clear winner. (unless we have a strong reason to believe that one kind of breakage is orders of magnitude less likely than the other)

@pipermerriam now that I refactored the CI runs to not depend on the dev dependencies anymore the trinity tests are missing ipython. Is trinity supposed to have a hard dependency against ipython by default (e.g. putting it into the trinity section directly) or should that go into something like trinity-extras (somewhat reasonable I believe since one can also run trinity with --vanilla-shell)?

I remember seeing a conversation that concluded that we should be opinionated and require ipython for trinity. (since the shell is part of the "UI" of trinity, and it gives a better shell experience)

Also, yay for uncovering missing dependencies with the more isolated tox setup! 🎉

I believe anyone who runs pip install py-evm does expect this to install evm, p2p and trinity fully to the point that no further manual installs are needed. At least for now that p2p and trinity are part of this package.

Fair enough. This mostly just makes me eager to split trinity out into its own repo for real (but we're not there right now).

So, if I understand you correctly you are saying that if you specify eth-hash as install_requires you can not overwrite it in an extra with eth-hash[pycryptodome]?

That was my experience, but I don't know for sure. It was one of those things that didn't just work and I didn't have time to investigate if it was travis or tox or pip.

As soon as you don't have deps anymore the extras didn't seem to have an effect.

That's super-weird and feels broken. Heck, that might have even been the cause of the previous problem. If that's trivially reproducible, that sounds like a fairly straightforward thing to file a bug against tox for.

That's super-weird and feels broken

Yep, in fact it really felt broken because it sometimes seemed to work for one run and then failed for another whereas .deps[x,y,z] worked reliably every single time.

Heck, that might have even been the cause of the previous problem

Yes, that might be it.

If that's trivially reproducible, that sounds like a fairly straightforward thing to file a bug against tox for

I can investigate that further another time 😬

Fixes ethereum#538

This is to not waste more computational CI resources on linting than necessary. Fixes ethereum#551

cburgdorf · 2018-04-12T08:00:48Z

Alright, I just updated this again and made iPython a base dependency of trinity. I also added one more commit to this PR and combined flake8 and mypy jobs to a common lint job as suggested in #551

cburgdorf · 2018-04-12T08:21:06Z

Looks like this reduced our build time by about 7 minutes 🎉

pipermerriam approved these changes Apr 11, 2018

View reviewed changes

cburgdorf force-pushed the christoph/fix/install_setup branch from 2f3a5cc to 10b542d Compare April 11, 2018 20:00

cburgdorf added 3 commits April 12, 2018 09:50

Refactor dependency management

4957671

Fixes ethereum#538

Update contributing instructions

0c2deda

Combine flake8 and mypy into common linting job

2f912f4

This is to not waste more computational CI resources on linting than necessary. Fixes ethereum#551

cburgdorf force-pushed the christoph/fix/install_setup branch from 10b542d to 2f912f4 Compare April 12, 2018 07:51

carver approved these changes Apr 12, 2018

View reviewed changes

pipermerriam approved these changes Apr 16, 2018

View reviewed changes

pipermerriam merged commit 7f1b00e into ethereum:master Apr 16, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor dependency handling #548

Refactor dependency handling #548

cburgdorf commented Apr 11, 2018 •

edited

Loading

pipermerriam Apr 11, 2018

cburgdorf Apr 11, 2018

cburgdorf Apr 11, 2018

pipermerriam Apr 11, 2018

cburgdorf Apr 11, 2018

pipermerriam Apr 11, 2018

pipermerriam Apr 11, 2018 •

edited

Loading

cburgdorf Apr 11, 2018

pipermerriam Apr 11, 2018

cburgdorf Apr 11, 2018

pipermerriam Apr 11, 2018

cburgdorf Apr 11, 2018

pipermerriam Apr 11, 2018

cburgdorf Apr 11, 2018

cburgdorf Apr 11, 2018

pipermerriam Apr 11, 2018

cburgdorf Apr 11, 2018

pipermerriam Apr 11, 2018

pipermerriam Apr 11, 2018

cburgdorf Apr 11, 2018

pipermerriam Apr 11, 2018

cburgdorf Apr 11, 2018

carver Apr 11, 2018

carver Apr 11, 2018 •

edited

Loading

carver Apr 11, 2018 •

edited

Loading

cburgdorf Apr 11, 2018

carver Apr 11, 2018

carver Apr 11, 2018

carver Apr 12, 2018

cburgdorf Apr 12, 2018

cburgdorf commented Apr 12, 2018

cburgdorf commented Apr 12, 2018

Refactor dependency handling #548

Refactor dependency handling #548

Conversation

cburgdorf commented Apr 11, 2018 • edited Loading

What was wrong?

How was it fixed?

Cute Animal Picture

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pipermerriam Apr 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

carver Apr 11, 2018 • edited Loading

Choose a reason for hiding this comment

carver Apr 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cburgdorf commented Apr 12, 2018

cburgdorf commented Apr 12, 2018

cburgdorf commented Apr 11, 2018 •

edited

Loading

pipermerriam Apr 11, 2018 •

edited

Loading

carver Apr 11, 2018 •

edited

Loading

carver Apr 11, 2018 •

edited

Loading