Overwriting channels in features broken in v0.21.0

[hoxbro]

Checks

• I have checked that this issue has not already been reported.

• I have confirmed this bug exists on the latest version of pixi, using pixi --version.

Reproducible example

pixi install
cat pixi.lock | grep datashader

      - conda: https://conda.anaconda.org/pyviz/label/dev/noarch/datashader-0.16.1-py_0.tar.bz2
      - conda: https://conda.anaconda.org/rapidsai/noarch/datashader-0.13.1a-py_0.tar.bz2
  name: datashader
  url: https://conda.anaconda.org/rapidsai/noarch/datashader-0.13.1a-py_0.tar.bz2
  name: datashader
  url: https://conda.anaconda.org/pyviz/label/dev/noarch/datashader-0.16.1-py_0.tar.bz2

[project]
name = "holoviews"
channels = ["pyviz/label/dev", "conda-forge"]
platforms = ["linux-64"]

[environments]
test = ["test"]
test-gpu = ["test", "test-gpu"]

[feature.test.dependencies]
python = "3.11.*"
datashader = ">=0.11.1"

[feature.test-gpu]
channels = ["pyviz/label/dev", "rapidsai", "conda-forge"]

[feature.test-gpu.dependencies]
cuda-version = "12.2.*"

Issue description

After upgrading to 0.21, my channel priorities seem messed up when combining features.

Expected behavior

I would expect both environments to have datashader 0.16.1 installed from the pyviz/label/dev

[ruben-arts]

You are correct. It looks like the index logic is not the same.
I can take a look at this tomorrow or @olivier-lacroix possibly has an idea already.

[ruben-arts]

The culprit is situated in the channel list creation logic.

pixi/src/project/has_features.rs

Line 27 in c8ff8e2

fn channels(&self) -> IndexSet<&'p Channel> {

Here is a test that reproduces the issue in simple terms:

	#[test]
    fn test_channel_feature_priority_with_redifinition(){
        let manifest = Project::from_str(
            Path::new("pixi.toml"),
            r#"
        [project]
        name = "holoviews"
        channels = ["a", "b"]
        platforms = ["linux-64"]

        [environments]
        foo = ["foo"]

        [feature.foo]
        channels = ["a", "c", "b"]
        "#,
        )
            .unwrap();

        let foobar_channels = manifest.environment("default").unwrap().channels();
        assert_eq!(
            foobar_channels
                .into_iter()
                .map(|c| c.name.clone().unwrap())
                .collect_vec(),
            vec!["a", "b"]
        );
        let foo_channels = manifest.environment("foo").unwrap().channels();
        assert_eq!(
            foo_channels
                .into_iter()
                .map(|c| c.name.clone().unwrap())
                .collect_vec(),
            vec!["a", "c", "b"]
        );
    }

This will fail with:

assertion `left == right` failed
  left: ["c", "a", "b"]
 right: ["a", "c", "b"]

Diving deeper 🤿

[olivier-lacroix]

Oopsie. My bad. I was so focused on making sure the default channels end up at the end as they did before that I messed it up. The double rev was a bad idea indeed.

Note the current code will have a different outcome from before, for environments that include several features that do not specify channels. In that case, project level channels will be picked up for these features, and their first instance kept in the final list of channels; Previously, default channels would always end up at the end of the list.

The current behaviour seems Ok to me, but I am not sure why @baszalmstra made sure project default channels were at the end of the list in the first place.

[baszalmstra]

The reason was that features could easily overwrite the order of the default channels without having to add priorities. Since the default feature is implicitly added this made sense to me.