sbellem commented 4 years ago

Tl;dr: Without write access to Docker Hub, the code that is tested will not be the code from the PR. Write access to Docker Hub is reserved to PRs made from a branch under initc3/HoneyBadgerMPC.

Please note that encrypted environment variables are not available for pull requests from forks.

-- https://docs.travis-ci.com/user/encryption-keys/

UPDATE: This issue was originally opened to address the problem of running Travis CI on one's own fork as this may be very useful for multiple reasons. But there is also the problem of being able to accept pull requests from forks. The two problems are somewhat related because in both cases Travis CI cannot access the encrypted credentials and therefore cannot login to Docker Hub and therefore cannot push built images which in the end results in Travis CI not being able to actually test the actual code of the PR, unless some changes are made.

Currently the two jobs under the Test stage, Sphinx Tests and Unit Tests:

depend on a docker image that is pulled from Docker Hub. That image is expected to have been pushed to Docker Hub during the previous Build stage.

Pull requests that are from forks or branches that are from forks cannot push to Docker Hub during the Build stage and consequently the image that is pulled from Docker Hub in the Test stage is from a previous merged PR not from the current PR.

Problem & Possible Solutions

It may be worthwhile to review the motivations behind the current situation. As far as I know the main motivation was to speed the build phase on Travis CI, and also for local development.

Problem: Travis CI builds take too long. Solution: Use Docker Hub as a caching system.

The current approach in place works well as long as PRs are from initc3/HoneyBadgerMPC. This leads to a new problem:

Problem: PRs must be from initc3/HoneyBadgerMPC, and more generally, Travis CI can only be used for branches under initc3/HoneyBadgerMPC. This limits contributions to people who have push access to initc3/HoneyBadgerMPC.

Solution 1: Limit external PRs to specific files

That is, limit external PRs to Python code modifications and/or files that do not require to be compiled/built. Mount those modifiable files into the container to actually test against these files. Detect changes to "non-modifiable" files using git diff and TRAVIS_COMMIT_RANGE. This requires maintaining a list of files that most not be modified. If an external PR modifies such files the PR should fail.

PROs

Quick to implement (at least that is the current guess)

CONs

Requires maintaining a list of non-modifiable files to check against for external PRs.
Prone to errors as it is easy to imagine the "black list" being out-of-date.
Prevents external contributors from making contributions to other parts of the code or would require a different process in place, in which a member of initc3/HoneyBadgerMPC would somehow need to bring in the changes.

Solution 2: Use separate repositories

Move the code for the base layer into one (or more) separate repository and version that layer such that at any point in time one working on the HoneyBadgerMPC Python code will work against a versioned "base". Modifications to that "base" would need to be done in that dedicated base repository.

PROs

Allows everyone to contribute on all parts of the code.
Invites for a refinement of the modularization of the current code base. For instance, perhaps pairing could be in its repo, and the apps as well.
Invites for having a base docker image that has the most basic dependencies such as rust, flint, ntl, pbc and charm (i.e.: line 25-130 of the Dockerfile).

CONs

Implementation effort more time-consuming (perhaps).
May complicate the development workflow for those needing to modify various layers of the system.

Some Details to Keep in Mind

It seems to me that it may be very useful to discern between dependencies or prerequisites that do not depend at all on the actual code found under the HoneyBadgerMPC repository. Flint and NTL are examples of such dependencies. On the other hand, building pairing or apps/asynchromix/cpp depend on the actual code found under HoneyBadgerMPC. This point is mentioned because dependencies that do not rely on the HoneyBadgerMPC code can be pre-built in a totally separate context outside the HoneyBadgerMPC context.

Separation of Concerns

The current setup addresses various concerns, such as image size, build speed, development environment, and testing on Travis CI. It may be useful, to think of each concern separately and to see what kind of solution is best. This thought exercise should not prevent to have an integrated solution in the end if that is what is best.

Concern 1: Travis CI

Travis CI is used to run tests, build documentation and run code and documentation quality checks. Travis CI results and code reviews are used to help validate pull requests. Once a pull request has passed all checks on Travis Ci and has been positively reviewed it may be merged by someone who has write privileges to the repository.

Concern 2: Managing the Development Lifecycle

One key issue that arises with managing the dev environment has to do with binaries and volume mounts (see #180 for instance).

Another issue, is that one needs to be able to relatively easily make changes to whatever part of the system, be it pure Python, C, C++, or Rust -based optimizations. Moreover, a developer needs to be capable to quickly re-build the system after modifications made to whatever part of the system. With pure Python this is somewhat trivial whereas with code that requires compilation a more substantial toolchain may be required.

Requirements:

The code under test must be the same as the code under review.
Make the builds fast.
Ideally support pull requests made from forks; from external contributors.

Currently, the above 3 are not possible. Builds are always fast but when they are from forks it is at the expense of (1) :fearful: .

Possible Improvements to Dockerfile

[ ] Use the Docker image of rustlang for nightly builds: https://hub.docker.com/r/rustlang/rust/.
[ ] To build pairing use a pre-built image with a working version of pyo3. In order to do this look into setting up a docker image on dockerhub that will include pyo3 and will get built automatically when the nightly rust changes, but if the the overall build with pyo3 fails then the image on dockerhub should remain with a previously working nightly rust. In other words the nightly rust version in use should work with pyo3 and pairing. HoneyBadgerMPC image would then depend on this "working" build. Doing so, if possible would prevent cases in which building no longer works, and one needs to then manually downgrade the rust nightly version.
[ ] For Travis CI it should not be required to install the project in development mode (with the -e or --editable option when using pip or equivalently using python setup.py develop. Currently if the editable is not used ntl cannot be imported. (ModuleNotFoundError: No module named 'honeybadgermpc.ntl._hbmpc_ntl_helpers' -- note that this problem is "strange" -- todo: provide example.)

Original text for this issue: This happens when Travis CI runs the build on a branch of a fork. To work on this issue requires clarifying whether the current build process can actually be executed on a fork. Having a build process that is "reproducible" on forks may be useful and desirable as it can allow others to execute the builds/tests under their own chosen CI environment.

Example (https://travis-ci.org/sbellem/HoneyBadgerMPC/jobs/606265339#L177):

Setting environment variables from .travis.yml
$ export 9ak80ojFZdSe1ZHfJlG7cQQs9q8KTh4Y558YbkciXtw=[secure]
We were unable to parse one of your secure environment variables.
Please make sure to escape special characters such as ' ' (white space) and $ (dollar symbol) with \ (backslash) .
For example, thi$isanexample would be typed as thi\$isanexample. See https://docs.travis-ci.com/user/encryption-keys.

Resources

sanket1729 commented 4 years ago

Strong favor for Approach 2. I think it is much neater and can save lots of development time in the future even though it may be costly for now.

I don't think we have lots of external contributors for now, so it is best to do this for now, so maybe we can invest time in doing it

The detecting changes seems to be hacky fix, and can result in errors

sbellem commented 4 years ago

Strong favor for Approach 2. I think it is much neater and can save lots of development time in the future even though it may be costly for now.

I don't think we have lots of external contributors for now, so it is best to do this for now, so maybe we can invest time in doing it

The detecting changes seems to be hacky fix, and can result in errors

Thanks for the feedback @sanket1729 .

amiller commented 4 years ago

I think I agree having a separate deps repo is a pretty flexible and simple way to do this.

initc3 / HoneyBadgerMPC

Running Travis CI jobs on forks & PRs from forks #387