revert installation of sqlalchemy-hana during flow run#2824
Conversation
alicia-koh
requested review from
SantanM,
Zhimin-arya,
brandantck,
csafreen,
p-hoffmann and
suwarnoong
as code owners
There was a problem hiding this comment.
Pull request overview
This PR removes per-flow runtime installation of sqlalchemy-hana (previously injected via each flow’s command) and introduces a centralized, env-driven mechanism for optionally overriding the Prefect Docker worker command when needed.
Changes:
- Removed
commandoverrides that installedsqlalchemy-hanaat flow-run time from several flowpackage.jsondefinitions. - Plumbed a new
INSTALL_SQLALCHEMYenvironment variable through Trex, docker-compose, and thealp-dataflow-gen-initfunction to control the Prefect workpool command default. - Standardized
INSTALL_SQLALCHEMY_HANAdefaults in flow images via Dockerfiles.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| services/trex/core/server/env.ts | Adds INSTALL_SQLALCHEMY to Trex server env passthrough. |
| plugins/functions/package.json | Exposes INSTALL_SQLALCHEMY to the functions runtime environment mapping. |
| plugins/functions/alp-dataflow-gen-init/src/env.ts | Adds INSTALL_SQLALCHEMY to function env parsing/exports. |
| plugins/functions/alp-dataflow-gen-init/src/customWorkpool.ts | Uses INSTALL_SQLALCHEMY as the default Prefect Docker workpool command. |
| plugins/flows/search_embedding/package.json | Removes flow-level command that installed sqlalchemy-hana during execution. |
| plugins/flows/search_embedding/Dockerfile | Sets INSTALL_SQLALCHEMY_HANA default for the image. |
| plugins/flows/loyalty_score/Dockerfile | Sets INSTALL_SQLALCHEMY_HANA default for the image. |
| plugins/flows/i2b2/Dockerfile | Sets INSTALL_SQLALCHEMY_HANA default for the image. |
| plugins/flows/hades/Dockerfile | Sets INSTALL_SQLALCHEMY_HANA default for the image. |
| plugins/flows/data_transformation/Dockerfile | Sets INSTALL_SQLALCHEMY_HANA default for the image. |
| plugins/flows/data_management/package.json | Removes flow-level command that installed sqlalchemy-hana during execution. |
| plugins/flows/base/package.json | Removes flow-level command that installed sqlalchemy-hana during execution. |
| docker-compose.yml | Adds INSTALL_SQLALCHEMY to the service environment. |
| command: { | ||
| type: "string", | ||
| title: "Command", | ||
| default: "", | ||
| default: INSTALL_SQLALCHEMY, | ||
| description: | ||
| "The command to use when starting a flow run. In most cases, this should be left blank and the command will be automatically generated by the worker.", | ||
| }, |
| PREFECT_API_URL: _env.PREFECT_API_URL, | ||
| D2E_MEMORY_LIMIT: _env.D2E_MEMORY_LIMIT, | ||
| D2E_SWAP_LIMIT: _env.D2E_SWAP_LIMIT, | ||
| WORKPOOL_NAME: _env.WORKPOOL_NAME | ||
| WORKPOOL_NAME: _env.WORKPOOL_NAME, | ||
| INSTALL_SQLALCHEMY: _env.INSTALL_SQLALCHEMY, | ||
| }; |
| USE_PUBLIC_WEBAPI_PROXY: _env.USE_PUBLIC_WEBAPI_PROXY, | ||
| PUBLIC_WEBAPI_PROXY_URL: _env.PUBLIC_WEBAPI_PROXY_URL, | ||
| PUBLIC_WEBAPI_DATASOURCE: _env.PUBLIC_WEBAPI_DATASOURCE, | ||
| GOOGLE_APPLICATION_CREDENTIALS: _env.GOOGLE_APPLICATION_CREDENTIALS, | ||
| REDIS_PASSWORD: _env.REDIS_PASSWORD, | ||
| INSTALL_SQLALCHEMY: _env.INSTALL_SQLALCHEMY, | ||
| D2E_MEMORY_LIMIT: _env.D2E_MEMORY_LIMIT, | ||
| D2E_SWAP_LIMIT: _env.D2E_SWAP_LIMIT, |
| D2E_SWAP_LIMIT: ${D2E_SWAP_LIMIT:-} | ||
| D2E_DUCKDB_MEMORY_LIMIT: ${D2E_DUCKDB_MEMORY_LIMIT:-15GB} | ||
| D2E_DUCKDB_THREADS: ${D2E_DUCKDB_THREADS:-} | ||
| INSTALL_SQLALCHEMY: ${INSTALL_SQLALCHEMY:-} |
There was a problem hiding this comment.
should we set bash -c 'uv pip install sqlalchemy-hana==2.2.0 && prefect flow-run execute' as default value of INSTALL_SQLALCHEMY
There was a problem hiding this comment.
no because that will require internet access to download sqlalchemy-hana when flow run starts and we don't want that
| D2E_SWAP_LIMIT: ${D2E_SWAP_LIMIT:-} | ||
| D2E_DUCKDB_MEMORY_LIMIT: ${D2E_DUCKDB_MEMORY_LIMIT:-15GB} | ||
| D2E_DUCKDB_THREADS: ${D2E_DUCKDB_THREADS:-} | ||
| INSTALL_SQLALCHEMY: ${INSTALL_SQLALCHEMY:-} |
There was a problem hiding this comment.
INSTALL_SQLALCHEMY in the d-c.yml but INSTALL_SQLALCHEMY_HANA in Dockerfile. Do I miss something?
There was a problem hiding this comment.
The dockerfile INSTALL_SQLALCHEMY_HANA is an env we set so when flow run container starts it runs a custom command to install sqlalchemy hana first. INSTALL_SQLALCHEMY is a variable that contains that custom command which we set in .env and import into the prefect workpool. The workpool will spin up containers with the command we set.
There was a problem hiding this comment.
cool, thanks for explaining
There was a problem hiding this comment.
@alicia-koh - The naming of the env variable does not sound right when it is intended for providing custom command for starting the container. Could you please change it to something like - PREFECT__FLOWRUN_STARTUP_CUSTOM_CMD?
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Merge Checklist
Please cross check this list if additions / modifications needs to be done on top of your core changes and tick them off. Reviewer can as well glance through and help the developer if something is missed out.
developbranch)