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

feat: warn on pypi conda clobbering #1353

Merged
merged 16 commits into from May 17, 2024

Conversation

nichmor
Copy link
Contributor

@nichmor nichmor commented May 8, 2024

pixi will print a warning, if for some reason a conda package was mapped wrong
and we reinstall it again but as a pypi package.

Example:
using this custom mapping https://gist.githubusercontent.com/nichmor/6e0b8965e948536ed278f03aac31feb0/raw/635b0b849b88530a70889c4ef2f861bcb2ecc88e/wrong_mapping.json

and having both mdurl in dependencies and pypi-dependencies

user will see following warning

Screenshot 2024-05-08 at 15 13 32

benchmarks:

[project]
name = "benchmark_hash"
version = "0.1.0"
description = "Add a short description here"
authors = ["nichmor <nmorkotilo@gmail.com>"]
channels = ["conda-forge"]
platforms = ["osx-arm64"]
conda-pypi-map = { "conda-forge" = "https://gist.githubusercontent.com/nichmor/9f2d0e5015f84c639394dfbf1c5d8cee/raw/79a2de856b835c81b2f2e78bb30933690bd7ed0b/wrong_jupterlab_mapping.json"}

[tasks]



[dependencies]
jupyterlab = "*"
detectron2 = "*"


[pypi-dependencies]
rich = "*"

with pixi.lock already present and warm cache

for ahash:

(default) graf@Nichitas-MacBook-Pro benchmark_hash % hyperfine '../../target/release/pixi install' --prepare 'rm -rf .pixi'
Benchmark 1: ../../target/release/pixi install
  Time (mean ± σ):     12.035 s ±  3.149 s    [User: 4.990 s, System: 21.933 s]
  Range (min … max):   10.001 s … 20.496 s    10 runs

for std::HashMap


(default) graf@Nichitas-MacBook-Pro benchmark_hash % hyperfine '../../target/release/pixi install' --prepare 'rm -rf .pixi'
Benchmark 1: ../../target/release/pixi install
  Time (mean ± σ):     10.753 s ±  0.611 s    [User: 4.972 s, System: 21.655 s]
  Range (min … max):    9.964 s … 11.517 s    10 runs

for std::HashMap with pre-allocated map:

(default) graf@Nichitas-MacBook-Pro benchmark_hash % hyperfine '../../target/release/pixi install' --prepare 'rm -rf .pixi'
Benchmark 1: ../../target/release/pixi install
  Time (mean ± σ):     11.482 s ±  2.112 s    [User: 4.955 s, System: 21.883 s]
  Range (min … max):    9.945 s … 16.980 s    10 runs

for ahash with pre-allocated map:

(default) graf@Nichitas-MacBook-Pro benchmark_hash % hyperfine '../../target/release/pixi install' --prepare 'rm -rf .pixi'
Benchmark 1: ../../target/release/pixi install
  Time (mean ± σ):     10.909 s ±  0.537 s    [User: 4.978 s, System: 21.892 s]
  Range (min … max):   10.065 s … 11.552 s    10 runs

so by using ahash and preallocating the map we have a win of 5% performance increase! ⬆️

without clobbering check:

 (default) graf@Nichitas-MacBook-Pro benchmark_hash % hyperfine 'pixi install' --prepare 'rm -rf .pixi'
Benchmark 1: pixi install
  Time (mean ± σ):     12.127 s ±  1.601 s    [User: 4.844 s, System: 23.112 s]
  Range (min … max):   10.413 s … 16.113 s    10 runs

with clobbering check:

(default) graf@Nichitas-MacBook-Pro benchmark_hash % hyperfine '../../target/release/pixi install' --prepare 'rm -rf .pixi'
Benchmark 1: ../../target/release/pixi install
  Time (mean ± σ):     12.241 s ±  2.749 s    [User: 5.005 s, System: 22.088 s]
  Range (min … max):   10.330 s … 19.690 s    10 runs

we add overhead of 0.100s

@nichmor nichmor marked this pull request as ready for review May 8, 2024 12:14
Copy link
Contributor

@ruben-arts ruben-arts left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, LGTM

@ruben-arts ruben-arts marked this pull request as draft May 10, 2024 14:04
@nichmor
Copy link
Contributor Author

nichmor commented May 13, 2024

it's missing a warning for the usecase when we first install pypi and later conda which don't remove the pypi one and we end up with two .dist-info in site-packages

@tdejager
Copy link
Contributor

Conflict it seems!

@nichmor nichmor marked this pull request as ready for review May 14, 2024 13:23
src/install_pypi.rs Outdated Show resolved Hide resolved
src/install_pypi.rs Outdated Show resolved Hide resolved
src/install_wheel.rs Show resolved Hide resolved
src/install_wheel.rs Show resolved Hide resolved
src/lock_file/update.rs Outdated Show resolved Hide resolved
src/install_wheel.rs Show resolved Hide resolved
@tdejager
Copy link
Contributor

Nice that you've included benchmarks! Just wondering what the cost is of doing the check as opposed to not doing it :), for these large environments?

@nichmor
Copy link
Contributor Author

nichmor commented May 16, 2024

Nice that you've included benchmarks! Just wondering what the cost is of doing the check as opposed to not doing it :), for these large environments?

added benchmark in the description!
but tl;dr - we add overhead of 0.100 seconds for large env

@nichmor nichmor requested a review from tdejager May 16, 2024 08:03
}
}

pub fn clobber_on_instalation(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you describe what it returns? And why this is important.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

@ruben-arts ruben-arts enabled auto-merge (squash) May 17, 2024 13:42
@ruben-arts ruben-arts merged commit a85a251 into prefix-dev:main May 17, 2024
24 checks passed
chawyehsu pushed a commit to chawyehsu/pixi that referenced this pull request May 17, 2024
Co-authored-by: Ruben Arts <ruben.arts@hotmail.com>
Co-authored-by: Tim de Jager <tdejager89@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants