Move deposit contract back #1127

hwwhww · 2019-05-27T11:07:41Z

Fix #980

Changelogs

Copy the contract and tests code from deposit contract repo ethereum/deposit_contract@fad5c32
The tests of deposit contract require pyspec's minimal_ssz and DepositData.
Update Makefile:
- Add install_deposit_contract_test command to install the testing requirements of deposit_contract
- Add test_deposit_contract command to run the deposit_contract tests
Update CircleCI config:
- Add restore_default_cached_venv and save_default_cached_venv to avoid setting venv_name and reqs_checksum in multiple places.

TODOs

Weird dependency issue
- Currently, commit 3e6f7a2 fails in CI but works when executing make clean && make pyspec && make install_test && make install_deposit_contract_test && make test_deposit_contract on my local side.
- Moreover, it works in CI after I added 0ec03db which I inserted pip3 install -r requirements-testing.txt in test_deposit_contract command.
- It looks like something is wrong when I dealt with the local dependencies+cache between CircleCI jobs. @protolambda do you have any insight on it? How would test_generators deal with it in CI?
Add README
Create aliases for venv_name and reqs_checksum in CiricleCI config?

1. Add `install_deposit_contract_test` command 2. Add `test_deposit_contract` command

djrtwo · 2019-05-30T02:42:35Z

.circleci/config.yml

+      - deposit_contract:
+          requires:
+            - install_env
+            - lint


Why require lint?

To avoid the race condition of venv :'( #1081

I will think more about how to make it in parallel...

Makefile

deposit_contract/contracts/validator_registration.json

deposit_contract/tests/contracts/test_deposit.py

deposit_contract/tests/contracts/utils.py

deposit_contract/tool/compile_deposit_contract.py

djrtwo · 2019-05-31T19:12:56Z

Do we want to get this merged or is there anything else you want to handle in this PR?

protolambda · 2019-05-31T22:30:45Z

@protolambda do you have any insight on it? How would test_generators deal with it in CI?

The CI caching problems may be because of the formatting of the cache string. (two checksums to pop through when finding the cache). It's tempting to just copy over the dependencies from the basic requirements file tot the requirements-testing, instead of importing it from within. Then we can just use the standard single-checksum cache key. If you like we can try it in this PR.

Test-generators don't run in CI. Mainnet tests are big, and writing half a GB worth of yaml is not worth it every push to CI. Instead, state-transition test generation is unified with testing; when testing, it runs in a pytest context, and doesn't output yaml.

hwwhww · 2019-06-01T03:20:39Z

@protolambda

The CI caching problems may be because of the formatting of the cache string. (two checksums to pop through when finding the cache).

Do you mean in reqs_checksum (I just rewrote it a little bit in 6617f89 )? (3 checksums)

reqs_checksum: cache-v1-{{ checksum "test_libs/pyspec/requirements.txt" }}-{{ checksum "test_libs/pyspec/requirements-testing.txt" }}-{{ checksum "deposit_contract/requirements-testing.txt" }}

Sorry I'm not sure why it causes the problem since they are all static txt files, and what do you mean by "just copy over the dependencies from the basic requirements file"? Could you give an example? 😃

And @djrtwo @protolambda
I'd say we can merge this PR first since it's blocking #1129, and then make the CI improvements in other PRs.

protolambda · 2019-06-01T12:51:19Z

@hwwhww I meant the opposite. Simplification.

That is, change the current requirements-testing.txt:
from:

-r requirements.txt
pytest>=3.6,<3.7
../config_helpers

to:

eth-utils>=1.3.0,<2
eth-typing>=2.1.0,<3.0.0
pycryptodome==3.7.3
py_ecc>=1.6.0
typing_inspect==0.4.0
pytest>=3.6,<3.7
../config_helpers

and have one less checksum to worry about, as caching problems seem to be worse than copying over a few dependencies rarely.

reqs_checksum: cache-v1-{{ checksum "test_libs/pyspec/requirements.txt" }}-{{ checksum "test_libs/pyspec/requirements-testing.txt" }}-{{ checksum "deposit_contract/requirements-testing.txt" }}

If it's a different venv, we should NOT include its checksum in the cache key. We're effectively taking the hash of the version numbers to version the cache of the downloaded dependencies of those version numbers. If the deposit contract requirements are used in a separate venv, then they should be cached separately.

Also, when incrementing a cache version number, it would look like:
v2-pyspec, not v1-pyspec-05 (why two versions?). (circle CI docs here)

