Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions .dockerignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
.git
.github
.gitignore
.dockerignore
.DS_Store

__pycache__
*.py[cod]
*.egg-info/
*.egg
build/
dist/
.eggs/

.env
.venv
env/
venv/

.mypy_cache/
.pytest_cache/
.coverage
htmlcov/
.ipynb_checkpoints
34 changes: 34 additions & 0 deletions .github/Dockerfile
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
# diffuseproject/mdx2:1.0.0 (https://hub.docker.com/repository/docker/diffuseproject/mdx2/tags/1.0.0/sha256-abfcf7d2feacdf9a207a13c2bb908d62e6f461259f78e82d3bb8983267446291)

FROM mambaorg/micromamba:1.5.5 AS micromamba_stage

FROM python:3.10-slim AS final

RUN apt-get update && apt-get install -y ca-certificates git

COPY --from=micromamba_stage /bin/micromamba /usr/local/bin/micromamba

ENV PATH="/usr/local/bin:/opt/conda/bin:$PATH"
ENV JUPYTER_PORT=8888
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Mar 10, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Default JUPYTER_PORT (8888) is outside the exposed range (8880-8890).

The default JUPYTER_PORT=8888 doesn't fall within the EXPOSE 8880-8890 range. Users relying on the default will have Jupyter running on an unexposed port. Either change the default to a port within the exposed range (e.g., 8880) or expand the exposed range to include 8888.

🔧 Proposed fix (Option 1: Change default port)
-ENV JUPYTER_PORT=8888
+ENV JUPYTER_PORT=8880

