Fix canonical name for local channels in 1.x #3056

isuruf · 2023-12-11T17:11:04Z

isuruf · 2023-12-11T17:11:16Z

jaimergp · 2023-12-11T17:21:52Z

This is one of those moments where one can only say "really?!". 😂 Thanks, I'll double check with conda-libmamba-solver later today!

jaimergp · 2023-12-11T18:10:26Z

Hm, can we add a test? I have applied these changes locally but it doesn't seem to fix the local::spec problem with conda-libmamba-solver. See conda/conda-libmamba-solver#401 for reference.

jaimergp · 2023-12-12T09:04:40Z

Ah, ok, it does work, but there's a gotcha compared to conda.

In conda, local refers to these paths:

                    for d in (
                        self._croot,
                        self.bld_path,
                        self.conda_build.get("root-dir"),
                        join(self.root_prefix, "conda-bld"),
                        "~/conda-bld",
                    )

So:

The value of bld_path in .condarc or CONDA_BLD_PATH env var
The value of conda_build.root-dir in .condarc
$CONDA_ROOT/conda-bld
~/conda-bld

In mamba, it hardcodes this list:

$TARGET_PREFIX/conda-bld
$ROOT_PREFIX/conda-bld
~/conda-bld

So, in order for conda to get a package from local, we need CONDA_BLD_PATH to be symlinked into those hardcoded locations.

I've tried redefining local via context.custom_multichannels, but I think we arrive too late. If I comment this block out, it does allow the custom (re)definition:

mamba/libmamba/src/core/channel.cpp

Lines 738 to 758 in 4c012a6

    
           // Local channels 
        
           std::vector<std::string> local_channels = { 
        
               Context::instance().prefix_params.target_prefix.string() + "/conda-bld", 
        
               Context::instance().prefix_params.root_prefix.string() + "/conda-bld", 
        
               "~/conda-bld" 
        
           }; 
        
           std::vector<std::string> local_names; 
        
           local_names.reserve(local_channels.size()); 
        
           for (const auto& p : local_channels) 
        
           { 
        
               if (fs::is_directory(p)) 
        
               { 
        
                   std::string url = util::path_to_url(p); 
        
                   auto channel = make_simple_channel(m_channel_alias, url, "", LOCAL_CHANNELS_NAME); 
        
                   std::string name = channel.name(); 
        
                   auto res = m_custom_channels.emplace(std::move(name), std::move(channel)); 
        
                   local_names.push_back(res.first->first); 
        
               } 
        
           } 
        
           m_custom_multichannels.emplace(LOCAL_CHANNELS_NAME, std::move(local_names));

So I wonder if we can add a check to only instantiate the hardcoded local channels if context.custom_multichannels does not define local. WDYT @AntoinePrv @JohanMabille?

Btw, if that were to happen, I think we also need the same fix a few lines below, @isuruf. Just add url here in the same fashion:

mamba/libmamba/src/core/channel.cpp

Line 780 in 4c012a6

auto channel = make_simple_channel(m_channel_alias, url, "", multichannelname);

isuruf · 2023-12-12T15:51:10Z

custom_multichannels is not supposed override local though according to the docs at https://docs.conda.io/projects/conda/en/latest/configuration.html

jaimergp · 2023-12-12T19:13:33Z

We could only allow that via the API, not the user configuration. e.g. mamba has this check:

mamba/mamba/mamba/utils.py

Line 248 in 4c012a6

if el not in ["defaults", "local"]:

