Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Some Python improvements. #1748

Merged
merged 4 commits into from
Dec 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 14 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,10 +1,23 @@
# Changelog

## Unreleased

#### 🐞 Fixes

- Fixed `moon ci` showing incorrect job related logs.
- Fixed some issues with the Python toolchain:
- pip is no longer required to be enabled to activate a virtual environment.
- Changed `python.rootRequirementsOnly` to `false` by default.
- The venv root is now the location of a found `requirements.txt`, otherwise the package root, or
workspace root if `python.rootRequirementsOnly` is enabled.
- Tasks will now inherit the correct venv paths in `PATH`.

## 1.30.3

#### 🐞 Fixes

- Fixed an issue where a task with explicit no inputs (`inputs: []`) would always be marked as affected.
- Fixed an issue where a task with explicit no inputs (`inputs: []`) would always be marked as
affected.

#### ⚙️ Internal

Expand Down
2 changes: 1 addition & 1 deletion crates/app/src/commands/ci.rs
Original file line number Diff line number Diff line change
Expand Up @@ -175,7 +175,7 @@ fn distribute_targets_across_jobs(

let job_index = args.job.unwrap_or_default();
let job_total = args.job_total.unwrap_or_default();
let batch_size = (targets.len() + job_total - 1) / job_total;
let batch_size = targets.len().div_ceil(job_total);
let batched_targets;

console.print_header("Distributing targets across jobs")?;
Expand Down
4 changes: 4 additions & 0 deletions crates/cli/tests/run_python_test.rs
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,10 @@ fn runs_install_deps_via_args() {
}),
..PartialPythonConfig::default()
});

// Needed for venv
sandbox.create_file("base/requirements.txt", "");

let assert = sandbox.run_moon(|cmd| {
cmd.arg("run").arg("python:poetry");
});
Expand Down
Original file line number Diff line number Diff line change
@@ -1,10 +1,8 @@
---
source: crates/cli/tests/run_python_test.rs
assertion_line: 60
expression: assert.output()
snapshot_kind: text
---
▪▪▪▪ activate virtual environment
▪▪▪▪ python venv
▪▪▪▪ pip install
▪▪▪▪ python:poetry
Poetry (version 1.8.4)
Expand Down
1 change: 0 additions & 1 deletion crates/config/src/toolchain/python_config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@ pub struct PythonConfig {

/// Assumes only the root `requirements.txt` is used for dependencies.
/// Can be used to support the "one version policy" pattern.
#[setting(default = true)]
pub root_requirements_only: bool,

/// Defines the virtual environment name, which will be created in the workspace root.
Expand Down
1 change: 1 addition & 0 deletions crates/toolchain/src/detect/languages.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ pub static PYTHON: StaticStringList = &[
"pyproject.toml",
".pylock.toml",
".python-version",
".venv",
// pip
"Pipfile",
"Pipfile.lock",
Expand Down
2 changes: 1 addition & 1 deletion crates/toolchain/src/detect/task_platform.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ pub fn detect_task_platform(command: &str, enabled_platforms: &[PlatformType]) -
}

if PYTHON_COMMANDS
.get_or_init(|| Regex::new("^(python|python3|pip|pip3)$").unwrap())
.get_or_init(|| Regex::new("^(python|python3|python-3|pip|pip3|pip-3)$").unwrap())
.is_match(command)
{
return use_platform_if_enabled(PlatformType::Python, enabled_platforms);
Expand Down
54 changes: 26 additions & 28 deletions legacy/python/platform/src/actions/install_deps.rs
Original file line number Diff line number Diff line change
@@ -1,44 +1,42 @@
use moon_action::Operation;
use moon_console::{Checkpoint, Console};
use moon_python_tool::PythonTool;
use moon_python_tool::{find_requirements_txt, PythonTool};
use std::path::Path;

use crate::find_requirements_txt;

pub async fn install_deps(
python: &PythonTool,
workspace_root: &Path,
working_dir: &Path,
console: &Console,
) -> miette::Result<Vec<Operation>> {
let mut operations = vec![];
let requirements_path = find_requirements_txt(working_dir, workspace_root);

if let Some(pip_config) = &python.config.pip {
let requirements_path = find_requirements_txt(working_dir, workspace_root);
let virtual_environment = if python.config.root_requirements_only {
workspace_root.join(&python.config.venv_name)
} else {
working_dir.join(&python.config.venv_name)
};
let venv_root = if python.config.root_requirements_only {
workspace_root.join(&python.config.venv_name)
} else {
requirements_path
.as_ref()
.and_then(|rp| rp.parent())
.unwrap_or(working_dir)
.join(&python.config.venv_name)
};

if !virtual_environment.exists() {
console
.out
.print_checkpoint(Checkpoint::Setup, "activate virtual environment")?;
if !venv_root.exists() && requirements_path.is_some() {
console
.out
.print_checkpoint(Checkpoint::Setup, "python venv")?;

let args = vec![
"-m",
"venv",
virtual_environment.to_str().unwrap_or_default(),
];
let args = vec!["-m", "venv", venv_root.to_str().unwrap_or_default()];

operations.push(
Operation::task_execution(format!("python {}", args.join(" ")))
.track_async(|| python.exec_python(args, workspace_root))
.await?,
);
}
operations.push(
Operation::task_execution(format!("python {}", args.join(" ")))
.track_async(|| python.exec_python(args, working_dir, workspace_root))
.await?,
);
}

if let Some(pip_config) = &python.config.pip {
let mut args = vec![];

// Add pip installArgs, if users have given
Expand All @@ -47,8 +45,8 @@ pub async fn install_deps(
}

// Add requirements.txt path, if found
if let Some(req) = &requirements_path {
args.extend(["-r", req.to_str().unwrap_or_default()]);
if let Some(reqs_path) = requirements_path.as_ref().and_then(|req| req.to_str()) {
args.extend(["-r", reqs_path]);
}

if !args.is_empty() {
Expand All @@ -60,7 +58,7 @@ pub async fn install_deps(

operations.push(
Operation::task_execution(format!("python {}", args.join(" ")))
.track_async(|| python.exec_python(args, working_dir))
.track_async(|| python.exec_python(args, working_dir, workspace_root))
.await?,
);
}
Expand Down
7 changes: 0 additions & 7 deletions legacy/python/platform/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,3 @@ mod python_platform;
mod toolchain_hash;

pub use python_platform::*;

use starbase_utils::fs;
use std::path::{Path, PathBuf};

fn find_requirements_txt(starting_dir: &Path, workspace_root: &Path) -> Option<PathBuf> {
fs::find_upwards_until("requirements.txt", starting_dir, workspace_root)
}
16 changes: 7 additions & 9 deletions legacy/python/platform/src/python_platform.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::{actions, find_requirements_txt, toolchain_hash::PythonToolchainHash};
use crate::{actions, toolchain_hash::PythonToolchainHash};
use moon_action::Operation;
use moon_action_context::ActionContext;
use moon_common::{path::is_root_level_source, Id};
Expand All @@ -12,7 +12,7 @@ use moon_platform::{Platform, Runtime, RuntimeReq};
use moon_process::Command;
use moon_project::Project;
use moon_python_lang::pip_requirements::load_lockfile_dependencies;
use moon_python_tool::{get_python_tool_paths, PythonTool};
use moon_python_tool::{find_requirements_txt, get_python_tool_paths, PythonTool};
use moon_task::Task;
use moon_tool::{get_proto_version_env, prepend_path_env_var, Tool, ToolManager};
use moon_utils::async_trait;
Expand Down Expand Up @@ -294,16 +294,14 @@ impl Platform for PythonPlatform {

if let Ok(python) = self.toolchain.get_for_version(&runtime.requirement) {
if let Some(version) = get_proto_version_env(&python.tool) {
let cwd = if python.config.root_requirements_only {
self.workspace_root.as_path()
} else {
working_dir
};

command.env("PROTO_PYTHON_VERSION", version);
command.env(
"PATH",
prepend_path_env_var(get_python_tool_paths(python, cwd)),
prepend_path_env_var(get_python_tool_paths(
python,
working_dir,
&self.workspace_root,
)),
);
}
}
Expand Down
26 changes: 20 additions & 6 deletions legacy/python/tool/src/python_tool.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,25 @@ use moon_toolchain::RuntimeReq;
use proto_core::flow::install::InstallOptions;
use proto_core::{Id, ProtoEnvironment, Tool as ProtoTool, UnresolvedVersionSpec};
use rustc_hash::FxHashMap;
use starbase_utils::fs;
use std::path::PathBuf;
use std::sync::Arc;
use std::{ffi::OsStr, path::Path};
use tracing::instrument;

pub fn get_python_tool_paths(python_tool: &PythonTool, working_dir: &Path) -> Vec<PathBuf> {
let venv_python = working_dir.join(&python_tool.config.venv_name);
pub fn find_requirements_txt(starting_dir: &Path, workspace_root: &Path) -> Option<PathBuf> {
fs::find_upwards_until("requirements.txt", starting_dir, workspace_root)
}

if venv_python.exists() {
vec![venv_python.join("Scripts"), venv_python.join("bin")]
pub fn get_python_tool_paths(
python_tool: &PythonTool,
working_dir: &Path,
workspace_root: &Path,
) -> Vec<PathBuf> {
if let Some(venv_root) =
fs::find_upwards_until(&python_tool.config.venv_name, working_dir, workspace_root)
{
vec![venv_root.join("Scripts"), venv_root.join("bin")]
} else {
get_proto_paths(&python_tool.proto_env)
}
Expand Down Expand Up @@ -68,7 +77,12 @@ impl PythonTool {
}

#[instrument(skip_all)]
pub async fn exec_python<I, S>(&self, args: I, working_dir: &Path) -> miette::Result<()>
pub async fn exec_python<I, S>(
&self,
args: I,
working_dir: &Path,
workspace_root: &Path,
) -> miette::Result<()>
where
I: IntoIterator<Item = S>,
S: AsRef<OsStr>,
Expand All @@ -78,7 +92,7 @@ impl PythonTool {
.envs(get_proto_env_vars())
.env(
"PATH",
prepend_path_env_var(get_python_tool_paths(self, working_dir)),
prepend_path_env_var(get_python_tool_paths(self, working_dir, workspace_root)),
)
.cwd(working_dir)
.with_console(self.console.clone())
Expand Down
1 change: 1 addition & 0 deletions tests/fixtures/python/base/.gitignore
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
.venv
7 changes: 3 additions & 4 deletions website/docs/config/toolchain.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -758,19 +758,18 @@ Python installation's are based on pre-built binaries provided by
Supports the "single version policy" or "one version rule" patterns by only allowing dependencies in
the root `requirements.txt`, and only installing dependencies in the workspace root, and not within
individual projects. It also bypasses all `workspaces` checks to determine package locations.
Defaults to `true`.

```yaml title=".moon/toolchain.yml" {2}
python:
rootRequirementsOnly: false
rootRequirementsOnly: true
```

### `venvName`

<HeadingApiLink to="/api/types/interface/PythonConfig#venvName" />

Defines the virtual environment file name, which will be created in the workspace or project root,
and where dependencies will be installed into. Defaults to `.venv`
Defines the virtual environment name, which will be created in the workspace or project root when a
`requirements.txt` exists, and where dependencies will be installed into. Defaults to `.venv`

```yaml title=".moon/toolchain.yml" {2}
python:
Expand Down
Loading