Also note that the key is prefix based, so incrementing to v2 forces the cache out. While changing a later part, tries to retrieve a cache keyed with an equal prefix (I think this is the source of our caching problems. Not 100% sure).

protolambda · 2019-06-01T12:55:48Z

.circleci/config.yml

-      - restore_cached_venv:
-          venv_name: v1-pyspec-03
-          reqs_checksum: '{{ checksum "test_libs/pyspec/requirements.txt" }}-{{ checksum "test_libs/pyspec/requirements-testing.txt" }}'
+      - restore_default_cached_venv


Let's not mingle caches of different venvs, there's no "default"

Let's reduce the checksum complexity to 1 hash, and follow the standard

Let's don't use double version tickers in the caching key

Multiple parts split by a - are likely the root of the cache-key problems. See point 2.

Caching is a nightmare sometimes, but we can get this right :)
Marking this PR as blocked for now, caching is not quite ready to merge currently.

hwwhww · 2019-06-03T08:21:02Z

@protolambda

Agreed and disagreed some points. 😄🤓

tl;dr

Making it in single checksum doesn't seem to solve the original issue.
We can consider using Pipenv to manage the packages instead of having -r requirements.txt. (but not in this PR)
Open to discuss if we want deposit contract to have its own venv or not.
Can I just (i) rename restore_default_cached_venv to something else and (ii) remove cache-v1- from reqs_checksum, and then we can merge this PR?

and have one less checksum to worry about, as caching problems seem to be worse than copying over a few dependencies rarely.

But in the current cache key, you already include (i) checksum "test_libs/pyspec/requirements.txt" and (ii) checksum "test_libs/pyspec/requirements-testing.txt". Sorry, could you point out why two checksums of the static files would cause problem? I don’t understand what’s the fundamental difference between your proposal and the current dev branch setting.
I tried to test with the single checksum by simply turning off other jobs: hwwhww@a0e13cd
The issue is still as same as 3e6f7a2: https://circleci.com/gh/hwwhww/eth2.0-specs/191?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link
By the way, I think ../config_helpers itself might cause some problem here since we are not aware of if ../config_helpers were changed.

If it’s a different venv, we should NOT include its checksum in the cache key. We’re effectively taking the hash of the version numbers to version the cache of the downloaded dependencies of those version numbers. If the deposit contract requirements are used in a separate venv, then they should be cached separately.

deposit_contract is sharing the same venv in this PR.

The ideal CI workflow in my mind would be:

* checkout_specs
    * install pyspec env
        * test pyspec
        * install deposit contract env
            * test deposit contract
    * install lint env
        * linter check

