Remove defaults channel on input `channel: nodefaults` #364

liamhuber · 2024-08-26T22:44:53Z

Changes conda setup to run conda config --remove channels defaults when nodefaults is found in the channels input instead of adding the - nodefaults channel. I.e.

- uses: conda-incubator/setup-miniconda
      with:
        channels: conda-forge,nodefaults
        channel-priority: strict

Will correctly give

channels:
- conda-forge
dependencies:
...

Instead of

channels:
- conda-forge
- nodefaults
dependencies:
...

Here is a log of the modified version in action.

Closes #207

liamhuber · 2024-08-27T21:58:32Z

Example 6 during Adding tools to 'base' env...:

  Removing channel defaults
  /home/runner/miniconda3/condabin/conda config --remove channels defaults
  Warning: 
  CondaKeyError: 'channels': 'defaults' is not in the 'channels' key of the config file
  
  
  
  CondaKeyError: 'channels': 'defaults' is not in the 'channels' key of the config file

The patch is working as-intendend and in Applying initial configuration... we correctly get:

  Removing channel defaults
  /home/runner/miniconda3/condabin/conda config --remove channels defaults

But then there's something in the Ex6 config that differs from my simple test case that causes it to double-dip for defaults.

liamhuber · 2024-08-27T22:05:42Z

Ok, got it: the setup runs applyCondaConfiguration a second time. So it's just my own new if-clause come to bite me.

E.g. because it is not there

liamhuber · 2024-08-27T23:32:49Z

Ok, the log shows the nodefaults option correctly purging the defaults channel. After that, defaults gets added back in because it's included explicitly in etc/example-environment.yml. This is a bit odd when nodefaults gets passed to as an action arg, but it is internally consistent with the existing behaviour where the provided env file is the highest authority, so it seems reasonable to me. I'm content with this and feel this is ready for review.

liamhuber · 2024-09-08T04:41:40Z

@conda-incubator/setup-miniconda tests are passing, just waiting on a review.

jaimergp · 2024-09-10T09:49:56Z

dist/setup/index.js

