Skip to content

Hotfix479 docker#480

Merged
akorosov merged 17 commits intomasterfrom
hotfix479-docker
Oct 13, 2020
Merged

Hotfix479 docker#480
akorosov merged 17 commits intomasterfrom
hotfix479-docker

Conversation

@akorosov
Copy link
Copy Markdown
Member

@akorosov akorosov commented Oct 8, 2020

Closes #479

  • Use a new image (nansencenter/nextsim_base_dev:latest or nansencenter/nextsim_base_pro:latest)
  • Keep compiled model inside image

@akorosov akorosov requested a review from tdcwilliams October 8, 2020 14:01
@einola
Copy link
Copy Markdown
Member

einola commented Oct 9, 2020

Doesn't quite work for me

Monaco:nextsim einola$ docker build . -t nextsim
Sending build context to Docker daemon 186.5MB
Step 1/18 : ARG BASE_IMAGE=nansencenter/nextsim_base_dev:latest
Step 2/18 : FROM $BASE_IMAGE
manifest for nansencenter/nextsim_base_dev:latest not found: manifest unknown: manifest unknown

@akorosov
Copy link
Copy Markdown
Member Author

akorosov commented Oct 9, 2020

Opps, forgot to push the latest tag. Now I pushed both 0.0.1 and latest. Can you try again?

Copy link
Copy Markdown
Contributor

@tdcwilliams tdcwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might need to be careful about libraries and things in /opt overriding things in /nextsim when trying to develop the code.

Is it necessary to do the ldconfig thing?
Perhaps using LD_LIBRARY_PATH instead? eg
LD_LIBRARY_PATH=/nextsim/lib:$LD_LIBRARY_PATH

  • in a similar way to what you do with PATH and /nextsim/model/bin

Maybe the make docker target could also then be removed?

Comment thread Dockerfile
@akorosov
Copy link
Copy Markdown
Member Author

akorosov commented Oct 9, 2020

We might need to be careful about libraries and things in /opt overriding things in /nextsim when trying to develop the code.

Is it necessary to do the ldconfig thing?
Perhaps using LD_LIBRARY_PATH instead? eg
LD_LIBRARY_PATH=/nextsim/lib:$LD_LIBRARY_PATH

  • in a similar way to what you do with PATH and /nextsim/model/bin

Perhaps. I'll try to implement and see how it impacts.

Maybe the make docker target could also then be removed?

No, make docker is quite handy here and when you only need to (re)compile the model (excluding contrib).

@akorosov
Copy link
Copy Markdown
Member Author

akorosov commented Oct 9, 2020

@tdcwilliams , I've updated Dockerfile. Now it compiles nextsim and keeps both the library and the exec inside /nextsim. LD_LIBRARY_PATH and PATH point to /nextsim.
If the image is run with /nextsim mounted, then it the inplace lib and exec will be used.

I've also simplified makefiles to always use -std=c++14 after consulting Einar.

I've tested and the model run both from inside the image and from compiled inplace.

Can you have another look?

@akorosov
Copy link
Copy Markdown
Member Author

akorosov commented Oct 9, 2020

And the latest base images are on docker hub. With the production one named base_nextsim_prod :)

akorosov and others added 3 commits October 9, 2020 16:20
dependency - wasn't working possibly due to interference with mapx,bamg
in both /opt and /nextsim/lib
Copy link
Copy Markdown
Contributor

@tdcwilliams tdcwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @akorosov - I was trying out removing all the mapx and bamg stuff since they had been problematic in the past. I built the nextsim-tools image using the nextsim image made here and could import bamg and mapx ok without these steps.

I also gave make core its proper dependency of contrib, allowing the make docker and make model targets to be removed.
You could add back make docker (with a better name?) with explicit steps to avoid recompiling contrib but in practice one usually does one full compilation and then cd model && make -j8 to avoid recompiling everything else, so I am of the opinion it is unnecessary repetition. It originally arose because @einola couldn't compile with docker due to problems with contrib (possibly some conflicts with /opt) even though it compiled fine on ubuntu docker. It still compiles fine on ubuntu, but maybe @einola would like to double check it compiles ok for him this time round.

So it's OK by me now, but I recommend others double-check and do the approval. Anyway, great job on the overhaul!

Comment thread Dockerfile Outdated
WORKDIR $NEXTSIMDIR
RUN make -j8

# copy mapx
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clarifying comment

Suggested change
# copy mapx
# copy mapx to be available for nextsim-tools

Comment thread Dockerfile Outdated
# copy bamg source, compile and copy libs and include into
# /opt/local/bamg/lib and /opt/local/bamg/include
COPY contrib/bamg $NEXTSIMDIR/contrib/bamg
# copy bamg
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clarifying comment

Suggested change
# copy bamg
# copy bamg to be available for nextsim-tools

@einola
Copy link
Copy Markdown
Member

einola commented Oct 12, 2020

I don't think I like the latest "compile inside image only" version :|
Reason being that now if I change a file I have to recompile everything - right? Also, the version.hpp file is now missing the version information:

/bin/sh -x version.sh
+ git describe --dirty --always --tags
fatal: not a git repository (or any of the parent directories): .git
+ version=
+ date
+ date=Mon Oct 12 12:22:06 UTC 2020
+ git rev-parse HEAD
fatal: not a git repository (or any of the parent directories): .git
+ commit=
+ git rev-parse --abbrev-ref HEAD
fatal: not a git repository (or any of the parent directories): .git
+ branch=
+ which mpicc
+ cc_path=/usr/bin/mpicc
+ mpicc -dumpversion

@akorosov
Copy link
Copy Markdown
Member Author

@einola , it is now possible to compile the model in-place:

docker run --rm -it -v /home/user/nextsim:/nextsim -w /nextsim/model nextsim make

where -w sets the working directory inside the container.

I've checked on both ubuntu and mack host machines. Can you check on yours. Don't forget to clean your working directory with mrproper before compiling in-place.

@akorosov akorosov merged commit 86bb712 into master Oct 13, 2020
@akorosov akorosov deleted the hotfix479-docker branch October 13, 2020 07:20
@einola
Copy link
Copy Markdown
Member

einola commented Oct 13, 2020

Works fine on my machine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Optimize docker images structure

3 participants