-
Notifications
You must be signed in to change notification settings - Fork 93
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
Should either Dockerize or better specify dependencies #14
Comments
Hi, about the minimal CMake version, the one given in Also, I notice that the CMake provided by And, |
Dependencies are also specified in the |
By the way, the change at 5c85631#diff-1e7de1ae2d059d21e1dd75d5812d5a34b0222cef273b7c3a2af62eb747f9d20aL21-R21 makes the above Dockerfile fail earlier. With that change reverted, the following Dockerfile succeeds: FROM ubuntu:20.04 AS build
ENV DEBIAN_FRONTEND noninteractive
RUN apt-get update \
&& apt-get install --yes \
build-essential \
cmake \
&& apt-get clean \
&& rm --recursive --force \
/var/lib/apt/lists/* \
/tmp/* \
/var/tmp/*
RUN mkdir /src
WORKDIR /src
COPY CMakeLists.txt ./
RUN mkdir --parents build/release \
&& cp CMakeLists.txt build/release/
COPY example ./example
COPY src ./src
COPY util ./util
RUN cmake -DCMAKE_BUILD_TYPE=Release -S . -B build/release \
&& cmake --build build/release --target Demo
FROM python:3.9
RUN pip install pandas sklearn
COPY --from=build /src /src
WORKDIR /src
ENTRYPOINT ["build/release/Demo"] Run via # again...no need to use sudo if running on macOS
sudo docker build --tag midas .
sudo docker run \
--tty \
--rm \
--volume $PWD/data:/src/data \
--volume $PWD/temp:/src/temp \
midas If you want, I can open a PR for that. The one piece I'd want to modify is how I install With a Dockerfile, it would be trivial to set up CI, which would automatically inform you of test failures induced by newly opened PRs. |
I found there's a missing header in the Also I wrote a Can you help test if the |
It can end up being a style preference, but I structured it as a multi-stage build like that so that if you needed to tweak any of the dependencies, you could tweak the C++ deps or the Python deps independently without invalidating the rest of the cached Docker layers. If you prefer to install the C++ and Python deps in the same image, I'd recommend moving the
The build seems to succeed. However, the
Definitely. It's much slower than I'd want for a build that I might need to run semi-regularly. The first improvement is usually re-arranging the steps to keep the slow-to-build but infrequently changing steps (e.g., installing The other thing to do is to utilize a publicly available base image that has the expensive steps already included. A number of ML/data science Python base images are available on Docker Hub that will have common deps like I'll ask around to see if anyone recommends a particular Python analytics-centric base image. |
Thanks for your advice. To be honest, I'm not quite familiar with docker, it would be very helpful if you can open a PR for it. I'll later test it on my Windows machine. About the segmentation fault, I've met this several times but those were because the code tries to read the data file which is failed to be opened. As you might notice, there's almost no defensive code in most source files, but this is on purpose, so other researchers can focus more on the core of the algorithm. |
I'd be happy to open a PR. Will try to do that tomorrow evening or this weekend. (Today was a really long day.) |
I'm running Ubuntu 18.04 and so created the following initial Dockerfile to get around the cmake version requirements that prevent my following the steps listed in the
Demo
section of the README:I then build it via
and run the compile
Demo
app viasudo docker run \ --tty \ --interactive \ --rm \ --volume $PWD/data:/src/data \ midas \ build/release/Demo
which, when shelling out to the Python scripts, aborts with the following
since
pandas
is not available.To better avoid the need for local environment debugging, my personal preference would be for a known-working Dockerfile.
The text was updated successfully, but these errors were encountered: