feat: validate machine using lockfile #2849

ruben-arts · 2025-01-07T15:40:57Z

Why:

This PR tries to make the running of environments more flexible. The system-requirements were previously checked before you could run an environment. This PR changes that to only allow installation of a prefix if all virtual packages are matched with the requirements in the lockfile. This results in a less strict rule, as we don't let the user the requirements (Now this became an even worse name).

Resulting in the following benefits:

Having a system-requirement on cuda would still allow an environment to be installed on Mac.
Having a strict system-requirement while there aren't actually packages that require it, allows the user to still run it.

Manifests that will now work but didn't work before:

[project]
channels = ["https://prefix.dev/conda-forge"]
name = "pytorch-conda-forge"
platforms = ["linux-64", "osx-arm64", "osx-64"]

# This could have been fixed with the `[target.linux.system-requirements]` if that existed but this also works as a result of this feature.
[system-requirements]
cuda = "12.0"

[target.linux.dependencies]
cuda-version = "==12.6"
pytorch-gpu = "*"

[dependencies]
python = "3.12.*"

[project]
channels = ["https://prefix.dev/conda-forge"]
name = "pytorch-conda-forge"
platforms = ["linux-64", "osx-arm64", "osx-64"]

[system-requirements]
libc = "123.0"

[dependencies]
python = "3.12.*"

Draft because:

I broke the logic that tasks can be checked if they run on a system (as there is no promise of a lockfile), not sure what to do yet.
PyPI also needs logic to validate the wheel tags against the machine.
Better testing, which is now even harder as the logic has more dependencies (lockfile)
Get feat: Match GenericVirtualPackage with MatchSpec conda/rattler#1016 in rattler merged and use the release
Run all tests before merging, label: test:extra_slow

…kage_from_lockfile

crates/pypi_modifiers/Cargo.toml

src/cli/run.rs

baszalmstra · 2025-01-10T10:22:05Z

src/cli/run.rs

@@ -249,7 +244,6 @@ fn command_not_found<'p>(project: &'p Project, explicit_environment: Option<Envi
            project
                .environments()
                .into_iter()
-                .filter(|env| verify_current_platform_has_required_virtual_packages(env).is_ok())


Removing this causes all tasks to be shown regardless of whether the current system supports it. Should we maybe call a function here to check that?

baszalmstra · 2025-01-10T10:23:21Z

src/cli/task.rs

-                            if verify_current_platform_has_required_virtual_packages(env).is_ok() {
-                                Some((env.clone(), env.get_filtered_tasks()))
-                            } else {
-                                None
-                            }


Same as my comment above, perhaps we can call a function to see if a certain environment is supported by the current platform.

baszalmstra · 2025-01-10T10:23:27Z

src/cli/task.rs

@@ -465,12 +458,7 @@ fn build_env_feature_task_map(project: &Project) -> Vec<EnvTasks> {
        .environments()
        .iter()
        .sorted_by_key(|env| env.name().to_string())
-        .filter_map(|env: &Environment<'_>| {


src/lock_file/update.rs

src/lock_file/virtual_packages.rs

baszalmstra · 2025-01-10T10:28:17Z

src/lock_file/virtual_packages.rs

+    let conda_records = environment
+        .conda_repodata_records(platform)
+        .map_err(VirtualPackageError::RepodataConversionError)?
+        .ok_or(VirtualPackageError::NoCondaRecordsFound(platform))?


Is that bad? It means there is an environment but that specific environment is empty. I guess its installable then?

I actually have a legitimate use case for empty environments: tasks that use software not managed by pixi.

We should indeed not error on an empty environment, I'm also a big user of that as a feature ;)

src/lock_file/virtual_packages.rs

baszalmstra · 2025-01-10T12:28:23Z

src/task/task_environment.rs

@@ -150,11 +148,6 @@ impl<'p, D: TaskDisambiguation<'p>> SearchEnvironments<'p, D> {
                    .iter()
                    // Filter out default environment
                    .filter(|env| !env.name().is_default())
-                    // Filter out environments that can not run on this machine.
-                    .filter(|env| {
-                        self.ignore_system_requirements


I think this should be added back somehow.

I currently implemented it in such a way that it only checks the platform but this would ambigue the cuda vs non cuda tasks.... I think I'll still have to fix that.

…kage_from_lockfile

Co-authored-by: Bas Zalmstra <[email protected]>

…kage_from_lockfile

feat: validate machine using lockfile

057b991

ruben-arts added the UX Related to the User Experience of pixi label Jan 7, 2025

ruben-arts added 5 commits January 8, 2025 16:25

feat: add pypi wheel tag resolution to validate the tags

dd4cc7a

wip: non compiling

178ef6d

feat: implement the pypi tag validation

341c1ee

chore: improve error handling

ba4cdbc

test: add a integration test to validate the new behavior

01868b1

ruben-arts marked this pull request as draft January 9, 2025 15:43

ruben-arts added the test:extra_slow Run the extra slow tests label Jan 10, 2025

ruben-arts added 2 commits January 10, 2025 10:53

update rattler

f468843

Merge remote-tracking branch 'upstream/main' into feature/virtual_pac…

fd05cb5

…kage_from_lockfile

baszalmstra reviewed Jan 10, 2025

View reviewed changes

ruben-arts and others added 18 commits January 14, 2025 14:03

Merge remote-tracking branch 'upstream/main' into feature/virtual_pac…

41b7c52

…kage_from_lockfile

Update src/cli/run.rs

f13dbdc

Co-authored-by: Bas Zalmstra <[email protected]>

fix: improve code flow

54b4578

fix: remove unused snaps

f299376

fix: task commands

3511d24

Merge remote-tracking branch 'upstream/main' into feature/virtual_pac…

92eda43

…kage_from_lockfile

fix: tests

78816fa

Merge branch 'main' into feature/virtual_package_from_lockfile

624d812

test: make test multi platform

01c9802

Merge branch 'main' into feature/virtual_package_from_lockfile

ad4940f

fix test

9368a67

remove glibc requirement to specifically test wheel tags

da5aecf

fix: empty lockfile should not fail

212e001

fix: empty lockfile should not fail

6d53b42

misc: add more tracing to help future debugging

c65e2c6

misc: improve logging

104bba7

fix: wheel test on windows

4ba402b

fix: remove warnings

9f7a355

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: validate machine using lockfile #2849

feat: validate machine using lockfile #2849

ruben-arts commented Jan 7, 2025 •

edited

Loading

baszalmstra Jan 10, 2025

baszalmstra Jan 10, 2025

baszalmstra Jan 10, 2025

baszalmstra Jan 10, 2025

akhmerov Jan 15, 2025

ruben-arts Jan 24, 2025

baszalmstra Jan 10, 2025

ruben-arts Jan 14, 2025

feat: validate machine using lockfile #2849

Are you sure you want to change the base?

feat: validate machine using lockfile #2849

Conversation

ruben-arts commented Jan 7, 2025 • edited Loading

Why:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ruben-arts commented Jan 7, 2025 •

edited

Loading