From 39c0dd3736da0722c7e18d84183c0e9b06cf2839 Mon Sep 17 00:00:00 2001 From: Ruben Fiszel Date: Thu, 20 Feb 2025 02:51:04 +0100 Subject: [PATCH] fix(python): PYTHON_PATH overrides python from uv --- .devcontainer/docker-compose.yml | 1 - .github/workflows/backend-test.yml | 5 +- README.md | 2 +- .../windmill-worker/src/python_executor.rs | 69 ++++++++++++++----- .../windmill-worker/src/worker_lockfiles.rs | 36 ++++++++-- flake.nix | 1 - 6 files changed, 83 insertions(+), 31 deletions(-) diff --git a/.devcontainer/docker-compose.yml b/.devcontainer/docker-compose.yml index 4de33c741cefa..98a82077cff0f 100644 --- a/.devcontainer/docker-compose.yml +++ b/.devcontainer/docker-compose.yml @@ -7,7 +7,6 @@ services: # image: mcr.microsoft.com/vscode/devcontainers/rust:bullseye environment: - DENO_PATH=/usr/local/cargo/bin/deno - - PYTHON_PATH=/usr/bin/python3 - NSJAIL_PATH=/bin/nsjail volumes: - .:/workspace:cached diff --git a/.github/workflows/backend-test.yml b/.github/workflows/backend-test.yml index 04668e96e0efc..1cbefbaf91e71 100644 --- a/.github/workflows/backend-test.yml +++ b/.github/workflows/backend-test.yml @@ -42,9 +42,6 @@ jobs: - uses: actions/setup-go@v2 with: go-version: 1.21.5 - - uses: actions/setup-python@v2 - with: - python-version: 3.11 - uses: oven-sh/setup-bun@v2 with: bun-version: 1.1.43 @@ -64,7 +61,7 @@ jobs: deno --version && bun -v && go version && python3 --version && SQLX_OFFLINE=true DATABASE_URL=postgres://postgres:changeme@localhost:5432/windmill - DISABLE_EMBEDDING=true RUST_LOG=info PYTHON_PATH=$(which python) + DISABLE_EMBEDDING=true RUST_LOG=info DENO_PATH=$(which deno) BUN_PATH=$(which bun) GO_PATH=$(which go) UV_PATH=$(which uv) cargo test --features enterprise,deno_core,license,python,rust,scoped_cache --all -- diff --git a/README.md b/README.md index 3f0fef9ff6047..d8ec51b3b0798 100644 --- a/README.md +++ b/README.md @@ -330,7 +330,7 @@ you to have it being synced automatically everyday. | SLACK_SIGNING_SECRET | None | The signing secret of your Slack app. See [Slack documentation](https://api.slack.com/authentication/verifying-requests-from-slack) | Server | | COOKIE_DOMAIN | None | The domain of the cookie. If not set, the cookie will be set by the browser based on the full origin | Server | | DENO_PATH | /usr/bin/deno | The path to the deno binary. | Worker | -| PYTHON_PATH | /usr/local/bin/python3 | The path to the python binary. | Worker | +| PYTHON_PATH | | The path to the python binary if wanting to not have it managed by uv. | Worker | | GO_PATH | /usr/bin/go | The path to the go binary. | Worker | | GOPRIVATE | | The GOPRIVATE env variable to use private go modules | Worker | | GOPROXY | | The GOPROXY env variable to use | Worker | diff --git a/backend/windmill-worker/src/python_executor.rs b/backend/windmill-worker/src/python_executor.rs index eb35cd7f4a5a5..9c297f6180ea6 100644 --- a/backend/windmill-worker/src/python_executor.rs +++ b/backend/windmill-worker/src/python_executor.rs @@ -39,8 +39,10 @@ use std::env::var; use windmill_queue::{append_logs, CanceledBy}; lazy_static::lazy_static! { - static ref PYTHON_PATH: String = - var("PYTHON_PATH").unwrap_or_else(|_| "/usr/local/bin/python3".to_string()); + static ref PYTHON_PATH: Option = var("PYTHON_PATH").ok().map(|v| { + tracing::warn!("PYTHON_PATH is set to {} and thus python will not be managed by uv and stay static regardless of annotation and instance settings. NOT RECOMMENDED", v); + v + }); static ref UV_PATH: String = var("UV_PATH").unwrap_or_else(|_| "/usr/local/bin/uv".to_string()); @@ -773,6 +775,30 @@ fn copy_dir_recursively(src: &Path, dst: &Path) -> windmill_common::error::Resul Ok(()) } +async fn get_python_path( + py_version: PyVersion, + worker_name: &str, + job_id: &Uuid, + w_id: &str, + mem_peak: &mut i32, + db: &sqlx::Pool, + occupancy_metrics: &mut Option<&mut OccupancyMetrics>, +) -> windmill_common::error::Result { + let python_path = if let Some(python_path) = PYTHON_PATH.clone() { + python_path + } else if let Some(python_path) = py_version + .get_python(&job_id, mem_peak, db, worker_name, w_id, occupancy_metrics) + .await? + { + python_path + } else { + return Err(Error::ExecutionErr(format!( + "uv could not manage python path. Please manage it manually by setting PYTHON_PATH environment variable to your python binary path" + ))); + }; + Ok(python_path) +} + #[tracing::instrument(level = "trace", skip_all)] pub async fn handle_python_job( requirements_o: Option<&String>, @@ -811,21 +837,16 @@ pub async fn handle_python_job( let PythonAnnotations { no_postinstall, .. } = PythonAnnotations::parse(inner_content); tracing::debug!("Finished handling python dependencies"); - let python_path = if let Some(python_path) = py_version - .get_python( - &job.id, - mem_peak, - db, - worker_name, - &job.workspace_id, - &mut Some(occupancy_metrics), - ) - .await? - { - python_path - } else { - PYTHON_PATH.clone() - }; + let python_path = get_python_path( + py_version, + worker_name, + &job.id, + &job.workspace_id, + mem_peak, + db, + &mut Some(occupancy_metrics), + ) + .await?; if !no_postinstall { if let Err(e) = postinstall(&mut additional_python_paths, job_dir, job, db).await { @@ -2395,8 +2416,20 @@ for line in sys.stdin: base_internal_url.to_string(), ); proc_envs.insert("BASE_URL".to_string(), base_internal_url.to_string()); + + let py_version = PyVersion::from_instance_version().await; + let python_path = get_python_path( + py_version, + worker_name, + &Uuid::nil(), + w_id, + &mut mem_peak, + db, + &mut None, + ) + .await?; handle_dedicated_process( - &*PYTHON_PATH, + &python_path, job_dir, context_envs, envs, diff --git a/backend/windmill-worker/src/worker_lockfiles.rs b/backend/windmill-worker/src/worker_lockfiles.rs index bab20bea651cb..6ac4ad6aabc35 100644 --- a/backend/windmill-worker/src/worker_lockfiles.rs +++ b/backend/windmill-worker/src/worker_lockfiles.rs @@ -747,6 +747,11 @@ fn get_deployment_msg_and_parent_path_from_args( (deployment_message, parent_path) } +struct LockModuleError { + id: String, + error: Error, +} + async fn lock_modules<'c>( modules: Vec, job: &QueuedJob, @@ -770,7 +775,9 @@ async fn lock_modules<'c>( )> { let mut new_flow_modules = Vec::new(); let mut modified_ids = Vec::new(); + let mut errors = Vec::new(); for mut e in modules.into_iter() { + let id = e.id.clone(); let mut nmodified_ids = Vec::new(); let FlowModuleValue::RawScript { lock, @@ -1022,12 +1029,7 @@ async fn lock_modules<'c>( } Err(error) => { // TODO: Record flow raw script error lock logs - tracing::warn!( - path = path, - language = ?language, - error = ?error, - "Failed to generate flow lock for raw script" - ); + errors.push(LockModuleError { id, error }); None } }; @@ -1046,6 +1048,28 @@ async fn lock_modules<'c>( new_flow_modules.push(e); continue; } + if !errors.is_empty() { + let error_message = errors + .iter() + .map(|e| format!("{}: {}", e.id, e.error)) + .collect::>() + .join("\n"); + let logs2 = sqlx::query_scalar!( + "SELECT logs FROM job_logs WHERE job_id = $1 AND workspace_id = $2", + &job.id, + &job.workspace_id + ) + .fetch_optional(db) + .await? + .flatten() + .unwrap_or_else(|| "no logs".to_string()); + + return Err(Error::ExecutionErr(format!( + "Error locking flow modules:\n{}\n\nlogs:\n{}", + error_message, + remove_ansi_codes(&logs2) + ))); + } Ok((new_flow_modules, tx, modified_ids)) } diff --git a/flake.nix b/flake.nix index 219a103cd9d98..6f4463e5ae28d 100644 --- a/flake.nix +++ b/flake.nix @@ -97,7 +97,6 @@ REMOTE_LSP = "http://127.0.0.1:3001"; RUSTC_WRAPPER = "${pkgs.sccache}/bin/sccache"; DENO_PATH = "${pkgs.deno}/bin/deno"; - PYTHON_PATH = "${pkgs.python3}/bin/python3"; GO_PATH = "${pkgs.go}/bin/go"; BUN_PATH = "${pkgs.bun}/bin/bun"; UV_PATH = "${pkgs.uv}/bin/uv";