-
Notifications
You must be signed in to change notification settings - Fork 458
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
Add final dockerfile updates #2139
Add final dockerfile updates #2139
Conversation
@mayankchetan, could you take a quick look through this? |
share/docker/Dockerfile
Outdated
RUN cmake .. ${CMAKE_OPTIONS} | ||
|
||
ARG BUILD_CORES=4 | ||
RUN make -j${BUILD_CORES} FAST.Farm |
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.
Lets leave it at RUN make -j${BUILD_CORES}
, that'll end up building openFAST, FAST.farm (thanks to the compiler flag) and the different drivers.
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.
Specifying FAST.Farm
should only build FAST.farm
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 replaced that line and the line below with just RUN make -j${BUILD_CORES}
and got this error when I built. Should I revert @mayankchetan?
689.3 gfortran: fatal error: Killed signal terminated program f951
689.3 compilation terminated.
689.4 make[2]: *** [modules/openfast-library/CMakeFiles/openfastlib.dir/build.make:75: modules/openfast-library/CMakeFiles/openfastlib.dir/src/FAST_Library.f90.o] Error 1
689.4 make[1]: *** [CMakeFiles/Makefile2:1816: modules/openfast-library/CMakeFiles/openfastlib.dir/all] Error 2
689.4 make[1]: *** Waiting for unfinished jobs....
695.7 [ 98%] Linking Fortran executable openfast
696.4 [ 98%] Built target openfast
1175.7 [ 98%] Building Fortran object glue-codes/fast-farm/CMakeFiles/FAST.Farm.dir/src/FAST_Farm_IO_Params.f90.o
1182.0 [ 98%] Building Fortran object glue-codes/fast-farm/CMakeFiles/FAST.Farm.dir/src/FAST_Farm_IO.f90.o
1186.6 [ 99%] Building Fortran object glue-codes/fast-farm/CMakeFiles/FAST.Farm.dir/src/FAST_Farm_Subs.f90.o
1311.1 [ 99%] Building Fortran object glue-codes/fast-farm/CMakeFiles/FAST.Farm.dir/src/FAST_Farm.f90.o
1314.1 [ 99%] Linking Fortran executable FAST.Farm
1314.6 [ 99%] Built target FAST.Farm
1314.6 make: *** [Makefile:136: all] Error 2
------
Dockerfile:56
--------------------
54 |
55 | ARG BUILD_CORES=4
56 | >>> RUN make -j${BUILD_CORES}
57 |
58 | # Build stage 2: provides built openfast in a small image.
--------------------
ERROR: failed to solve: process "/bin/sh -c make -j${BUILD_CORES}" did not complete successfully: exit code: 2
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.
It may be running out of memory. You may need to just build serially without the -j${BUILD_CORES}
arg.
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.
Thanks, that seems to have fixed that issue but I have a new one:
Dockerfile:60
--------------------
58 | # Build stage 2: provides built openfast in a small image.
59 | FROM ${BASE} as production
60 | >>> COPY --from=build /openfast/install /openfast/install
61 |
62 | ARG TIMEZONE=UTC
--------------------
ERROR: failed to solve: failed to compute cache key: failed to calculate checksum of ref f7ddb83a-5ddc-43e4-be99-233c40f82c60::q28yvsic562t9xxemy83w7yfb: "/openfast/install": not found
I think I'll replace
RUN make -j${BUILD_CORES}
with
RUN make -j${BUILD_CORES} FAST.Farm
RUN make -j${BUILD_CORES} install
which worked for me before. Will that be ok?
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.
Do these two lines not individually build them?
RUN make -j${BUILD_CORES} FAST.Farm
RUN make -j${BUILD_CORES} install
They're preceded by the cmake
line in the dockerfile
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.
make install will result in all the targets getting built, so we could just do RUN make -j${BUILD_CORES} install without specifying any other targets individually.
Ah ok that's what I initially had - I'll revert to that unless @mayankchetan has any comments
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.
The full list of all targets that will be built by install
includes:
aerodyn_driver
aerodyn_inflow_c_binding
beamdyn_driver
FAST.Farm
feam_driver
hydrodyn_c_binding
hydrodyn_driver
ifw_c_binding
inflowwind_driver
moordyn_c_binding
moordyn_driver
openfast
orca_driver
seastate_driver
(will be included in 4.0.0, not in 3.5.3)servodyn_driver
subdyn_driver
turbsim
unsteadyaero_driver
I don't think it is a problem to have all of these included. The *_c_binding
libraries will be of limited use in a docker container, so perhaps we should add cmake option to disable those.
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.
Ok I'll revert to this then:
RUN make -j${BUILD_CORES} install
Are you planning to add the new cmake arguments to this release or the next one? The dockerfile won't need to change to accomodate them as an arbitrary number of cmake args can be passed in as docker build args
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.
Thanks @cortadocodes! I don't think we need to modify the cmake args any further.
@cortadocodes I'm curious about the use of |
Thanks for pointing this out, I just left this option in there from the previous dockerfile so I'm happy to have it turned off by default! I don't think we need double-precision but I'll double check |
@cortadocodes, Can we start publishing the docker image in the GitHub image registry? and then add dockerhub when we get the credentials? |
We can add it as an Asset attached to the release. I might need you or @deslaughter to build and add it after we post. |
Yes if you like although I think we should leave the docker workflows as is so they're ready for the docker hub repository |
Long term the plan would be docker hub. Still waiting on getting that setup through the NREL org. So adding it as an Asset is a short term solution until then. |
You might find it better in an image registry because if you add it as a static asset, I don't think people will be able to But, the GitHub container registry should work absolutely fine for these purposes, and might even circumvent the need for a dockerhub account (sorry I hadn't thought about it before!). For @cortadocodes benefit the existing workflow should be able to push to ghcr.io instead of dockerhub quite directly. |
@thclark, the GitHub container registry is a really interesting idea! I hadn't heard of it before. This could be simpler than going through docker hub. |
GHCR would be fine although I think using a Docker Hub repository in the longer term would give the openfast images more visibility. It's quite good for public images and it looks like NREL already has 35 other repositories on there so everything would be in one place. In my experience docker hub is more searchable and open than GHCR. On the other hand, you wouldn't have to wait for the docker hub account details if you went with GHCR! |
e.g. you have this which I don't think GCHR has an equivalent of (but correct me if I'm wrong!) |
From what I can tell, NREL doesn't have an organization setup on docker hub. Instead it appears maintainers from individual projects simply prepend So all things considered, it may be preferable to go with docker hub. |
I found this but maybe it's not official: https://hub.docker.com/u/nrel |
@cortadocodes, good catch. I was mixing up with PyPi where there is no official organization. I haven't heard back from NREL regarding the Docker org yet. |
Is there anything else outstanding to complete on this PR? If not I propose we merge this, and add an asset docker container to the release (maybe add to the GH containers as well). Sometime later we can update as necessary when NREL's docker hub repository is available. |
@andrew-platt I just need to revert the |
I think we're good to go now |
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 all for the hard work!
This pull request adds the final docker-related changes from the partially-merged
update-dockerfiles
branch (#2124).