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

Add 'conda-remove-defaults' setting and support 'nodefaults' as a keyword channel #367

Merged
merged 23 commits into from
Oct 30, 2024

Conversation

jaimergp
Copy link
Member

Supersedes #364

@jaimergp jaimergp requested a review from a team as a code owner September 28, 2024 11:19
@jaimergp
Copy link
Member Author

Errors in the mamba examples are unrelated and caused by the mamba 2.0 release not having mamba.bat under condabin/ anymore. Adding it at conda-forge/mamba-feedstock#251

@jakirkham
Copy link
Member

Thanks Jaime! 🙏

Am actually wondering whether we need a setting for this or if we should just treat an explicit channel as just that what a user intended

Think it is easier and clearer for users to add defaults if they want it in the channel list

@jaimergp
Copy link
Member Author

jaimergp commented Oct 1, 2024

Yes, in the long term, that's where I want to get at. My current concern if that if we change that behaviour overnight without warning first, then we might break a few CI pipelines that were inadvertently relying on defaults as a channel (because strict priority is not default either).

After all this action is relied on by... 16k+ workflows. Of those, ~6k set channels. Of those, ~3.7k workflows specify defaults explicitly.

@jaimergp
Copy link
Member Author

jaimergp commented Oct 3, 2024

Fix for example 6 available at #368. Once that's merged we can continue here.

Copy link
Member

@jezdez jezdez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd suggest to be.. more explicit with the option name, since all it does right now is removing "defaults", nothing else. Also prefix the option with conda since this only applies to conda, not mamba.

action.yml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
.github/workflows/example-14.yml Outdated Show resolved Hide resolved
.github/workflows/example-14.yml Outdated Show resolved Hide resolved
src/types.ts Outdated Show resolved Hide resolved
src/conda.ts Outdated Show resolved Hide resolved
dist/setup/index.js Show resolved Hide resolved
dist/setup/index.js Outdated Show resolved Hide resolved
action.yml Outdated Show resolved Hide resolved
jaimergp and others added 3 commits October 4, 2024 10:41
Co-authored-by: Jannis Leidel <jannis@leidel.info>
Co-authored-by: liamhuber <liamhuber @users.noreply.github.com>
@jaimergp jaimergp changed the title Add 'no-implicit-channels' setting Add 'conda-remove-defaults' setting and support 'nodefaults' as a keyword channel Oct 4, 2024
fail-fast: false
matrix:
os: ["ubuntu-latest", "macos-latest", "windows-latest"]
conda-remove-defaults: ["true", "false"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about something like

Suggested change
conda-remove-defaults: ["true", "false"]
explicitly-listed-channels-only: ["true", "false"]

Admittedly verby, but I think we can write this in a way that gets closer to the long-term intended behavior

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See #367 (review). I don't particularly care about the name itself, given it'll be probably be obsolete by March next year.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the long-term future for this feature is only to migrate conda users to the same pattern as mamba and pixi follow, setting a default channel in the distribution/bootstrapper layer and not hard-coding it in the codebase. That's why I think the name of the parameter for setup-miniconda is okay to focus on conda only, and not required to be named generically.

@jakirkham
Copy link
Member

jakirkham commented Oct 4, 2024

Yes, in the long term, that's where I want to get at. My current concern if that if we change that behaviour overnight without warning first, then we might break a few CI pipelines that were inadvertently relying on defaults as a channel (because strict priority is not default either).

After all this action is relied on by... 16k+ workflows. Of those, ~6k set channels. Of those, ~3.7k workflows specify defaults explicitly.

AIUI the path proposed here is...

  1. Add a new flag and document
  2. Recommend users start setting (maybe with a warning)
  3. Change the default in a major version
  4. Deprecate the flag
  5. Remove the flag (in the next major version)

It is certainly a valid way to do it. Am just concerned by the length of time that will take and the effort it puts us and users. Can we cutdown this process a bit?

Admittedly there is the other extreme, change behavior in a major version with documentation. This is not an unheard of approach. Though we need not do that either

Think there is a reasonable middle path. We already are deprecating nodefaults. Why not leverage it for one last release then do a major version bump that drops it everywhere?

If not that, can we thin this process a bit? How can we get this to no more than one deprecation cycle?

@jaimergp
Copy link
Member Author

jaimergp commented Oct 4, 2024

In a way we are tied to the conda deprecation cycle, where this behavior won't be default til March 2025. We might need to keep the flag around for users of older versions.

Think there is a reasonable middle path. We already are deprecating nodefaults. Why not leverage it for one last release then do a major version bump that drops it everywhere?

This looks reasonable to me, but the major version bump should happen in March to align with conda. Is that too long?

@jaimergp
Copy link
Member Author

jaimergp commented Oct 4, 2024

@jezdez
Copy link
Member

jezdez commented Oct 4, 2024

I strongly disagree that nodefaults should be elevated to a first class feature in the action. I'm lukewarm even acknowledging it with a warning, but I guess that's fine. My main concern is that this is feature creep for a very short timeframe.

@jaimergp
Copy link
Member Author

jaimergp commented Oct 4, 2024

Yea, I don't like it either, but there are ~200 workflows out there using it. We need to warn them about the new option, enable the behavior, and then turn it into an error.

It's not even documented in the README, while conda-remove-defaults is. I think this is the best compromise we can afford for now.

fail-fast: false
matrix:
os: ["ubuntu-latest", "macos-latest", "windows-latest"]
conda-remove-defaults: ["true", "false"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the long-term future for this feature is only to migrate conda users to the same pattern as mamba and pixi follow, setting a default channel in the distribution/bootstrapper layer and not hard-coding it in the codebase. That's why I think the name of the parameter for setup-miniconda is okay to focus on conda only, and not required to be named generically.

@jaimergp jaimergp merged commit 65da104 into main Oct 30, 2024
81 checks passed
@jaimergp jaimergp deleted the no-implicit-channels branch October 30, 2024 18:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants