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 pinned_packages option in conda-forge.yml #1776

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jaimergp
Copy link
Member

@jaimergp jaimergp commented Sep 28, 2023

Checklist

  • Added a news entry

Comes from discussion at #1773

@jaimergp
Copy link
Member Author

Needs some live testing in a demo feedstock.

@isuruf
Copy link
Member

isuruf commented Sep 28, 2023

@jaimergp
Copy link
Member Author

Sadly not. We need the pins before conda-forge-ci-setup has been installed.

@h-vetinari
Copy link
Member

Thanks for tackling all this so quickly! I'm wondering if this isn't effectively duplicating remote_ci_setup functionality in a slightly different way? Not saying that's bad, but if so we should probably consider getting rid of one or the other (at least in the medium term)?

Or is there some non-overlapping task that remote_ci_setup satisfies, but pinned_packages doesn't?

Looking at the history, remote_ci_setup was introduced in b0d8879, and updated to take a list in 83fb02e. It was also designed to allow <other_channel>::<pkg> dependencies, but I'm not sure those are in use anywhere.

@jaimergp
Copy link
Member Author

AFAIU, remote_ci_setup is meant to add packages to the base environment. We were also using it to constrain existing ones, but that doesn't guarantee it's not overridden later in the workflow (e.g. those update commands would rely on how the history is handled).

So my idea here is to have two mechanisms:

  • remote_ci_setup to add extra stuff to your base environment. A different name these days would better clarify the intent here, but I don't mind too much.
  • pinned_packages to constrain the base environment. I guess this can even be used à la run_constrained with impossible specs to prevent certain packages from being installed, but I don't think we have seen that need yet.

@h-vetinari
Copy link
Member

AFAIU, remote_ci_setup is meant to add packages to the base environment.

Yeah, though stuff in conda-meta/pinned is effectively added to the base environment as well. 🤷

I don't mind too much, but thought I'd point out the overlap.

@jaimergp
Copy link
Member Author

Yeah, though stuff in conda-meta/pinned is effectively added to the base environment as well.

I'm not sure I follow. I can add tensorflow=2 to pinned and if I don't request it in the CLI it won't be installed.

@h-vetinari
Copy link
Member

Hm, my bad. At the time when I last used it, the information I had found said (paraphrasing) "is injected upon every install/update". Wanted to double-check the docs, but they're empty. Further searches in the code did not produce more insights either, so I guess I had misunderstood that part. Tested it now locally and indeed it doesn't get added. Sorry for the noise.

@jaimergp
Copy link
Member Author

No probs, happy to clarify. The key line in classic is this one. The "explicit pool", AFAIK, is the subset of the index that contains all the possible packages in the dependency tree of roughly {*requested, *installed}. So, if the tree contains a pin, then it's enforced. Otherwise it's ignored.

@isuruf
Copy link
Member

isuruf commented Oct 18, 2023

I don't think this is necessary after #1774 right?

@jaimergp
Copy link
Member Author

We don't need it as a fix, but I'd say that this feature could be useful or at least the intent is more clear (perhaps with a different name). For example, right now we might need to use remote_ci_setup to nudge the solver into more recent versions: conda-forge/compiler-rt-feedstock#83 (comment)

I explained the idea in more detail at #1776 (comment)

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.

3 participants