Dev-iL commented on code in PR #1488: URL: https://github.com/apache/hamilton/pull/1488#discussion_r2845166750
########## dev_tools/language_server/pyproject.toml: ########## @@ -1,42 +1,67 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + [build-system] -requires = ["setuptools >= 65.0.0"] -build-backend = "setuptools.build_meta" +requires = ["flit_core >=3.11,<4"] +build-backend = "flit_core.buildapi" Review Comment: Just curious: what led you make this move? Why did this require changing? ########## ui/backend/server/server/urls.py: ########## @@ -44,12 +43,50 @@ if settings.HAMILTON_ENV == "mini": # mini-mode # TODO -- do meda assets correctly -- this just hardcodes logo.png for now + import os Review Comment: better to use `pathlib.Path` ########## ui/backend/server/entrypoint.sh: ########## @@ -17,5 +17,6 @@ set -e -python manage.py migrate # Apply database migrations -python manage.py runserver 0.0.0.0:8241 # Start the server +# Use uv run to execute Python commands with the virtual environment +uv run python manage.py migrate # Apply database migrations +uv run python manage.py runserver 0.0.0.0:8241 # Start the server Review Comment: Should we shorten this using `[project.scripts]`? I.e. ```toml [project.scripts] migrate = "manage:migrate" server = "manage:runserver" ``` then, ```suggestion uv run migrate # Apply database migrations uv run server 0.0.0.0:8241 # Start the server ``` ########## ui/backend/Dockerfile.backend: ########## @@ -15,21 +15,40 @@ # specific language governing permissions and limitations # under the License. -FROM python:3.8-slim +# Apache Hamilton is an effort undergoing incubation at The Apache Software +# Foundation (ASF), sponsored by the Apache Incubator. Incubation is required +# of all newly accepted projects until a further review indicates that the +# infrastructure, communications, and decision making process have stabilized +# in a manner consistent with other successful ASF projects. While incubation +# status is not necessarily a reflection of the completeness or stability of +# the code, it does indicate that the project has yet to be fully endorsed by +# the ASF. -ENV PYTHONDONTWRITEBYTECODE 1 -ENV PYTHONUNBUFFERED 1 +FROM ghcr.io/astral-sh/uv:python3.12-bookworm-slim + +ENV PYTHONDONTWRITEBYTECODE=1 \ + PYTHONUNBUFFERED=1 \ + UV_COMPILE_BYTECODE=1 \ + UV_LINK_MODE=copy WORKDIR /code -# Install system dependencies +# Install system dependencies for psycopg2-binary and other compiled packages RUN apt-get update && apt-get install -y \ gcc \ libc-dev \ libffi-dev \ + libpq-dev \ Review Comment: Is this required for `psycopg` (v3)? ########## ui/backend/server/requirements.txt: ########## Review Comment: Why don't we transition fully to `pyproject.toml`? ########## contrib/hamilton/contrib/dagworks/author.md: ########## @@ -22,6 +22,6 @@ title: DAGWorks --- # Officially maintained Apache Hamilton dataflows -Here you'll find dataflows that are officially maintained by the DAGWorks team. Popular +Here you'll find dataflows that are officially maintained by the Apache Hamilton github/mailing lists. Popular user-contributed dataflows will be moved here as necessary to support their continued development and maintenance. Review Comment: nit: inconsistent line length ########## ui/docker-compose-prod.yml: ########## @@ -3,7 +3,7 @@ version: '3.8' services: db: - image: postgres:12 + image: postgres:16 Review Comment: why specifically 16 (and not say 18)? ########## ui/.claude/settings.local.json: ########## Review Comment: git-ignore `*.local.*`? ########## hamilton/plugins/pandas_extensions.py: ########## Review Comment: I encourage you to split this into a separate PR since it's completely standalone and can immediately benefit CI stability. ########## ui/backend/server/server/settings.py: ########## @@ -147,9 +148,13 @@ def get_from_env( if HAMILTON_ENV == "mini": STATIC_ROOT = os.path.join(BASE_DIR, "staticfiles") STATIC_URL = "/static/" - STATICFILES_DIRS = [ - os.path.join(BASE_DIR, "build/static/"), - ] + # Support both Vite (assets/) and CRA (static/) build outputs + staticfiles_dirs = [] + if os.path.exists(os.path.join(BASE_DIR, "build/assets/")): + staticfiles_dirs.append(os.path.join(BASE_DIR, "build/assets/")) + if os.path.exists(os.path.join(BASE_DIR, "build/static/")): + staticfiles_dirs.append(os.path.join(BASE_DIR, "build/static/")) Review Comment: nit: I suggest using `pathlib.Path` so that ruff doesn't complain about this later ########## ui/frontend/.claude/settings.local.json: ########## Review Comment: Same comment as before. ########## .github/workflows/hamilton-sdk.yml: ########## @@ -33,9 +33,9 @@ jobs: - name: Install dependencies run: | python -m pip install --upgrade pip - pip install -e ${{ github.workspace }} pip install -r requirements.txt pip install -r requirements-test.txt + pip install -e ${{ github.workspace }} Review Comment: I came across this too while trying to fix CI in #1490. This is something that Claude suggested initially, but (at least in my case) it became unnecessary after the package name was update in key places. ########## ui/sdk/pyproject.toml: ########## @@ -1,62 +1,102 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + [build-system] -requires = ["setuptools >= 61.0.0", "wheel"] -build-backend = "setuptools.build_meta" +requires = ["flit_core >=3.11,<4"] +build-backend = "flit_core.buildapi" [project] -name = "sf-hamilton-sdk" -description = "Hamilton SDK for reading and writing to the Hamilton backend APIs that support the UI." +name = "apache-hamilton-sdk" +version = "0.8.0" +description = "Apache Hamilton SDK for reading and writing to the Apache Hamilton backend APIs that support the UI." +readme = "README.md" +requires-python = ">=3.10, <4" +license = "Apache-2.0" +license-files = ["LICENSE", "NOTICE", "DISCLAIMER"] +keywords = ["hamilton", "dagworks", "observability"] authors = [ {name = "Stefan Krawczyk", email = "[email protected]"}, {name = "Elijah ben Izzy", email = "[email protected]"} ] -readme = "README.md" -keywords = ["hamilton", "dagworks", "observability"] classifiers = [ "Development Status :: 4 - Beta", "Intended Audience :: Developers", "Natural Language :: English", - "License :: OSI Approved :: Apache Software License", "Programming Language :: Python :: 3", "Programming Language :: Python :: 3.10", "Programming Language :: Python :: 3.11", "Programming Language :: Python :: 3.12", "Programming Language :: Python :: 3.13", "Programming Language :: Python :: 3.14" ] -requires-python = ">=3.10, <4" -dynamic = ["dependencies", "optional-dependencies", "version"] +dependencies = [ + "aiohttp", + "click", + "gitpython", + "jinja2", + "loguru", + "posthog", + "requests", + "apache-hamilton>=1.43.0", + "sqlglot", +] + +[project.optional-dependencies] Review Comment: I suggest including [development dependencies](https://docs.astral.sh/uv/concepts/projects/dependencies/#development-dependencies) under a _group_: ```toml [dependency-groups] test = [ ... ] ``` ########## ui/start_mini_mode.sh: ########## @@ -0,0 +1,88 @@ +#!/bin/bash +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +# Start Hamilton UI in mini mode for local testing + +set -e + +echo "🚀 Starting Hamilton UI in Mini Mode" +echo "====================================" +echo "" + +# Check if PostgreSQL container is running +if ! docker ps --format '{{.Names}}' | grep -q "^hamilton-ui-db$"; then + echo "📦 Starting PostgreSQL container..." + docker run -d \ + --name hamilton-ui-db \ + -e POSTGRES_DB=hamilton \ + -e POSTGRES_USER=hamilton \ + -e POSTGRES_PASSWORD=hamilton \ + -p 5433:5432 \ + postgres:12 Review Comment: Why is this 12 and not 16/18? ########## ui/start_mini_mode.sh: ########## @@ -0,0 +1,88 @@ +#!/bin/bash +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +# Start Hamilton UI in mini mode for local testing + +set -e + +echo "🚀 Starting Hamilton UI in Mini Mode" +echo "====================================" +echo "" + +# Check if PostgreSQL container is running +if ! docker ps --format '{{.Names}}' | grep -q "^hamilton-ui-db$"; then + echo "📦 Starting PostgreSQL container..." + docker run -d \ + --name hamilton-ui-db \ + -e POSTGRES_DB=hamilton \ + -e POSTGRES_USER=hamilton \ + -e POSTGRES_PASSWORD=hamilton \ + -p 5433:5432 \ + postgres:12 + + echo "⏳ Waiting for PostgreSQL to be ready..." + sleep 5 + + # Wait for PostgreSQL to accept connections + until docker exec hamilton-ui-db pg_isready -U hamilton; do + echo "Waiting for database..." + sleep 2 + done + echo "✅ PostgreSQL is ready" +else + echo "✅ PostgreSQL container already running" +fi + +echo "" +echo "📝 Setting up environment variables..." + +# Export environment variables for mini mode +export HAMILTON_ENV=mini +export DB_HOST=localhost +export DB_PORT=5433 +export DB_NAME=hamilton +export DB_USER=hamilton +export DB_PASSWORD=hamilton +export HAMILTON_BLOB_STORE=local +export HAMILTON_LOCAL_BLOB_DIR=$(pwd)/ui/backend/server/blobs +export DJANGO_SECRET_KEY=mini-mode-secret-key-for-testing-only +export DJANGO_DEBUG=True +export HAMILTON_AUTH_MODE=permissive +export HAMILTON_PERMISSIVE_MODE_GLOBAL_KEY=test-key + +echo "✅ Environment configured" +echo "" + +# Create blob directory if it doesn't exist +mkdir -p "$HAMILTON_LOCAL_BLOB_DIR" + +echo "🔧 Running database migrations..." +cd ui/backend/server +python manage.py migrate Review Comment: Didn't you intend to use uv for this? ########## ui/start_mini_mode.sh: ########## @@ -0,0 +1,88 @@ +#!/bin/bash +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. + +# Start Hamilton UI in mini mode for local testing + +set -e + +echo "🚀 Starting Hamilton UI in Mini Mode" +echo "====================================" +echo "" + +# Check if PostgreSQL container is running +if ! docker ps --format '{{.Names}}' | grep -q "^hamilton-ui-db$"; then + echo "📦 Starting PostgreSQL container..." + docker run -d \ + --name hamilton-ui-db \ + -e POSTGRES_DB=hamilton \ + -e POSTGRES_USER=hamilton \ + -e POSTGRES_PASSWORD=hamilton \ + -p 5433:5432 \ + postgres:12 + + echo "⏳ Waiting for PostgreSQL to be ready..." + sleep 5 + + # Wait for PostgreSQL to accept connections + until docker exec hamilton-ui-db pg_isready -U hamilton; do + echo "Waiting for database..." + sleep 2 + done + echo "✅ PostgreSQL is ready" +else + echo "✅ PostgreSQL container already running" +fi + +echo "" +echo "📝 Setting up environment variables..." + +# Export environment variables for mini mode +export HAMILTON_ENV=mini +export DB_HOST=localhost +export DB_PORT=5433 +export DB_NAME=hamilton +export DB_USER=hamilton +export DB_PASSWORD=hamilton +export HAMILTON_BLOB_STORE=local +export HAMILTON_LOCAL_BLOB_DIR=$(pwd)/ui/backend/server/blobs +export DJANGO_SECRET_KEY=mini-mode-secret-key-for-testing-only +export DJANGO_DEBUG=True +export HAMILTON_AUTH_MODE=permissive +export HAMILTON_PERMISSIVE_MODE_GLOBAL_KEY=test-key + +echo "✅ Environment configured" +echo "" + +# Create blob directory if it doesn't exist +mkdir -p "$HAMILTON_LOCAL_BLOB_DIR" + +echo "🔧 Running database migrations..." +cd ui/backend/server +python manage.py migrate + +echo "" +echo "✨ Starting Hamilton UI server..." +echo "" +echo " URL: http://localhost:8241"; +echo " Auth: Permissive mode (no login required)" +echo "" +echo "Press Ctrl+C to stop the server" +echo "" + +# Start the server +python manage.py runserver 0.0.0.0:8241 Review Comment: uv -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
