Skip to content

Commit

Permalink
fix(python): PYTHON_PATH overrides python from uv
Browse files Browse the repository at this point in the history
  • Loading branch information
rubenfiszel committed Feb 20, 2025
1 parent d5b3a04 commit 39c0dd3
Show file tree
Hide file tree
Showing 6 changed files with 83 additions and 31 deletions.
1 change: 0 additions & 1 deletion .devcontainer/docker-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
5 changes: 1 addition & 4 deletions .github/workflows/backend-test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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 --
Expand Down
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 |
Expand Down
69 changes: 51 additions & 18 deletions backend/windmill-worker/src/python_executor.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<String> = 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());
Expand Down Expand Up @@ -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<sqlx::Postgres>,
occupancy_metrics: &mut Option<&mut OccupancyMetrics>,
) -> windmill_common::error::Result<String> {
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>,
Expand Down Expand Up @@ -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 {
Expand Down Expand Up @@ -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,
Expand Down
36 changes: 30 additions & 6 deletions backend/windmill-worker/src/worker_lockfiles.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<FlowModule>,
job: &QueuedJob,
Expand All @@ -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,
Expand Down Expand Up @@ -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
}
};
Expand All @@ -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::<Vec<String>>()
.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))
}

Expand Down
1 change: 0 additions & 1 deletion flake.nix
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down

0 comments on commit 39c0dd3

Please sign in to comment.