-
Notifications
You must be signed in to change notification settings - Fork 401
Update Dockerfile and its README #1098
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
base: main
Are you sure you want to change the base?
Conversation
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
@Bevacizumab Thank you for this contribution! It looks like Google's bots don't have a record of an agreement to the contributor's license agreement (CLA), so please follow the instructions in the CLA bot comment earlier. (It's simple and doesn't require giving away any rights.) |
@mhucka Thank you for checking this pull request. I completed the CLA last week, right after I saw the bot's comment. I've just confirmed that the CLA status is green in the "Checks" tab. It seems you might have been viewing an older notification without refreshing it. Please let me know if you have any feedback on the code changes themselves. Thank you! |
Oh, great. Thanks for doing that.
Yes, I will review it. |
@Bevacizumab Thanks again for your work. There is a problem, but it may be easy to fix. The Anaconda terms of service requires a license for business organizations. Specifically, under section 1.b of https://www.anaconda.com/legal/terms/terms-of-service (dated 2025-07-25):
So, OpenFermion can't use make use of conda, miniconda, or the packages from https://repo.anaconda.com/pkgs/. However, we can use conda-forge and miniforge. Would it be possible to rewrite your Dockerfile to use the miniforge3 installer and the conda-forge channel? |
@mhucka Thank you for detailing the issue and introducing me an alternative approach. I wasn't aware of the issue with conda for this project.
I will try to update dockerfile with conda-forge and miniforge. Additionally, i'm planning to pin the versions of the installed five packages ( Due to my current schedule, I may not be able to make the changes immediately, but I expect to have it done within the next few weeks (hopefully sooner). There is a question about the versions of ubuntu and python for this project. I have tentatively used ubuntu 22.04 and python 3.12 because |
Hello @mhucka. Thank you for your patience over the last few weeks. The latest changes have been pushed to this pull request. |
4c28a21
to
f71d433
Compare
Hi @mhucka, I've just rebased my branch on top of the latest master. Sorry for the long wait. |
@Bevacizumab Thank you for your continued efforts on this. I've added some review comments. I will also do a PR to update the |
## Setting up Docker for the first time | ||
|
||
The Dockerfile is based on the [Ubuntu image](https://hub.docker.com/_/ubuntu) (ver. 22.04). | ||
It creates a Python (ver. 3.12) virtual environemnt (named `fermion`) using Miniforge and installs all dependencies within it. Psi4 is installed with a conda [command](https://psicode.org/installs/v191/). |
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 creates a Python (ver. 3.12) virtual environemnt (named `fermion`) using Miniforge and installs all dependencies within it. Psi4 is installed with a conda [command](https://psicode.org/installs/v191/). | |
It creates a Python (ver. 3.12) virtual environment (named `fermion`) using Miniforge and installs all dependencies within it. Psi4 is installed with a conda [command](https://psicode.org/installs/v191/). |
|
||
The Dockerfile is based on the [Ubuntu image](https://hub.docker.com/_/ubuntu) (ver. 22.04). | ||
It creates a Python (ver. 3.12) virtual environemnt (named `fermion`) using Miniforge and installs all dependencies within it. Psi4 is installed with a conda [command](https://psicode.org/installs/v191/). | ||
The default configuration uses the latest Miniforge installer on Linux `aarch64` architecture. |
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.
Why does this change the default from the previous x86_64 architecture? From our experiences, x86_84-based systems are still more common than arm64 systems.
you can open the container with the following option: | ||
``` | ||
docker cp [path to file on disk] [container name]:[path in container] | ||
Command Palette -> Dev Containers: Attach to Running Container.. |
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.
This change refers to a GUI interface, correct? As far as I know, docker
does not provide a GUI. There is a separate tool, Docker Desktop, that provides a GUI, but we can't assume that users will be using that. (Also, Docker Desktop has license restrictions.)
Please revert this back to CLI-based instructions.
FROM ubuntu | ||
FROM ubuntu:22.04 | ||
|
||
USER root |
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.
This was in the original Dockerfile, and not a new addition, but it may as well be fixed as part of this PR. The Dockerfile should not need root, so it should be possible to omit this line entirely. (I'm not sure why the original had it in the first place.) Removing the line will solve the check failure about "USER should not be root".
|
||
RUN apt-get update && \ |
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 DL3009, I will add a configuration setting to .hadolint.yaml
to disable this particular check because it's just not worth the hassle.
For DL3015, go ahead and add the recommended --no-install-recommends
flag.
For DL3008, we should probably pin the versions, although there are pros and cons to doing that. One way to find out the versions that can be used is to do roughly the following:
- switch to the x86_64 architecture as discuss in a previous comment above
- run the docker build as-is
- figure out what versions of the packages were installed by the build
- edit the file to add those vesions to the apt install command.
I don't know if it's possible with apt-get install
, but try to see if it will let you specify just major.minor versions, not full major.minor.patch.whatever; i.e., "12.3" instead of "12.3.0.build34" etc.
The original Dockerfile has issues and is not working. Ubuntu and Python versions are not specified, and the link to Psi4 is expired. This pull request fixes the Dockerfile and updates its README.md to match.
The new configuration has been tested on an Apple Silicon Mac (M4). Customization for other platforms is straightforward and can be understood easily with the updated README.md.
Changes to Dockerfile:
WORKDIR
to mount local files.PATH
withENV
.wget
.fermion
).RUN
commands to reduce the number of layers.Changes to its README.md:
docker run
command and introduce a simpler way to mount local files. So some previous explanations are deleted.