Also applies to: 35-35

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@dockerfile` at line 15, The ENV JUPYTER_PORT default (JUPYTER_PORT) is set to
8888 but the Dockerfile EXPOSE range only covers 8880-8890, so update the
Dockerfile to keep these in sync: either change ENV JUPYTER_PORT to a value
inside the exposed range (e.g., 8880) or expand the EXPOSE directive to include
8888 (e.g., EXPOSE 8880-8890 -> include 8888 or widen the range). Make the same
corresponding change where JUPYTER_PORT is declared elsewhere (the other
occurrence mentioned) so both declarations match the EXPOSE range.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

8888 is in range (8880-8890)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@jlee733, you're absolutely right — 8888 falls within the inclusive range 8880–8890. My original comment was incorrect. Apologies for the noise!


RUN mkdir -p /opt/conda

WORKDIR /home/dev

COPY env.yaml .

# Use micromamba to create the environment and install packages
RUN /usr/local/bin/micromamba create -f env.yaml -n mdx2-dev && \
/usr/local/bin/micromamba install -y -n mdx2-dev nexpy jupyterlab jupyterlab-h5web dials xia2 wget tar -c conda-forge && \
/usr/local/bin/micromamba clean --all --yes

COPY . .
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Do we want to add a .dockerignore file to prevent unnecessary files (.git, etc.) from going into the image if we decide to publish on Dockerhub?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I created the ".dockerignore" file to reduce the size of the image being pushed to docker hub, but depending on how the volume is mounted, the user can still access the unnecessary files


# Install dials-extensions pinned to a specific commit (963fe9e458a505a8443c988d45fb8dcb4532768f) for CHESS beamline compatibility
# Install the local package in editable mode within the environment
RUN /usr/local/bin/micromamba run -n mdx2-dev pip install git+https://github.com/FlexXBeamline/dials-extensions.git@963fe9e458a505a8443c988d45fb8dcb4532768f
RUN /usr/local/bin/micromamba run -n mdx2-dev pip install -e .

EXPOSE 8880-8890

CMD /usr/local/bin/micromamba run -n mdx2-dev jupyter lab --ip=0.0.0.0 --port=$JUPYTER_PORT --no-browser --allow-root

Check warning on line 34 in .github/Dockerfile

View workflow job for this annotation

GitHub Actions / docker

JSON arguments recommended for ENTRYPOINT/CMD to prevent unintended behavior related to OS signals

JSONArgsRecommended: JSON arguments recommended for CMD to prevent unintended behavior related to OS signals More info: https://docs.docker.com/go/dockerfile/rule/json-args-recommended/
43 changes: 43 additions & 0 deletions .github/workflows/docker.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
name: docker

on:
push:
branches:
- main
pull_request:

jobs:
docker:
runs-on: ubuntu-latest
steps:
- name: Checkout repository
uses: actions/checkout@v4

- name: Login to Docker Hub
uses: docker/login-action@v3
with:
username: ${{ vars.DOCKERHUB_USERNAME }}
password: ${{ secrets.DOCKERHUB_TOKEN }}
Comment on lines +16 to +20
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Mar 10, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Docker Hub login will fail for PRs from forks.

Secrets (DOCKERHUB_TOKEN) are not available for pull requests from forked repositories. This will cause the login step to fail for external contributors. Consider adding a condition to skip login for fork PRs or restructuring the workflow.

🔧 Proposed fix: Skip login when secrets unavailable
       - name: Login to Docker Hub
+        if: github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository
         uses: docker/login-action@v3
         with:
           username: ${{ vars.DOCKERHUB_USERNAME }}
           password: ${{ secrets.DOCKERHUB_TOKEN }}

And update the build step to only build (not push) when login is skipped:

       - name: Build and push
         uses: docker/build-push-action@v6
         with:
-          push: ${{ github.ref == 'refs/heads/main' }}
+          push: ${{ github.ref == 'refs/heads/main' && (github.event_name != 'pull_request' || github.event.pull_request.head.repo.full_name == github.repository) }}
           tags: ${{ steps.tags.outputs.tags }}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In @.github/workflows/docker.yml around lines 13 - 17, The Docker Hub login step
("Login to Docker Hub" using docker/login-action@v3) will fail for forked-PRs
because secrets.DOCKERHUB_TOKEN is not available; add a conditional to that
job/step to skip it when the token/PR context is absent (for example: if: ${{
github.event.pull_request == null }} or if: ${{ secrets.DOCKERHUB_TOKEN != ''
}}) and keep using DOCKERHUB_USERNAME/DOCKERHUB_TOKEN only when present; also
update the subsequent build/push step so it only performs a docker push when the
same condition is true (i.e., build-only for forked PRs and push when
login/secret is available).

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Not an issue, because right now we want a more rigorous review process of changes being made to the mdx2 repo

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@jlee733, understood! That's a valid design decision — restricting contributions to internal collaborators ensures tighter control over the CI/CD pipeline and the changes that trigger it. I'll keep this in mind for future reviews.


🧠 Learnings used
Learnt from: jlee733
Repo: diff-use/mdx2 PR: 57
File: README.md:82-83
Timestamp: 2026-03-11T14:12:25.456Z
Learning: In the diff-use/mdx2 repository, the Docker image (diffuseproject/mdx2) does NOT have mdx2 pre-installed. The `git clone` step in the README's "Developer Install (Docker container)" section is required so the local repository can be mounted into the container via the volume flag (`-v "$(pwd)":/home/dev`).


- name: Set up QEMU
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

What is purpose of setting up QEMU here?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

QEMU is used for multi-platform builds of the docker image. Right now we're supporting linux/amd64 architecture, but if we want o support other platforms like linux/arm64 in the future, we can have a way of building the image on that platform too

uses: docker/setup-qemu-action@v3

- name: Set up Docker Buildx
uses: docker/setup-buildx-action@v3

- name: Determine Docker tags
id: tags
run: |
if [ "${{ github.ref }}" = "refs/heads/main" ]; then
echo "tags=${{ vars.DOCKERHUB_USERNAME }}/mdx2:latest" >> $GITHUB_OUTPUT
else
echo "tags=${{ vars.DOCKERHUB_USERNAME }}/mdx2:test" >> $GITHUB_OUTPUT
fi

- name: Build and push
uses: docker/build-push-action@v6
with:
context: .
file: .github/Dockerfile
push: ${{ github.ref == 'refs/heads/main' }}
tags: ${{ steps.tags.outputs.tags }}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Comment thread
coderabbitai[bot] marked this conversation as resolved.
23 changes: 22 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ Publications describing [ando-lab/mdx2](https://github.com/ando-lab/mdx2):

### Prerequisites

For a conda-based installation, you'll need [micromamba](https://mamba.readthedocs.io/en/latest/installation/micromamba-installation.html) or equivalent.
For a conda-based installation, you'll need [micromamba](https://mamba.readthedocs.io/en/latest/installation/micromamba-installation.html) or equivalent. The Docker-based installation requires you to install and login to [Docker](https://www.docker.com/get-started/).

### User install (conda environment)

Expand All @@ -71,3 +71,24 @@ pip install -e ".[dev]"
```

The last line installs mdx2 in editable mode, with optional development tools including pytest and ruff

### Developer Install (Docker container)

Pull image from Docker Hub and run the container. Search the Docker container logs for the localhost address to access mdx2 within a Jupyter Lab notebook or terminal.

