-
Notifications
You must be signed in to change notification settings - Fork 398
feat: Support for optional python_site_packages_path
in repodata
#3579
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
base: main
Are you sure you want to change the base?
Conversation
This is likely not a complete implementation for CEP-17 in mamba but may be enough to unblock This is rough and likely needs some work to finish things up but was hoping for initial feedback (or someone else to take over 😄). With these changes I was able to get the field to show up in the output of
|
Hi, thanks for tackling this! The implementation is neat and looks good to me, can you add some test to check this is correctly working? Also running |
python_site_packages_path
in repodata
Hi @jjhelmus, the support of CEP-17 is important for supporting Python 3.13t builds. Are you still interested in continuing this PR? |
I can still work on this but progress will be slow. If anyone wants to take over this work that would be fine. When I original started for on this, supported free-threading was a high priority for the team I was on. Since then, free-threading support is no longer a priority so I have very little time to work on this. |
Hi! I'm reviewing conda/conda-libmamba-solver#628 and in there we are already assuming that |
Add support for the optional python_site_packages_path field that can appear in repodata for python packages to specify the location of the site-packages directory. This optional field is defined in CEP-17.
Add the `python_site_packages_path` attribute to the PackageInfo class in the libmambapy bindings module.
Use the python_site_packages_path parameter from the python package in the environment as the site_package_path when available. When not specified fall back to setting this from the python version.
I found some time to work on this again and have been making some progress. Micromamba will now copy the contents of This can be checked using:
There should be a single directory, I'm still working on adding some tests, should have these complete by the end of the week. |
This PR is ready for review. With these changes, This field is exposed in There are tests at a unit and integration level. Happy to address any concerns. |
Bumping this PR. Is there anything additional needed for a review? |
Bump again. Looking for a review or guidance on if anything is needed to merge this change required to fully support free-threading Python in the conda ecosystem. |
We have other constraints at the moment, but we will try to have a look soon. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your patience and this important contribution, @jjhelmus.
I have a few comment and suggestions.
I think we also need some integration tests for installation and usage of python3.13t
installation, usage and uninstallation with mamba
, in particular in the previous presence of python3.13
(e.g. must normal python packages for python3.13
be uninstalled for safety or are they ignored?)
/** | ||
* Set the python_site_packages_path of the solvable. | ||
* | ||
* This is not used by libsolv and is purely for data storing. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you motivate this API by mentioning some context from CEP-17 for reference?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you extend those tests for some free threading builds of CPython?
pkg.python_site_packages_path = "lib/python3.99t/site-packages" | ||
assert pkg.python_site_packages_path == "lib/python3.99t/site-packages" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you assert its value before it is set?
assert res["query"]["type"] == "search" | ||
assert "conda-forge" in res["result"]["pkgs"][0]["channel"] | ||
assert res["result"]["pkgs"][0]["name"] == "python" | ||
assert res["result"]["pkgs"][0]["version"] == "3.13.1" | ||
assert res["result"]["pkgs"][0]["python_site_packages_path"] == "lib/python3.13t/site-packages" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert res["query"]["type"] == "search" | |
assert "conda-forge" in res["result"]["pkgs"][0]["channel"] | |
assert res["result"]["pkgs"][0]["name"] == "python" | |
assert res["result"]["pkgs"][0]["version"] == "3.13.1" | |
assert res["result"]["pkgs"][0]["python_site_packages_path"] == "lib/python3.13t/site-packages" | |
assert res["query"]["type"] == "search" | |
package_info = res["result"]["pkgs"][0] | |
assert "conda-forge" in package_info["channel"] | |
assert package_info["name"] == "python" | |
assert package_info["version"] == "3.13.1" | |
assert package_info["track_features"] == "free-threading" | |
assert package_info["python_site_packages_path"] == "lib/python3.13t/site-packages" |
assert os.path.isdir(env_prefix) | ||
|
||
if platform.system() == "Windows": | ||
assert os.path.isdir(env_prefix / "lib" / "site-packages" / "imagesize") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So Windows stores all packages together indifferently from the version of Python and now from the free threaded builds?
Can explicit tests be added for python=3.13t
for Windows for the presence or absence of folders?
helpers.install("-n", env_name, "imagesize") | ||
|
||
if helpers.platform.system() == "Windows": | ||
assert os.path.isdir(env_prefix / "lib" / "site-packages" / "imagesize") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as above.
@@ -206,6 +206,22 @@ namespace solv | |||
return set_license(str.c_str()); | |||
} | |||
|
|||
auto ObjSolvableViewConst::python_site_packages_path() const -> std::string_view | |||
{ | |||
return ptr_to_strview(::solvable_lookup_str(const_cast<::Solvable*>(raw()), SOLVABLE_MEDIABASE) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for working on this!
One concern (raised in this issue) would be that SOLVABLE_MEDIABASE
seems to be used for the channel here.
channel
and set_channel
don't seem to be used though other than in the tests, but still, I'm not sure how using that placeholder could interfere with what libsolv
could be doing...
Maybe @AntoinePrv has more insight?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! Basically for most attribute we use (other name name, version...), this is only a key:value store and has little importance which key we use (the point of these methods is to hide them as implementation details).
I think since for the channel it is used with another struct, it does not matter. Alternatively one could use another obscure key with proper comment.
Hi @AntoinePrv! We're looking to merge this in conda and conda-libmamba-solver, and I wondered if there is a chance we can merge this soon? |
I may have some time to look at this in the next two days but if not I will not be able to update this until the week of June 16. |
Add support for the optional
python_site_packages_path
field that can appear in repodata forpython
packages as a means to specify the location of the site-packages directory.This optional field is defined in CEP-17.
partial implementation for #3558
Please be kind as this is my first contribution to mamba and the first time digging into the source.