-
Notifications
You must be signed in to change notification settings - Fork 1
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
chore: add basic Dockerfile #270
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #270 +/- ##
==========================================
+ Coverage 85.14% 85.17% +0.03%
==========================================
Files 62 63 +1
Lines 8924 9073 +149
==========================================
+ Hits 7598 7728 +130
- Misses 1326 1345 +19 ☔ View full report in Codecov by Sentry. |
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 is not totally clear to me what is the goal of this container.
If it is to provide a functional image for the end-user, the location is ok, if it is more for development and benchmarking, I think putting the file elsewhere or renaming it to benchmark.dockerfile
or something like that is better.
Dockerfile
Outdated
RUN apt-get update && apt-get install -y git | ||
|
||
# Clone repositories (honeycomb + inputs) | ||
RUN git clone https://github.com/LIHPC-Computational-Geometry/honeycomb . |
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 have mixed feelings about this. If you want to create a container to bench the current branch you are one, it will not do.
Plus, it will not work if you are not connected to github .
I think accessing or copying the current repository into the builder makes more sense. You can use "COPY" or "RUN --mount" (see https://docs.docker.com/reference/dockerfile/#run---mount).
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 switched to binaries install due to issues with generated images
Dockerfile
Outdated
|
||
# Clone repositories (honeycomb + inputs) | ||
RUN git clone https://github.com/LIHPC-Computational-Geometry/honeycomb . | ||
RUN git clone https://github.com/imrn99/meshing-samples |
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.
For this one, you can use ADD
to download and decompress the tarball from github. This way git is not required anymore.
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 seems like ADD timeouts on git links from my trials
Dockerfile
Outdated
# Install performance tools | ||
RUN apt-get update && apt-get install -y \ | ||
linux-tools-generic \ | ||
heaptrack |
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.
Does not having the source code available a problem here?
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.
see global comment
I updated the file to be closer to how fastiron's one worked. I think you're right, it will be easier to generate some binaries + mount the repo for the rest of the files. the perf install didn't work though, I think there is a version incompatibility. If that's ok I will fix it later, when we need actual profiling instead of exec times. |
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 still think that a more precise name like benchmarks.dockerfile
makes more sense.
oh ok, I thought this point was related to the original content. I renamed the file |
Description
I used fastiron's file as reference: https://github.com/cea-hpc/fastiron/blob/main/Dockerfile
I adjusted it to:
TODO: test it