-
Notifications
You must be signed in to change notification settings - Fork 4.5k
[BEAM-10891] Standardized developer build environment using Docker #13308
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[BEAM-10891] Standardized developer build environment using Docker #13308
Conversation
|
R: @TheNeuralBit |
f66285d to
5814f96
Compare
|
Status: One I'm kinda stuck on how to fix it is this one: Suggestions are welcome. |
5814f96 to
50ced86
Compare
Aha! I ran into this same error just now, its because I had JAVA_HOME set to use a Java 11 JDK. Switching back to Java 8 resolved it. We don't yet support Java 11, can you have the image specify Java 8? |
dev-support/docker/Dockerfile
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can add time to the package installation and will remove the need for this. I did that on my end and it worked!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, already copied that improvement from your version.
|
I just pulled the branch and ran Only 2 tests failed, but I think their cause is known:
I think with that, we are good to go! That script is pretty awesome @nielsbasjes! |
|
I found a very subtle permission problem in the go part. |
|
What I ran into is that if you run |
|
I'm in doubt about something. Right now I put the The reason for that is that if I put it in a directory that is part of the source tree it will cause IntelliJ to see thousands of changed files way too often. If I link it to the real The downside is that if you have multiple copies of the beam code cloned then these will all share this one instance and may very well (untested) cause conflicts. What is the best approach here? |
|
@nielsbasjes I am not familiar with how this works, and would love your thoughts on this: In the status quo (without docker containers), if you have multiple copies of beam repos, wouldn't they all use If so, I do not think it matters which setup you go with. It seems like the former may be cleaner as it separates the Let me know what you think! |
|
I agree with @omarismail94's assessment |
|
In the current version still fails to First this error that "sometimes" occurs: The funny thing is that apparently the code is locked to a commit in etcd that was put in in February 2018, almost 2 years ago. Second building the website consistently fails with this. A container for the image A generic problem I found here is that if you run this multiple time you end up with multiple docker containers running that are not terminated automatically. I'm ignoring this for now. |
|
I ran a test with So far the and My knowledge of Go and Gradle is too limited to have any idea how to fix this. Please help |
|
@omarismail94
|
|
@lostluck, @youngoli any advice for the goVet errors? For the GCP permissions issue, if this is an IT I don't think it should be run as part of |
|
The beam/sdks/java/extensions/ml/build.gradle Lines 47 to 49 in 40f517f
There should be a separate EDIT: I drafted #13444 for this |
|
@TheNeuralBit |
Yeah we can handle that separately. I would like to get to the bottom of the goVet issue in this PR though. Alternatively, if we can confirm this is working for Python and Java development, we could file a follow-up jira to figure out the issues with the Go SDK. (Also FYI there's been some discussion about this effort on the dev@ mailing list) |
|
I created https://issues.apache.org/jira/browse/BEAM-11363 for the IT issue |
|
I just read the dev list and what you wrote. I think committing it as it is now and improve from there is a good idea. |
661605a to
5e6296f
Compare
I'll give it a try @rohdesamuel FYI you can use this to pull in the PR git fetch origin pull/13308/head:BEAM-10891-Development-Docker git checkout BEAM-10891-Development-Docker (assumes origin is set to apache beam git repo) |
Okay
Okay. I gave it a try. And in its current state, IMO I wouldn't use it, it would be too complicated to use compare to what I have now. I was basically just aiming to go through the commands here, run the python tests, run the lint, format steps, etc. And make sure they all work First I couldn't run pyenv, to setup a virutalenv and perform the steps in that guide. Then I would be able to setup a python virtualenv and perform the steps in the wiki Even better, I would like to see this setup a the docker container setup a python virtualenv as well. I think everything can run from within there. I didn't really attempt steps for java though. Also I needed to run sudo to start-build-env.sh. I am wondering if we can have this work without sudo. As I imagine this is giving my docker instance too much permissions sudo ./start-build-env.sh |
Could you just skip the virtualenv and install everything in that guide directly into the container's python install? |
|
I've been experimenting with the container in a clean clone of Beam on this branch. Weirdly it seems that ... until I run I suspect this is a separate issue. If this can be repro'd outside of the dev container, let's file a bug and move on (I'm out of time for today, I'll try it myself tomorrow if no one else does). |
|
@ajamato I did some googling on how to setup pyenv and pyenv-virtualenv and I've added that to the docker image (along with pre downloading the latest patch releases of the python versions Beam needs. Note that my knowledge level around python is very limited so other than just installing these tools I do not know how to proceed. So feedback is appreciated. |
|
Python build now fails with this error (with by the looks of it the same cause) Error log: |
|
@TheNeuralBit I ran the current docker image locally with your changes from #13444 included. Turns out that sdks/java/io/google-cloud-platform also has a few tests that require Google credentials to run. All of these failed also with My current guess is that these should be moved to be an IT test instead of a regular test. Do you want to include the #13444 fixes? |
|
Strange; after a |
|
Honestly I'm not sure it makes sense to include pyenv and pyenv-virtualenv by default. This is yet another level of containerization that confuses things. Once users have started the build environment they could just treat its system python(s) like a virtualenv. We should consider pre-installing the tools from https://cwiki.apache.org/confluence/display/BEAM/Python+Tips for the system python though. For those that want to use pyenv inside the build env container they're free to install it. |
That's probably the right thing to do, but I want to bring it up with the dev list first. I'll use BEAM-11363 to track fixing those tests as well. For now you could try just deleting or |
|
@TheNeuralBit I had a look at the python page you mentioned regarding the tooling on it. So at this point I'm a bit confused what you guys want to have installed and what not regarding the python tools. |
74c9a6d to
3c715c6
Compare
|
@nielsbasjes I think he meant having the following packages:
I see tox as part of the commit files already, but not the latter 2 |
|
@omarismail94 Cool, can you give me a hint what the correct place is to put them in? (I'm a real Python noob) |
TheNeuralBit
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for all your work on this @nielsbasjes, I'm sure this will make life much easier for new contributors. I would really like it if this could successfully run ./gradlew check but it looks like the issues were running into there are independent of this container. I filed BEAM-11402 to address that separately.
Just one last request - I think we should avoid the extra layer of pyenv for the reasons I mentioned above. I can merge once that's addressed.
|
As far as i can tell the current image pre installs the pyenv but it is not activated by default. Please explain what you would like me to change. |
Let's just not install pyenv and its dependencies |
|
🎉 thanks again @nielsbasjes and @omarismail94! |
This adds a docker based build environment intended for developers to have a predictable working environment with all needed tools present.
On a Linux machine (I ONLY tested this on my Ubuntu 20.04 laptop) just run the
start-build-env.shscript to build and start a Docker based build setup that wraps around the current source tree beam copy.This is based upon the initiative started by @omarismail94 #12837 and copies large parts from https://github.com/apache/hadoop
As suggested by @TheNeuralBit in #12837 I was able to run
./gradlew :sdks:python:container:py37:dockerinside this docker setup which actually created a 2.39GB docker imageapache/beam_python3.7_sdk:2.27.0.devTODO:
Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:
R: @username).[BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replaceBEAM-XXXwith the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.CHANGES.mdwith noteworthy changes.See the Contributor Guide for more tips on how to make review process smoother.
Post-Commit Tests Status (on master branch)
Pre-Commit Tests Status (on master branch)
See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.
GitHub Actions Tests Status (on master branch)
See CI.md for more information about GitHub Actions CI.