- Docker image exposes ports 8880-8890 (default: 8888)
- Platform: linux/amd64
```bash
git clone https://github.com/diff-use/mdx2.git
cd mdx2
Comment on lines +82 to +83
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Mar 10, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟡 Minor

Clarify why cloning is needed when Docker already contains mdx2.

The Docker image already has mdx2 installed. The git clone step is presumably to mount local code for development, but this isn't explained. Users expecting a "just works" Docker install will be confused.

📝 Suggested clarification

Add context before the commands:

Clone the repository to mount your local code into the container for development. This allows you to edit files on your host and see changes reflected inside the container.

Or, if cloning is optional:

(Optional) Clone the repository if you want to mount local code for development:
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@README.md` around lines 81 - 82, Clarify why the README asks users to run
"git clone" before starting Docker: add a short sentence above the two commands
explaining that cloning the repository is optional and is only needed if they
want to mount local code into the container for development so edits on the host
are reflected inside the container; alternatively mark the step as optional and
state that the Docker image already contains mdx2 for users who just want to run
the container without mounting local sources.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

The Docker image doesn't have mdx2 installed, so that's why the git clone step is needed

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@jlee733, thanks for the clarification! I was wrong about the image having mdx2 pre-installed.

That said, the README could still benefit from a brief note explaining that the git clone is required (not optional) because mdx2 is not bundled in the image — it's mounted from the local clone. This would prevent confusion for new users who might wonder why they need to clone before running Docker. For example:

### Developer Install (Docker container)

Pull image from Docker Hub and run the container. The repository must be cloned locally so it can be mounted into the container — mdx2 is not pre-installed in the image. Search the Docker container logs for the localhost address to access mdx2 within a Jupyter Lab notebook or terminal.

This is just a suggestion to keep the documentation clear. Feel free to word it however fits best!


✏️ Learnings added
Learnt from: jlee733
Repo: diff-use/mdx2 PR: 57
File: README.md:82-83
Timestamp: 2026-03-11T14:12:25.456Z
Learning: In the diff-use/mdx2 repository, the Docker image (diffuseproject/mdx2) does NOT have mdx2 pre-installed. The `git clone` step in the README's "Developer Install (Docker container)" section is required so the local repository can be mounted into the container via the volume flag (`-v "$(pwd)":/home/dev`).

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

docker pull diffuseproject/mdx2:1.0.0
docker run --platform linux/amd64 --name mdx2_container -it -d -e JUPYTER_PORT={CONTAINER_PORT} -p {HOST_PORT}:{CONTAINER_PORT} -v "$(pwd)":/home/dev diffuseproject/mdx2:1.0.0
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

Undefined placeholders {CONTAINER_PORT} and {HOST_PORT} are not actionable.

Users cannot execute this command without knowing what values to substitute. The Dockerfile sets JUPYTER_PORT=8888 by default, so provide concrete defaults with guidance on customization.

🐛 Proposed fix with concrete values and explanation

Replace lines 77-86 with:

Pull image from Docker Hub and run the container. The Jupyter Lab URL will be printed when you run `docker logs`.

- Default Jupyter port: **8888**
- Platform: `linux/amd64`
- To use a different host port, replace `8888:8888` with `{YOUR_HOST_PORT}:8888`

```bash
git clone https://github.com/diff-use/mdx2.git
cd mdx2
docker pull diffuseproject/mdx2:latest
docker run --platform linux/amd64 --name mdx2_container -d -p 8888:8888 -v "$(pwd)":/home/dev diffuseproject/mdx2:latest
docker logs mdx2_container
</details>

<details>
<summary>🤖 Prompt for AI Agents</summary>

Verify each finding against the current code and only fix it if needed.

In @README.md at line 84, Replace the undefined placeholders {CONTAINER_PORT}
and {HOST_PORT} in the docker run example with concrete defaults and a
customization note: use the IMAGE env var JUPYTER_PORT default 8888 (set via
JUPYTER_PORT) and show the docker run mapping as -p 8888:8888 by default,
explain that users can change the host side (e.g., {YOUR_HOST_PORT}:8888) to
customize, and add a note to run docker logs <container_name> to retrieve the
Jupyter URL; update the README.md docker run line accordingly and mention the
default port and how to override it.


</details>

<!-- fingerprinting:phantom:poseidon:ocelot -->

<!-- This is an auto-generated comment by CodeRabbit -->

docker logs mdx2_container
```
Comment thread
coderabbitai[bot] marked this conversation as resolved.

(Optional) Open a shell in the running container:
```bash
docker exec -it mdx2_container /bin/bash
eval "$(micromamba shell hook --shell bash)"
micromamba activate 'mdx2-dev'
```
Loading