But unfortunately, due to the race condition (#1081), all jobs are executed sequentially… :’(

So that’s what we have now:

* checkout_specs
    * install all env (pyspec, linter, deposit contract)
        * test pyspec
            * linter check
                * test deposit contract

If we want to make deposit_contract in another venv, the CI workflow would be like:

* checkout_specs
    * install all env (pyspec, linter)
        * test pyspec
            * linter check
* checkout_specs
    * install pyspec (requirements.txt only)
        * install deposit contract env
            * test deposit contract

^^^^ IMO That would increase some redundant jobs and make CircleCI config more complicated.

Do you feel strongly that it should be in separate venv? I'm in inclined to use single venv unless the race condition issue has been solved. But open to discuss. 😄

Let’s not mingle caches of different venvs, there’s no “default”

Ohh the *_default_* commands are just for making the keywords in one place as we only have one venv. Assume if we keep only one venv. Do you feel okay if I rename it to restore_configurated_cached_venv or restore_main_cached_venv?

Let’s reduce the checksum complexity to 1 hash, and follow the standard

I can see how “just copy over the dependencies from the basic requirements file” make it in one checksum (although I think it doesn't solve the problem above 😄). But anyway, if we want to keep deposit contract in the same venv, there will still be some more dependencies.

Here are some recommendations from CircleCI doc:

https://circleci.com/docs/2.0/caching/#using-a-lock-file:
An alternative is to do ls -laR your-deps-dir > deps_checksum and reference it with {{ checksum “deps_checksum” }}. For example, in Python, to get a more specific cache than the checksum of your requirements.txt file you could install the dependencies within a virtualenv in the project root venv and then do ls -laR venv > python_deps_checksum.
- I found it’s weird that the result includes timestamps, not sure if it can be used in our case, but I can see the point of "dumping the dependencies in one file and calculate the checksum of it".
Or using Pipenv to manage the package. I tried a little bit, it can install the with local dependency. For example:

[[source]]
name = "pypi"
url = "https://pypi.org/simple"
verify_ssl = true

[dev-packages]
pytest = ">=3.6,<3.7"
config_helpers = {editable = true, path = "./../config_helpers"}

[packages]
eth-utils = "<2,>=1.3.0"
eth-typing = "<3.0.0,>=2.1.0"
pycryptodome = "==3.7.3"
py-ecc = ">=1.6.0"

[requires]
python_version = "3.6"

pipenv looks nice and also may be a compromise between requirements.txt files and setup.py! It can easily make single checksum of one package; and if we want to serve multiple packages in one checksum, we can still dump multiple Pipfile.lock files into one text file, and calculate the checksum of it. config_helpers would have its own Pipfile.lock file as well.

Let’s don’t use double version tickers in the caching key

Agreed! 👍

Multiple parts split by a - are likely the root of the cache-key problems.

It looks like as same as your point 2? Also not sure why that’s a problem. Any link? :)

protolambda · 2019-06-03T15:15:10Z

Sorry, could you point out why two checksums of the static files would cause problem

I'm not sure about the issues in CI aside from linting running on the wrong version. But hyphens cause the CI to fall back on keys with the same prefix (up to any hyphen), which may be the reason / co-factor of dependencies of the 2nd checksum not being installed as expected. Circle CI docs here: https://circleci.com/docs/2.0/caching/#restoring-cache, note the "partial cache restore", we should re-run the dependcies install on top of the restored cache contents.

Ohh the default commands ...

The previous CI helper functions where there to make it easy to implement storing + restoring of additional venvs. The generators never made it due to speed & parallellization issues in the CI environment, but we may have others in the future. (contract testing?). Hence the preference to not call anything "default", and just make things explicit.

Or using Pipenv to manage the package

Looks good! The current reason to stick with requirements.txt is that it is much less verbose/complex than setup.py, and easy to checksum. But the generators need to be able to require the pyspec, config_helpers and gen_helpers modules, and other modules may just need a few of them. Hence a setup.py for those modules :(. Changing to just a setup.py never made it because of local editable dependency issues (yes, I dislike it too, but things have to work somehow, and options/time were limited, without breaking encapsulation of modules). A POC with pipenv would be nice 👍

I'm going to try and fix the linting dependency blocking #1077 by moving the linting install out of the makefile, into the testing requirements. (We could make a 3rd requirements file for non-runtime deps, or another bigger change, but really just need to unblock the PR).

djrtwo · 2019-06-05T15:55:02Z

@protolambda @hwwhww I believe one of you was going to make a change to the circleci config before this merged. Is that correct?

hwwhww · 2019-06-07T19:59:40Z

@protolambda @djrtwo Sorry for the delay, I was traveling yesterday.

(Decided to push commit into this branch to get review easier 😅) here is the new config:
96237c7

And the new workflow:
https://circleci.com/workflow-run/1b479336-9951-4d27-bc06-81ecf7c7cd95

Ready for another look.

djrtwo

lgtm

protolambda · 2019-06-08T00:01:26Z

cd $(PY_SPEC_DIR); python3 -m venv venv; . venv/bin/activate;
cd ../..; cd $(DEPOSIT_CONTRACT_DIR); \

Why not just create a separate venv? It's much simpler and cleaner (no caching of separate cache of same venv)

Say, the location would be ./deposit_contract/venv (nicely with the code that needs the venv)

in circle ci config:

venv_path: ./deposit_contract/venv

Then we have a clear consistent reproducible venv locally. And there would be no need to clear and refresh the venv of pyspec when you're working on different things and switching feature branches, as they are separate.

PR feedback + bump eth-tester

hwwhww · 2019-06-08T03:11:17Z

@protolambda

Why not just create a separate venv?

I was thinking about efficiency and the use case of testing deposit contract is rare. But after I created a new venv as you said, Makefile became much simpler so 👍! (PR feedback is applied: 5b8cca8)

Ready for another look. :)

protolambda · 2019-06-08T11:34:17Z

Regarding that last 1-line addition: I declared the new targets (and 1 missing one: lint) as non-file make targets. See https://www.gnu.org/software/make/manual/html_node/Phony-Targets.html

hwwhww added 10 commits May 27, 2019 13:15

copied from deposit contract

09f7114

Update Makefile and CI config

b05444a

1. Add `install_deposit_contract_test` command 2. Add `test_deposit_contract` command

Add restore_default_cached_venv

bff945a

Add deposit_contract/requirements-testing.txt to checksum

18cb674

Add save_default_cached_venv

98cabb6

deposit contract requires eth2spec.phase0.spec.DepositData

05dc4b5

Bump to latest deposit contract dev branch (fad5c32)

30bb898

sequentially execute the jobs

11196ea

kick

3e6f7a2

test

0ec03db

JustinDrake mentioned this pull request May 27, 2019

Deposit contract checklist #1129

Closed

9 tasks

hwwhww added the scope:deposit contract label May 28, 2019

hwwhww added 3 commits May 30, 2019 09:31

Merge branch 'dev' into deposit_contract

22a3bb2

Merge branch 'dev' into deposit_contract

2cd0dee

Sync with ethereum/deposit_contract#43

b280e0a

hwwhww marked this pull request as ready for review May 30, 2019 02:15

djrtwo reviewed May 30, 2019

View reviewed changes

hwwhww added 4 commits May 30, 2019 13:19

Merge branch 'dev' into deposit_contract

12fee0e

Add compile_deposit_contract command

c277703

Use eth2spec.utils.hash_function.hash

0424fab

Add README.md

a81fa54

djrtwo approved these changes May 31, 2019

View reviewed changes

Rewrite reqs_checksum

6617f89

protolambda self-requested a review June 1, 2019 12:51

protolambda suggested changes Jun 1, 2019

View reviewed changes

Remove version from reqs_checksum

b8eddfa

hwwhww added 3 commits June 7, 2019 15:22

Merge branch 'dev' into deposit_contract_venvs

b78f5b2

fix path

cf23a25

two venv caches

96237c7

hwwhww requested a review from protolambda June 7, 2019 19:59

djrtwo approved these changes Jun 7, 2019

View reviewed changes

hwwhww added 2 commits June 7, 2019 22:41

deposit_contract/venv

5b8cca8

PR feedback + bump eth-tester

fix

e52c4a5

improve makefile: declare new targets as non-file

bce6c89

protolambda approved these changes Jun 8, 2019

View reviewed changes

protolambda merged commit 6f82480 into dev Jun 8, 2019

protolambda deleted the deposit_contract branch June 8, 2019 11:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move deposit contract back #1127

Move deposit contract back #1127

hwwhww commented May 27, 2019 •

edited

Loading

djrtwo May 30, 2019

hwwhww May 30, 2019 •

edited

Loading

djrtwo commented May 31, 2019

protolambda commented May 31, 2019

hwwhww commented Jun 1, 2019

protolambda commented Jun 1, 2019

protolambda Jun 1, 2019 •

edited

Loading

hwwhww commented Jun 3, 2019 •

edited

Loading

protolambda commented Jun 3, 2019

djrtwo commented Jun 5, 2019

hwwhww commented Jun 7, 2019

djrtwo left a comment

protolambda commented Jun 8, 2019 •

edited

Loading

hwwhww commented Jun 8, 2019 •

edited

Loading

protolambda commented Jun 8, 2019

Move deposit contract back #1127

Move deposit contract back #1127

Conversation

hwwhww commented May 27, 2019 • edited Loading

Changelogs

TODOs

djrtwo May 30, 2019

Choose a reason for hiding this comment

hwwhww May 30, 2019 • edited Loading

Choose a reason for hiding this comment

djrtwo commented May 31, 2019

protolambda commented May 31, 2019

hwwhww commented Jun 1, 2019

protolambda commented Jun 1, 2019

protolambda Jun 1, 2019 • edited Loading

Choose a reason for hiding this comment

hwwhww commented Jun 3, 2019 • edited Loading

tl;dr

protolambda commented Jun 3, 2019

djrtwo commented Jun 5, 2019

hwwhww commented Jun 7, 2019

djrtwo left a comment

Choose a reason for hiding this comment

protolambda commented Jun 8, 2019 • edited Loading

hwwhww commented Jun 8, 2019 • edited Loading

protolambda commented Jun 8, 2019

hwwhww commented May 27, 2019 •

edited

Loading

hwwhww May 30, 2019 •

edited

Loading

protolambda Jun 1, 2019 •

edited

Loading

hwwhww commented Jun 3, 2019 •

edited

Loading

protolambda commented Jun 8, 2019 •

edited

Loading

hwwhww commented Jun 8, 2019 •

edited

Loading