You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@hamilton.apache.org by "skrawcz (via GitHub)" <gi...@apache.org> on 2025/10/05 17:25:24 UTC
[PR] Updates UI: removes elkjs, updates backend deps [hamilton]
skrawcz opened a new pull request, #1403:
URL: https://github.com/apache/hamilton/pull/1403
## Changes
[Removes elkjs and adds dagre](https://github.com/apache/hamilton/commit/5921124f2de41372a7f32a1d55a1932f40f0d40c)
Need to test this more -- but basic graphs work.
[Updates backend to use UV and updates to use 3.9](https://github.com/apache/hamilton/commit/23b955dfc94b85d34bbeb847144d9316227e2e08)
So python 3.8 is gone. Updates to use python 3.9
Updates deps and to use uv for reproducible builds.
## How I tested this
- locally
## Notes
## Checklist
- [ ] PR has an informative and human-readable title (this will be pulled into the release notes)
- [ ] Changes are limited to a single goal (no scope creep)
- [ ] Code passed the pre-commit check & code is left cleaner/nicer than when first encountered.
- [ ] Any _change_ in functionality is tested
- [ ] New functions are documented (with a description, list of inputs, and expected output)
- [ ] Placeholder code is flagged / future TODOs are captured in comments
- [ ] Project documentation has been updated if adding/changing functionality.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@hamilton.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Updates UI: removes elkjs, updates backend deps [hamilton]
Posted by Stefan Krawczyk <st...@gmail.com>.
good catch -- I vibe coded this. Let me have a look.
We had two originally because you can't have if else in docker files. So we
want slightly different setup in dev vs prod.
On Mon, Oct 6, 2025 at 6:52 AM zilto (via GitHub) <gi...@apache.org> wrote:
>
> zilto commented on code in PR #1403:
> URL: https://github.com/apache/hamilton/pull/1403#discussion_r2406408993
>
>
> ##########
> ui/backend/Dockerfile.backend:
> ##########
>
>
> Review Comment:
> For reference, there are ways to optimize the Docker build process such
> that it uses the host's local `uv` cache when building Python dependencies
> inside the container. This can lead to significant speed improvement in dev
>
> example:
> https://github.com/astral-sh/uv-docker-example/blob/main/multistage.Dockerfile
>
>
>
> ##########
> ui/backend/Dockerfile.backend-prod:
> ##########
> @@ -1,20 +1,24 @@
> -FROM python:3.8-slim
> +FROM python:3.9-slim
>
> ENV PYTHONDONTWRITEBYTECODE 1
> ENV PYTHONUNBUFFERED 1
>
> WORKDIR /code
>
> -# Install system dependencies
> +# Install system dependencies and uv
> RUN apt-get update && apt-get install -y \
> gcc \
> libc-dev \
> libffi-dev \
> - && rm -rf /var/lib/apt/lists/*
> + curl \
> + && rm -rf /var/lib/apt/lists/* \
> + && curl -LsSf https://astral.sh/uv/install.sh | sh
> +
> +ENV PATH="/root/.local/bin:$PATH"
>
> COPY . /code/
> -COPY server/requirements.txt /code/
> +COPY server/requirements-locked.txt /code/
>
> -RUN pip install -r /code/requirements.txt
> +RUN uv pip install --system -r /code/requirements-locked.txt
>
> Review Comment:
> I see that `Dockerfile.backend` and `Dockerfile.backend-prod` already
> existed, but it seems they were identical except the additional line 15
> (origin) in `backend-prod`?
>
> I have a few questions:
> - why are two files needed?
> - why use the frozen `requirements-locked.txt` in `backend-prod`? This
> just seems like a weaker `uv.lock`. Are there any particular trade-offs?
> - In `backend-prod`, the line `COPY . /code/` appears twice (line 15
> and line 20 in origin). Is this intended?
>
>
>
> --
> This is an automated message from the Apache Git Service.
> To respond to the message, please log on to GitHub and use the
> URL above to go to the specific comment.
>
> To unsubscribe, e-mail: notifications-unsubscribe@hamilton.apache.org
>
> For queries about this service, please contact Infrastructure at:
> users@infra.apache.org
>
>
Re: [PR] Updates UI: removes elkjs, updates backend deps [hamilton]
Posted by "skrawcz (via GitHub)" <gi...@apache.org>.
skrawcz closed pull request #1403: Updates UI: removes elkjs, updates backend deps
URL: https://github.com/apache/hamilton/pull/1403
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@hamilton.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Updates UI: removes elkjs, updates backend deps [hamilton]
Posted by "zilto (via GitHub)" <gi...@apache.org>.
zilto commented on code in PR #1403:
URL: https://github.com/apache/hamilton/pull/1403#discussion_r2406408993
##########
ui/backend/Dockerfile.backend:
##########
Review Comment:
For reference, there are ways to optimize the Docker build process such that it uses the host's local `uv` cache when building Python dependencies inside the container. This can lead to significant speed improvement in dev
example: https://github.com/astral-sh/uv-docker-example/blob/main/multistage.Dockerfile
##########
ui/backend/Dockerfile.backend-prod:
##########
@@ -1,20 +1,24 @@
-FROM python:3.8-slim
+FROM python:3.9-slim
ENV PYTHONDONTWRITEBYTECODE 1
ENV PYTHONUNBUFFERED 1
WORKDIR /code
-# Install system dependencies
+# Install system dependencies and uv
RUN apt-get update && apt-get install -y \
gcc \
libc-dev \
libffi-dev \
- && rm -rf /var/lib/apt/lists/*
+ curl \
+ && rm -rf /var/lib/apt/lists/* \
+ && curl -LsSf https://astral.sh/uv/install.sh | sh
+
+ENV PATH="/root/.local/bin:$PATH"
COPY . /code/
-COPY server/requirements.txt /code/
+COPY server/requirements-locked.txt /code/
-RUN pip install -r /code/requirements.txt
+RUN uv pip install --system -r /code/requirements-locked.txt
Review Comment:
I see that `Dockerfile.backend` and `Dockerfile.backend-prod` already existed, but it seems they were identical except the additional line 15 (origin) in `backend-prod`?
I have a few questions:
- why are two files needed?
- why use the frozen `requirements-locked.txt` in `backend-prod`? This just seems like a weaker `uv.lock`. Are there any particular trade-offs?
- In `backend-prod`, the line `COPY . /code/` appears twice (line 15 and line 20 in origin). Is this intended?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@hamilton.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Updates UI: removes elkjs, updates backend deps [hamilton]
Posted by "skrawcz (via GitHub)" <gi...@apache.org>.
skrawcz commented on PR #1403:
URL: https://github.com/apache/hamilton/pull/1403#issuecomment-4529793815
Superseded by #1600 which takes a cleaner approach using dagre's native compound graph support.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@hamilton.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org