-
Notifications
You must be signed in to change notification settings - Fork 75
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 Dockerfile #59
Add Dockerfile #59
Conversation
@lacava Can you try to build a docker image on your machine, following an instruction I added to README? |
Hi @lacava Thank you |
sorry for the delay and thanks for the reminder. I have to better understand how to integrate the docker container with our continuous integration setup before merging it. I'll try to do that by end of next week. |
no updates but I haven't forgotten, just wrapped up. thanks for your patience |
@yoshitomo-matsubara - what do you think about replacing the lines:
with this instead?
This way it will just copy the current version of the repository (assuming you build the image inside the root directory) and any uncommitted files rather than checking out the master branch. So this would let you use the docker image for testing and CI. |
One other suggestion: use |
I pushed a new commit with |
@@ -0,0 +1,49 @@ | |||
FROM mambaorg/micromamba:0.19.1 | |||
|
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.
Need to include:
USER root
here (to get micromamba to work).
vim \ | ||
build-essential \ | ||
jq \ | ||
python3.7-dev && \ |
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.
python3.7-dev
is unavailable on this image. I think micromamba should have most things already, but if not, the following packages may do it:
zlib1g-dev \
libedit-dev \
libncurses5-dev \
libssl-dev \
bzip2 \
libbz2-dev \
libreadline-dev \
RUN mkdir -p /usr/share/man/man1 | ||
|
||
# Install base packages. | ||
RUN apt-get update --fix-missing && apt-get install -y \ |
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.
Should be apt update
and apt install
. Also, the --fix-missing
doesn't seem to work with this image so I would delete it.
# variables a newer one. | ||
ENV NVIDIA_VISIBLE_DEVICES all | ||
ENV NVIDIA_DRIVER_CAPABILITIES compute,utility | ||
LABEL com.nvidia.volumes.needed="nvidia_driver" |
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 don't have a GPU so I can't verify these steps work, so just double check on your end!
rm -rf /var/lib/apt/lists/* | ||
|
||
# Install Python packages. | ||
RUN pip install --upgrade pip |
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.
Should be deleted as the srbench
env will have its own version of pip which installs in the correct location.
COPY . . | ||
|
||
RUN micromamba update micromamba -y | ||
RUN bash configure.sh |
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.
configure.sh
uses conda
instead of micromamba
so this will not work. Maybe paste the commands of configure.sh
in the dockerfile, and swap with micromamba
?
Alternatively, you could set up micromamba
as an alias to conda
so all other commands will work? (e.g., in the various install scripts).
@MilesCranmer I think all the new points are specifically for micromamba and I am not a micromamba user. I confirmed on my end that the initial Dockerfile works, but cannot guarantee that the big changes with micromamba still doesn't change the behavior. Also, it's been several months since the last time I have touched the code of srbench, merging this PR first and customizing it on your side would be more efficient. |
Sounds good to me! |
@MilesCranmer Thank you, could you merge this PR or tell the person in charge to do so? |
@MilesCranmer micromamba sounds interesting, let's open a separate issue and PR to discuss using it. |
Then, what do you do with this PR ? |
@yoshitomo-matsubara we will review this PR and merge it, don't worry. My bandwidth has been extremely limited since I've moved jobs and cities. I've just invited a number of contributors to be maintainers of this repo (@foolnotion, @marcovirgolin , @folivetti , etc), so if any of them would like to work on this PR they are welcome. I'm hesitant to add the docker container without having an element of CI that is checking /using the docker image. Otherwise there is no one to maintain it once it is merged, and it is going to need to be maintained. I can't use docker on the compute cluster that I'm using to run the experiments, and because I won't be using it, the tests are extra important. @yoshitomo-matsubara are you willing/able to integrate the docker build into test.yml? Some of the steps @MilesCranmer mentioned appear relevant. Otherwise you'll have to wait for me or someone else to do it. |
You can build docker containers with GitHub actions using https://github.com/docker/build-push-action/. That should let you test whether this Dockerfile builds, and also try to run the test suite inside the container as part of another GitHub action. I would recommend having a separate GitHub workflow (e.g., |
my PR yoshitomo-matsubara#1 was supposed to fix the remaining issues with this dockerfile. I suppose that now it's a little outdated but the changes are small and they could even be manually picked and merged into the current branch. |
One additional comment: This docker container (and actually, ALL attempts to build the conda environment
The reason for this is because these packages are not built for
If I select |
@foolnotion Sorry, I didn't notice your PR to my forked repo for a while. I didn't have any issue on my machine with the original docker file and environment.yaml. Maybe there were some minor updates that crashed the dependencies. @MilesCranmer I think that the original one worked on Ubuntu 18LTS with |
moved to PR #75 |
This PR will resolve #56 and #57