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

Use python_site_packages_path when specified #14256

Merged
merged 28 commits into from
Nov 8, 2024
Merged

Conversation

jjhelmus
Copy link
Contributor

@jjhelmus jjhelmus commented Sep 20, 2024

I'm putting this up as an example and for discussion. More work is needed for a complete implementation, especially given that this one breaks backwards compatibility in various function.

Description

When the python package in an environment specifies a python_site_packages_path in its repodata entry, use that path to install noarch: python packages.

Note that this currently only works with the classic solver as libmamba/conda-libmamba-solver does not yet understand this field.

Close #14053.

Checklist - did you ...

  • Add a file to the news directory (using the template) for the next release's release notes?
  • Add / update necessary tests?
  • Add / update outdated documentation?

@jjhelmus
Copy link
Contributor Author

An example of this:

❯ conda create -n example -c jjhelmus/label/sp_path -c conda-forge python=3.99.90 imagesize --solver classic --yes
Collecting package metadata (current_repodata.json): done
Solving environment: done

## Package Plan ##

  environment location: /Users/jhelmus/workspace/conda/devenv/Darwin/arm64/envs/devenv-3.10-c/envs/example

  added / updated specs:
    - imagesize
    - python=3.99.90


The following NEW packages will be INSTALLED:

  imagesize          conda-forge/noarch::imagesize-1.4.1-pyhd8ed1ab_0 
  python             jjhelmus/label/sp_path/noarch::python-3.99.90-0 



Downloading and Extracting Packages:

Preparing transaction: done
Verifying transaction: done
Executing transaction: done
#
# To activate this environment, use
#
#     $ conda activate example
#
# To deactivate an active environment, use
#
#     $ conda deactivate

❯ tree /Users/jhelmus/workspace/conda/devenv/Darwin/arm64/envs/devenv-3.10-c/envs/example
/Users/jhelmus/workspace/conda/devenv/Darwin/arm64/envs/devenv-3.10-c/envs/example
├── conda-meta
│   ├── history
│   ├── imagesize-1.4.1-pyhd8ed1ab_0.json
│   └── python-3.99.90-0.json
└── sample_path
    └── from_the_file
        └── index_json
            ├── imagesize
            │   ├── __init__.py
            │   └── imagesize.py
            └── imagesize-1.4.1.dist-info
                ├── INSTALLER
                ├── LICENSE.rst
                ├── METADATA
                ├── RECORD
                ├── REQUESTED
                ├── WHEEL
                ├── direct_url.json
                └── top_level.txt

7 directories, 13 files

The python package used here was created with conda-build that includes the change from conda/conda-build#5502.

@conda-bot conda-bot added the cla-signed [bot] added once the contributor has signed the CLA label Sep 20, 2024
@jezdez jezdez changed the title DRAFT: use python_site_packages_path when specified Use python_site_packages_path when specified Sep 21, 2024
@jezdez jezdez changed the title Use python_site_packages_path when specified Use python_site_packages_path when specified Sep 21, 2024
Copy link

codspeed-hq bot commented Nov 4, 2024

CodSpeed Performance Report

Merging #14256 will not alter performance

Comparing jjhelmus:sp_dir (adbb7b9) with main (979716c)

Summary

✅ 21 untouched benchmarks

@jezdez jezdez self-assigned this Nov 4, 2024
@jjhelmus jjhelmus marked this pull request as ready for review November 4, 2024 22:34
@jjhelmus jjhelmus requested a review from a team as a code owner November 4, 2024 22:34
jezdez
jezdez previously approved these changes Nov 5, 2024
@jjhelmus
Copy link
Contributor Author

jjhelmus commented Nov 5, 2024

In ea89bc0 I added unit test to validate the noarch: python packages get installed into the path specified by the python_site_packages_path field. This uses one of the sample, empty packages made in conda/conda-build#5502 which is in my channel on anaconda.org. I'd be in favor or a more robust solution for testing if there are any suggestions.

@jezdez
Copy link
Member

jezdez commented Nov 5, 2024

@jjhelmus You're right, we're trying to move away from relying on hosted pacakges, and have added test_recipes_channel pytest fixture (which is used in a number of places) that points the tests to the tests/test-recipes directory, in which we basically store slimmed down variants of packages for testing purposes, that we converted in #12879.

Could you add a custom python package to the directory and rely on it in your test?

cc @kenodegard since you may thoughts on this one

@jjhelmus
Copy link
Contributor Author

jjhelmus commented Nov 5, 2024

Could you add a custom python package to the directory and rely on it in your test?

This is exactly what I was looking for. I'll update the PR to use this fixture and add the necessary packages. Thanks much.

@jjhelmus
Copy link
Contributor Author

jjhelmus commented Nov 5, 2024

Test was updated to use the test_recipes_channel pytest fixture with two embedded packages.

@jjhelmus
Copy link
Contributor Author

jjhelmus commented Nov 6, 2024

The single failing test, FAILED tests/core/test_solve.py::test_solve_1[libmamba] looks unrelated to this change.

@jjhelmus
Copy link
Contributor Author

jjhelmus commented Nov 6, 2024

In order for this field to be usable with the libmamba solver, libmamba-conda-solver needs to pass this field, see conda/conda-libmamba-solver#560. This in turn requires mamba to support this field (mamba-org/mamba#3558) which mamba-org/mamba#3579 is a start of.

@jezdez
Copy link
Member

jezdez commented Nov 7, 2024

@jaimergp do you feel like this should be bumped to January to wait for the CLS implementation?

@jaimergp
Copy link
Contributor

jaimergp commented Nov 7, 2024

Not sure if we want to wait two months "just in case". There's a chance that the mamba folks release that sooner than the January release, and in that case users would only need to update the libmamba version (I assume conda-libmamba-solver would be compatible with the new field by then; if it's not yet, then the necessary PR would be trivial). All of this makes me think that no, it's not necessary.

That said:

  • The mamba PR is targeting v2, which is not even on defaults yet (is there a request already up?).
  • Do we want libmamba v1 (and thus conda-libmamba-solver <=24.9) compatibility?

@jezdez
Copy link
Member

jezdez commented Nov 8, 2024

Not sure if we want to wait two months "just in case". There's a chance that the mamba folks release that sooner than the January release, and in that case users would only need to update the libmamba version (I assume conda-libmamba-solver would be compatible with the new field by then; if it's not yet, then the necessary PR would be trivial). All of this makes me think that no, it's not necessary.

I concur, let's merge this, since it would only start to work if we add the field anyways

That said:

  • The mamba PR is targeting v2, which is not even on defaults yet (is there a request already up?).

I just opened the package update request

  • Do we want libmamba v1 (and thus conda-libmamba-solver <=24.9) compatibility?

My guess is that this won't be possible, since the mamba team unlikely will want to add new features to 1.x right?

@jezdez jezdez merged commit 113d9ee into conda:main Nov 8, 2024
62 checks passed
@ForgottenProgramme ForgottenProgramme mentioned this pull request Nov 18, 2024
30 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed [bot] added once the contributor has signed the CLA
Projects
Status: 🏁 Done
5 participants