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

Merge multiple Dockerfiles into a single one #2167

Open
wants to merge 23 commits into
base: staging
Choose a base branch
from

Conversation

SimonYansenZhao
Copy link
Collaborator

Description

This PR merges the following multiple uses of Dockerfile into a single place in tools/docker/Dockerfile for easier maintainance in the future:

Related Issues

None

References

Checklist:

  • I have followed the contribution guidelines and code style for this project.
  • I have added tests covering my contributions.
  • I have updated the documentation accordingly.
  • I have signed the commits, e.g. git commit -s -m "your commit message".
  • This PR is being made to staging branch AND NOT TO main branch.

Signed-off-by: Simon Zhao <[email protected]>
@@ -1,189 +1,177 @@
# syntax=docker/dockerfile:1
Copy link
Collaborator

Choose a reason for hiding this comment

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

What do you think of adding a new set up section in the instructions: https://github.com/recommenders-team/recommenders/blob/main/SETUP.md

We could explain how to install docker in a command line and how to do it with VSCode.

I'm particularly interested in the VSCode one.

What do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure, I will.

@@ -27,24 +29,32 @@
},
"isort.args": ["--profile", "black"],
"python.analysis.autoImportCompletions": true,
"python.defaultInterpreterPath": "/usr/local/conda/envs/Recommenders/bin/python",
// Conda env name *must* align with ENV_HOME in Dockerfle
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think, as Miguel suggested, that it would be good to have the instructions in a SETUP page somewhere as well.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Sure

Signed-off-by: Simon Zhao <[email protected]>
@miguelgfierro
Copy link
Collaborator

hi @SimonYansenZhao the tests should be working now after #2169

Signed-off-by: Simon Zhao <[email protected]>
Signed-off-by: Simon Zhao <[email protected]>
Signed-off-by: Simon Zhao <[email protected]>
Signed-off-by: Simon Zhao <[email protected]>
Signed-off-by: Simon Zhao <[email protected]>
Signed-off-by: Simon Zhao <[email protected]>
Copy link
Collaborator

@miguelgfierro miguelgfierro left a comment

Choose a reason for hiding this comment

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

LGTM

.devcontainer/devcontainer.json Show resolved Hide resolved
Comment on lines +23 to +25
# * [buildpack-deps:24.04](https://github.com/docker-library/buildpack-deps/blob/master/ubuntu/noble/Dockerfile)
# + [Created on 2024-08-17](https://hub.docker.com/layers/library/buildpack-deps/noble/images/sha256-dbfee7e7ee2340b0d6567efd3a8a9281ce45ee78598485b4d7a7f09fe641811a)
FROM buildpack-deps@sha256:dbfee7e7ee2340b0d6567efd3a8a9281ce45ee78598485b4d7a7f09fe641811a AS cpu
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is this? would it be possible to explain what it is and why we are using a specific sha?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This base image is built on top of ubuntu by adding the essential tools for building packages such as gcc. Specifying the sha is for speeding up the image build, otherwise, it will pull the base image every time when there is a new release of the image.

Copy link
Collaborator

Choose a reason for hiding this comment

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

got it, nice!

Signed-off-by: Simon Zhao <[email protected]>
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.

4 participants