Skip to content
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

Merged
merged 5 commits into from
Feb 3, 2025
Merged

chore: add basic Dockerfile #270

merged 5 commits into from
Feb 3, 2025

Conversation

imrn99
Copy link
Collaborator

@imrn99 imrn99 commented Jan 21, 2025

Description

I used fastiron's file as reference: https://github.com/cea-hpc/fastiron/blob/main/Dockerfile

I adjusted it to:

  • automatically clone the repo before build. That way we can restrict space mount to an "out dir"
  • clone the repo I use to store input files
  • remove the path installation since we're working with many binaries/benchmarks
  • use a base with more features for the final image. I want to use the docker in interactive mode, so having a working shell is nice

TODO: test it

@imrn99 imrn99 marked this pull request as ready for review January 21, 2025 15:23
@codecov-commenter
Copy link

codecov-commenter commented Jan 21, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.17%. Comparing base (6e38028) to head (183d06f).
Report is 4 commits behind head on master.

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.
📢 Have feedback on the report? Share it here.

Copy link
Member

@cedricchevalier19 cedricchevalier19 left a 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 .
Copy link
Member

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).

Copy link
Collaborator Author

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
Copy link
Member

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.

Copy link
Collaborator Author

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
Copy link
Member

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

see global comment

@imrn99
Copy link
Collaborator Author

imrn99 commented Jan 29, 2025

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.

Copy link
Member

@cedricchevalier19 cedricchevalier19 left a 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.

@imrn99
Copy link
Collaborator Author

imrn99 commented Feb 3, 2025

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

@imrn99 imrn99 merged commit 180ac7f into master Feb 3, 2025
13 checks passed
@imrn99 imrn99 deleted the add-dockerfile branch February 3, 2025 14:24
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.

3 participants