(I'm not sure where micromamba would define that behavior).

After all, I just want to inject the customizable conda's local into libmamba's hardcoded local.

Maybe it's a matter of adding the conda_build_root stuff to context too, but that feels like a bigger lift for an arguably niche feature (I hope very few folks are defining custom_multichannels to begin with, and even fewer trying to redefine local).

We could also use a secret key name if that's preferred, like _local_override or something.

AntoinePrv · 2023-12-15T16:21:40Z

@jaimergp I'm happy to add any hooks that will let you add what you need (a private private_conda_bld_paths in Context ?). In 2.0 the behavior is totally flexible from an API standpoint: local is a custom_multichannel that must be configured by the developer.

jaimergp · 2023-12-17T18:40:12Z

(a private private_conda_bld_paths in Context ?)

That would be awesome @AntoinePrv! Whatever's easier from your point of view.

isuruf · 2023-12-18T08:32:31Z

libmamba/src/core/context.cpp

+            return { prefix_params.target_prefix.string() + "/conda-bld",
+                     prefix_params.root_prefix.string() + "/conda-bld",
+                     "~/conda-bld" };


Here target_prefix is used and therefore we can't initialize the variable in the Context creation time and therefore we need to use a getter function here.

isuruf · 2023-12-18T08:33:01Z

libmambapy/src/main.cpp

@@ -543,6 +543,11 @@ PYBIND11_MODULE(bindings, m)
        .def_readwrite("custom_channels", &Context::custom_channels)
        .def_readwrite("custom_multichannels", &Context::custom_multichannels)
        .def_readwrite("default_channels", &Context::default_channels)
+        .def_property(
+            "conda_build_local_paths",


I'm using conda_build_local_paths because conda has a read only property with the same name.

isuruf · 2023-12-20T09:33:44Z

This is ready for a review.

JohanMabille · 2023-12-20T11:38:07Z

Thanks @isuruf for this fix! The failure on Windows is unrelated to this change, see #3064

JohanMabille · 2023-12-20T11:45:27Z

@jaimergp if the setter set_conda_build_local_paths, is enough for your need, I can merge this and release a 1.5.6 today.

jaimergp · 2023-12-20T11:48:27Z

Thanks @JohanMabille, that would be awesome! Let me run a quick test locally to make sure this works with conda-libmamba-solver and I'll give you the thumbs up!

jaimergp · 2023-12-20T12:00:34Z

Ok, conda/conda-libmamba-solver#401 passes locally with this branch so we have a beautiful green! @JohanMabille, you may proceed. Thanks to @isuruf too!

mbargull · 2023-12-23T02:02:19Z

libmamba/src/core/context.cpp

+    {
+        if (conda_build_local_paths.empty())
+        {
+            return { prefix_params.target_prefix.string() + "/conda-bld",


I don't know if it has been intentional, but this differs from what conda.base.context.conda_build_local_paths would have:
https://github.com/conda/conda/blob/23.11.0/conda/base/context.py#L505-L523

This adds non-existent folders whereas conda only adds existing ones.

bld_path config value (and as such ${CONDA_BLD_PATH}) is not used.

${CONDA_PREFIX}/conda-bld is added whereas conda would never add this
(unless it's the root prefix which always considered).

conda_build: root-dir config values is not used.

TBH, I didn't even know that conda_build: root-dir existed -- and also not that conda-build uses that config entry but not bld_path (it only checks the environment variable):
https://github.com/conda/conda-build/blob/3.28.2/conda_build/config.py#L467-L468

So, yeah, the behavior in conda/conda-build is a bit convoluted..
But at least the ${CONDA_PREFIX}/conda-bld addition and ${CONDA_BLD_PATH} omission here seem rather unexpected to me.

This keeps the existing paths in mamba with the option to change this from the API. In conda-libmamba-solver, we set these values from conda's context, so they will be optional. So, if mamba is used from conda-libmamba-solver, the behaviour will be the same. I agree that it differs when using mamba directly.

Ah, didn't see @jaimergp's comment in #3056 (comment) which iterates the same points.
So, why do we want different behaviors between the two implementations?

This keeps the existing paths in mamba with the option to change this from the API. In conda-libmamba-solver, we set these values from conda's context, so they will be optional. So, if mamba is used from conda-libmamba-solver, the behaviour will be the same. I agree that it differs when using mamba directly.

I'd consider this behavior a bug in Mamba and not worth preserving...

I guess in case we assume that we only use this via conda-libmamba-solver with the value overridden, then it's probably just a thing to change for the 2.x versions in main.

Added #3124 to track this.

Fix canonical name for local channels in 1.x

8764016

isuruf mentioned this pull request Dec 11, 2023

Conda 23.10.0 fails to find and install local files with the new solver libmamba on MacOS (M1) conda/conda-libmamba-solver#398

Open

2 tasks

isuruf added 2 commits December 11, 2023 12:14

Add test

034f68c

Update local test

0cb1fb6

jaimergp mentioned this pull request Dec 12, 2023

Investigate local:: specs issues conda/conda-libmamba-solver#401

Merged

3 tasks

isuruf force-pushed the canonical branch 3 times, most recently from 0d079dc to 6c85fc4 Compare December 18, 2023 06:40

isuruf added 2 commits December 18, 2023 13:43

Add conda_build_local_paths to Context to set conda-bld paths

dc6f1e0

no need for configuration

cec76b0

isuruf force-pushed the canonical branch from 030de0f to cec76b0 Compare December 18, 2023 08:14

use a setter as well

43c30d4

isuruf commented Dec 18, 2023

View reviewed changes

Update stub

c5698ef

JohanMabille approved these changes Dec 20, 2023

View reviewed changes

JohanMabille merged commit e2de9db into mamba-org:1.x Dec 20, 2023
16 of 17 checks passed

mbargull reviewed Dec 23, 2023

View reviewed changes

jaimergp mentioned this pull request Jan 12, 2024

local multichannel for libmamba v2 #3124

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix canonical name for local channels in 1.x #3056

Fix canonical name for local channels in 1.x #3056

isuruf commented Dec 11, 2023

isuruf commented Dec 11, 2023

jaimergp commented Dec 11, 2023

jaimergp commented Dec 11, 2023

jaimergp commented Dec 12, 2023

isuruf commented Dec 12, 2023

jaimergp commented Dec 12, 2023 •

edited

Loading

AntoinePrv commented Dec 15, 2023 •

edited

Loading

jaimergp commented Dec 17, 2023

isuruf Dec 18, 2023

isuruf Dec 18, 2023

isuruf commented Dec 20, 2023

JohanMabille commented Dec 20, 2023

JohanMabille commented Dec 20, 2023 •

edited

Loading

jaimergp commented Dec 20, 2023

jaimergp commented Dec 20, 2023

mbargull Dec 23, 2023

isuruf Dec 23, 2023

mbargull Dec 23, 2023

mbargull Dec 23, 2023

mbargull Dec 23, 2023

jaimergp Jan 12, 2024

Fix canonical name for local channels in 1.x #3056

Fix canonical name for local channels in 1.x #3056

Conversation

isuruf commented Dec 11, 2023

isuruf commented Dec 11, 2023

jaimergp commented Dec 11, 2023

jaimergp commented Dec 11, 2023

jaimergp commented Dec 12, 2023

isuruf commented Dec 12, 2023

jaimergp commented Dec 12, 2023 • edited Loading

AntoinePrv commented Dec 15, 2023 • edited Loading

jaimergp commented Dec 17, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

isuruf commented Dec 20, 2023

JohanMabille commented Dec 20, 2023

JohanMabille commented Dec 20, 2023 • edited Loading

jaimergp commented Dec 20, 2023

jaimergp commented Dec 20, 2023

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

jaimergp commented Dec 12, 2023 •

edited

Loading

AntoinePrv commented Dec 15, 2023 •

edited

Loading

JohanMabille commented Dec 20, 2023 •

edited

Loading