+            core.info("Removing channel defaults");
+            if (channel === "nodefaults") {


Suggested change

core.info("Removing channel defaults");

if (channel === "nodefaults") {

if (channel === "nodefaults") {

core.info("Removing channel defaults");

src/conda.ts

jaimergp

Just a tiny comment with the logging statements. LGTM otherwise.

jezdez · 2024-09-10T11:22:29Z

I don't agree we should further the "nodefaults" workaround in setup-miniconda, it's a bad API since it's not an actual channel and tends to increase the risk of a messy channel management

jezdez · 2024-09-10T11:53:58Z

I think it might be enough to simply offer "override_channels" as a config option, which requires specifying a channel but doesn't hard-code the "nodefaults" workaround.

jezdez

A more explicit and future-proof solution is relying on the --override-channels CLI option since nodefaults is a pretty bad workaround within conda. That way, running conda config --remove channels defaults isn't needed here.

Co-authored-by: jaimergp <[email protected]>

liamhuber · 2024-09-10T16:04:55Z

Just a tiny comment with the logging statements. LGTM otherwise.

@jaimergp, nice catch! Committed 🚀

I don't agree we should further the "nodefaults" workaround in setup-miniconda, it's a bad API since it's not an actual channel and tends to increase the risk of a messy channel management...

@jezdez, the current situation is one "bad" API and a second API that silently deviates from the first API in an unproductive way (i.e. a channel named "nodefaults" is literally added to the channels list). Regardless of whether the conda API is good or bad, I would strongly recommend conda-incubator/setup-miniconda favour consistency over goodness where the two APIs use the same terminology.

I definitely like your suggested --override-channels solution, but I see it as a "yes, and" approach, and I'm afraid you'll need to identify a different volunteer for its implementation.

liamhuber · 2024-09-10T16:07:52Z

Nice, even better, if conda/conda#14227 is merged I think we can even simply close this PR?

jaimergp · 2024-09-10T18:08:38Z

Yep, timing will work this time. The only other workaround right now is to explicitly pass a condarc file correctly populated to the condarc-file input.

liamhuber · 2024-09-10T18:19:13Z

Yep, timing will work this time. The only other workaround right now is to explicitly pass a condarc file correctly populated to the condarc-file input.

Haha, yep, did exactly that downstream a couple weeks ago! pyiron/actions#129

oursland · 2024-09-27T04:08:46Z

Builds are breaking in CI as the package defaults/noarch::conda-libmamba-solver-23.12.0 is being selected, which is incompatible.

liamhuber · 2024-09-27T16:15:07Z

@oursland I'm afraid I don't have enough context to understand your statement. The only thing breaking here was the linter test -- which failed because I never rebuilt the committed ts suggestions into the js.

I did that now, but I also see that conda/conda#14227 got merged last week. @jaimergp my understanding is that now we just need to wait for the next release of conda, and this PR becomes completely unnecessary, yeah? If that's the case I think we can just close this PR.

oursland · 2024-09-27T16:32:18Z

GitHub Actions should not be considered like a normal software release as this is a piece of core CI infrastructure and is not something that can always be deferred until the next release of an upstream package, which will then be packaged into a release, and eventually distribution.

The current behavior of setup-miniconda still breaks GitHub actions. During update, it pulls in an incompatible version of libmamba-solver from the defaults channel. This has been breaking CI for FreeCAD among others.

liamhuber · 2024-09-27T16:53:21Z

GitHub Actions should not be considered like a normal software release as this is a piece of core CI infrastructure and is not something that can always be deferred until the next release of an upstream package, which will then be packaged into a release, and eventually distribution.

I don't think I feel as strongly about it, but I also don't disagree. My opinion doesn't matter though, I'm just a rando who patched what was bothering me. If you want to get this in ASAP you'll need to convince the two reviewers.

If you want something immediately that avoids erroneously including the defaults, you can check out https://github.com/pyiron/actions/blob/main/cached-miniforge/action.yml, a wrapper for setup-miniconda that patches over the defaults issue and solves an edge case in caching (#361)

The current behavior of setup-miniconda still breaks GitHub actions. During update, it pulls in an incompatible version of libmamba-solver from the defaults channel. This has been breaking CI for FreeCAD among others.

I'm afraid I'm a little lost again. Do you mean the main branch of setup-miniconda, or do you mean that you tried out this branch and something I've done here broke the libmamba-solver versioning? If it's the main branch, raising a separate issue and referencing this PR might be the way to go; if it's something I've done here I'll need some assistance tracking down the problem -- at least a MWE for the breakage -- as I don't usually do any work in javascript.

oursland · 2024-09-27T17:35:49Z

I'm afraid I'm a little lost again. Do you mean the main branch of setup-miniconda, or do you mean that you tried out this branch and something I've done here broke the libmamba-solver versioning?

The main branch is broken. This PR fixes the issue. Waiting what could be months for to fix CI is not a reasonable strategy.

Your fix through caching is unfortunately not feasible for our application. For FreeCAD and others, we already use 100% of our GitHub provided caching storage budget for various compiler caches.

liamhuber · 2024-09-27T18:19:30Z

@oursland, the action I linked also removes the defaults channel; if this PR fixes your problem, it's likely that action will provide immediate relief as well -- the caching issue is an additional, unrelated issue it fixes. It is not a pure-wrapper though -- it will also try to pip install your local package by default -- but you should have enough flexibility in the inputs to get it to work as a pure wrapper. If you need something to solve the defaults problem immediately, I recommend trying that.

jakirkham · 2024-09-27T20:04:25Z

AIUI Conda will go through a depreciation cycle before removal. It will probably be a bit before installers come prebaked with the newer Conda

Not to mention some users may pin an installer version as part of this workflow

Given this, think this change still is desirable. What do we need to do to get it in?

jaimergp · 2024-09-28T10:50:16Z

Thanks @jakirkham, yes, when I posted this comment I had gone for the straight removal disregarding the deprecation cycle, which was brought in the code review.

This change is still needed as a result, but I think @jezdez doesn't want the nodefaults hack in more places, so we'll have to come up with a different solution.

My ideas:

Warn about channels implicitly adding defaults
Add a new setting to remove defaults if not in channels explicitly. Something like ignore_defaults_in_channels.

Since this is a new approach, I'll open a new PR. Thanks all for the fruitful discussion!

oursland · 2024-09-28T17:20:04Z

Add a new setting to remove defaults if not in channels explicitly. Something like ignore_defaults_in_channels.

This is the expected behavior. If I provide a list of channels, then I expect that those channels and only those channels are being used. The inclusion of defaults implicitly in the channels list was a surprise when it caused breakage in our builds. I consider this inclusion to be a bug.

I personally do not believe an additional flag to remove defaults should be necessary as I consider it's implicit inclusion to be a bug that should be resolved.

jaimergp · 2024-09-30T10:50:57Z

Believe it or not, it was intentionally designed that way. Not an oversight. You are "adding" channels to the list, which is prepopulated with defaults. I agree it is not great UX, so that's why conda is deprecating that using the standard deprecation window of three months.

If anything, the bug is here in setup-miniconda because we are using conda config --add to populate the channels, and it wasn't clear in the documentation that it happened that way. The option would have been better called add_channels. Since we can't guarantee that changing the default will break users workflows, the conservative way forward is adding a flag that people can opt-in to, and eventually turn it on by default. If you'd like to advocate for a more aggressive change, let's discuss that in the new PR.

jakirkham · 2024-09-30T17:59:21Z

Yep that is reasonable Jaime. Am not saying it needs to be this fix, but we do need something

How about we do the same thing we do in conda-forge? Namely conda config --remove channels defaults, which gives us

# .condarc
channels: []

Then we can add any channels after

liamhuber · 2024-09-30T18:25:08Z

Regardless of whether the nodefaults syntax is good or not, and even in the presence of the new better PR, I would actually still suggest also merging this PR. As long as conda itself supports nodefaults in the channels removing defaults, I feel it would be wise to synchronize behaviour in the channels arg here. Literally adding nodefaults to the channels list is unexpected and undesirable.

jakirkham

Thanks Liam! 🙏

Perhaps a concrete suggestion makes it clearer what I'm proposing. Please let me know what you think 🙂

jakirkham · 2024-09-30T18:47:54Z

dist/setup/index.js

@@ -47284,8 +47284,19 @@ function applyCondaConfiguration(inputs, options) {
        // LIFO: reverse order to preserve higher priority as listed in the option
        // .slice ensures working against a copy
        for (const channel of channels.slice().reverse()) {


Suggested change

for (const channel of channels.slice().reverse()) {

if (channels.length) {

yield condaCommand(["config", "--remove", "channels", "defaults"], options);

}

for (const channel of channels.slice().reverse()) {

Doesn't this change the behaviour so that the absence of defaults is the new default behaviour?

I actually disagree with @oursland that including defaults by default is a bug that needs to be removed -- at least here. I'm 100% in favour of getting rid of it in the default behaviour of conda itself, but for me it's important that setup-miniconda be consistent with conda where they share terminology (like "channels"). To this end I actually don't recommend changing the default behaviour, and still never want to see the channel nodefaults explicitly get added to the channels list.

Ie fix problems upstream, and keep behaviour downstream consistent with upstream (maybe adding extra convenience on top of that consistency as in... can't find the link on mobile, but you know, the other PR relevant here)

In the case when a user specifies channels (like below), it would only use those channels

setup-miniconda/README.md

Lines 349 to 353 in 8f65dda

- uses: conda-incubator/setup-miniconda@v3

with:

activate-environment: foo

python-version: 3.6

channels: conda-forge,spyder-ide

If no channels are specified, it would fallback to the default behavior of the selected installer/Conda (typically using defaults)

But this still differs from default conda behviour, yeah? Where defaults is included by default and setting the channel nodefaults removes it rather than adding a channel nodefaults?

Not sure I follow

We don't specify how the channel public API of this GHA is implemented. It could be specified through conda config commands (like now) or it could be specified by writing out a condarc or it could be options tacked onto each conda create/conda install command or some other way not currently being considered

Think relying on how these public APIs are implemented under-the-hood is relying on an implementation detail, which would be subject to change. There is no guarantee the implementation details would remain the same

Though did miss we do say we will add defaults to the end of the list 🤦‍♂️

setup-miniconda/README.md

Lines 335 to 337 in 8f65dda

- conda-forge

- spyder-ide

- defaults

That said, I think this is worth breaking. It would mean using a major version bump

jakirkham · 2024-09-30T18:49:52Z

dist/setup/index.js

+            if (channel === "nodefaults") {
+                core.info("Removing channel defaults");
+                try {
+                    yield condaCommand(["config", "--remove", "channels", "defaults"], options);
+                }
+                catch (err) {
+                    core.info("Removing defaults raised an error -- it was probably not present.");
+                }
+            }
+            else {
+                core.info(`Adding channel '${channel}'`);
+                yield condaCommand(["config", "--add", "channels", channel], options);
+            }


Suggested change

if (channel === "nodefaults") {

core.info("Removing channel defaults");

try {

yield condaCommand(["config", "--remove", "channels", "defaults"], options);

}

catch (err) {

core.info("Removing defaults raised an error -- it was probably not present.");

}

}

else {

core.info(`Adding channel '${channel}'`);

yield condaCommand(["config", "--add", "channels", channel], options);

}

core.info(`Adding channel '${channel}'`);

yield condaCommand(["config", "--add", "channels", channel], options);

jakirkham · 2024-09-30T18:50:43Z

src/conda.ts

@@ -126,8 +126,22 @@ export async function applyCondaConfiguration(
  // LIFO: reverse order to preserve higher priority as listed in the option
  // .slice ensures working against a copy
  for (const channel of channels.slice().reverse()) {


Suggested change

for (const channel of channels.slice().reverse()) {

if (channels.length) {

yield condaCommand(["config", "--remove", "channels", "defaults"], options);

}

for (const channel of channels.slice().reverse()) {

jakirkham · 2024-09-30T18:52:04Z

src/conda.ts

+    if (channel === "nodefaults") {
+      core.info("Removing channel defaults");
+      try {
+        await condaCommand(
+          ["config", "--remove", "channels", "defaults"],
+          options,
+        );
+      } catch (err) {
+        core.info(
+          "Removing defaults raised an error -- it was probably not present.",
+        );
+      }
+    } else {
+      core.info(`Adding channel '${channel}'`);
+      await condaCommand(["config", "--add", "channels", channel], options);
+    }


Suggested change

if (channel === "nodefaults") {

core.info("Removing channel defaults");

try {

await condaCommand(

["config", "--remove", "channels", "defaults"],

options,

);

} catch (err) {

core.info(

"Removing defaults raised an error -- it was probably not present.",

);

}

} else {

core.info(`Adding channel '${channel}'`);

await condaCommand(["config", "--add", "channels", channel], options);

}

core.info(`Adding channel '${channel}'`);

yield condaCommand(["config", "--add", "channels", channel], options);

liamhuber · 2024-09-30T20:56:53Z

It's maybe worth quickly revisiting why this PR exists: I ran into the nasty situation that defaults was getting added to my channels even if I didn't ask for it, #186. When I looked at conda, I saw this could be fixed by adding the channel nodefaults, but in setup-miniconda this simply literally added the channel nodefaults!

Now the real solution is to fix this upstream in conda, which is exactly what @jaimergp did conda/conda#14227, so to the extend that this PR raised the visibility of #186 high enough to motivate that change, it's a full success. Everything after that real, upstream fix to the fundamental problem is just transient window dressing.

But what window dressing should there be? IMO, unless it is very explicitly documented, setup-miniconda should behave like conda where we would naively expect it to. That means I don't recommend getting rid of defaults by default; extending new syntax to get rid of defaults like in #367 is great; and where setup-miniconda input looks like conda input and behaves almost like conda input, it ought to behave exactly like conda input -- anything else is just super confusing.

I patched the problem downstream in my own workflows by initializing an empty .condarc, so I've got basically zero skin in this game beyond trying to be a helpful neighbour. My feelings won't be hurt if you close this and just run with #367 until conda/conda#14227 eventually becomes the conda standard -- but for the sake of other users, it's my earnest recommendation to drive for consistency between setup-miniconda and conda above other considerations.

EDIT: missed a mini

Remove defaults on channel nodefaults

680a72b

liamhuber requested a review from a team as a code owner August 26, 2024 22:44

This was referenced Aug 26, 2024

Be extra rigorous about using conda-forge pyiron/actions#127

Closed

[patch] Nodefaults pyiron/actions#129

Merged

liamhuber added 2 commits August 27, 2024 15:13

Allow removing defaults to fail

5dae362

E.g. because it is not there

Refactor: rename error variable to be consistent with codebase

503b0a3

jaimergp mentioned this pull request Sep 10, 2024

Channel handling improvements conda/conda#14217

Open

13 tasks

jaimergp reviewed Sep 10, 2024

View reviewed changes

src/conda.ts Outdated Show resolved Hide resolved

jaimergp reviewed Sep 10, 2024

View reviewed changes

jezdez requested changes Sep 10, 2024

View reviewed changes

Update src/conda.ts

e2848d3

Co-authored-by: jaimergp <[email protected]>

Build the ts committed suggestions into the js

5f0df52

jaimergp mentioned this pull request Sep 28, 2024

Add 'no-implicit-channels' setting #367

Open

jakirkham reviewed Sep 30, 2024

View reviewed changes

		core.info("Removing channel defaults");
		if (channel === "nodefaults") {

	- uses: conda-incubator/setup-miniconda@v3
	with:
	activate-environment: foo
	python-version: 3.6
	channels: conda-forge,spyder-ide

Remove defaults channel on input channel: nodefaults #364

Are you sure you want to change the base?

Remove defaults channel on input channel: nodefaults #364

Conversation

liamhuber commented Aug 26, 2024

liamhuber commented Aug 27, 2024

liamhuber commented Aug 27, 2024

liamhuber commented Aug 27, 2024

liamhuber commented Sep 8, 2024

Choose a reason for hiding this comment

jaimergp left a comment

Choose a reason for hiding this comment

jezdez commented Sep 10, 2024

jezdez commented Sep 10, 2024

jezdez left a comment

Choose a reason for hiding this comment

liamhuber commented Sep 10, 2024

liamhuber commented Sep 10, 2024

jaimergp commented Sep 10, 2024

liamhuber commented Sep 10, 2024

oursland commented Sep 27, 2024

liamhuber commented Sep 27, 2024

oursland commented Sep 27, 2024

liamhuber commented Sep 27, 2024

oursland commented Sep 27, 2024

liamhuber commented Sep 27, 2024

jakirkham commented Sep 27, 2024

jaimergp commented Sep 28, 2024

oursland commented Sep 28, 2024

jaimergp commented Sep 30, 2024

jakirkham commented Sep 30, 2024

liamhuber commented Sep 30, 2024

jakirkham left a 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

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

liamhuber commented Sep 30, 2024 • edited Loading

Remove defaults channel on input `channel: nodefaults` #364

Remove defaults channel on input `channel: nodefaults` #364

liamhuber commented Sep 30, 2024 •

edited

Loading