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

remove defaults as an allowed channel_source #339

Merged
merged 6 commits into from
Oct 7, 2024

Conversation

h-vetinari
Copy link
Member

Noticed that we haven't removed this as an allowed channel_source, even though we haven't been using the defaults channels in conda-forge since almost 3 years: conda-forge/conda-forge-pinning-feedstock#1954

@h-vetinari h-vetinari requested a review from a team as a code owner September 10, 2024 13:39
@conda-forge-webservices
Copy link
Contributor

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe/meta.yaml) and found it was in an excellent condition.

I do have some suggestions for making it better though...

For recipe/meta.yaml:

  • No valid build backend found for Python recipe for package conda-forge-ci-setup using pip. Python recipes using pip need to explicitly specify a build backend in the host section. If your recipe has built with only pip in the host section in the past, you likely should add setuptools to the host section of your recipe.

@h-vetinari
Copy link
Member Author

@conda-forge-webservices
Copy link
Contributor

conda-forge-webservices bot commented Sep 10, 2024

Hi! This is the friendly automated conda-forge-linting service.

I just wanted to let you know that I linted all conda-recipes in your PR (recipe/meta.yaml) and found it was in an excellent condition.

@pgunn
Copy link

pgunn commented Sep 10, 2024

I hope this is considered blocked from merging until there are recent tensorflow and pytorch builds on conda-forge for Win+Mac+Linux

@h-vetinari
Copy link
Member Author

I hope this is considered blocked from merging until [...]

This is not really how conda-forge works. We have switched away from using anything from defaults a long time ago, but forgot to close this loophole here (at least, that's my view). If your organisation depends on tensorflow builds on windows, it would be great if you could look into sponsoring some CI resources, which is the main limiting factor for both pytorch and tensorflow.

We now have infrastructure to do builds from special-purpose runners (which pytorch is using already, prefix.dev is providing the windows CI runners used in conda-forge/pytorch-cpu-feedstock#231), so basically as soon as we get the required resources, we can build tensorflow as well.

PS. Even if this PR ends up getting merged, you can set

remote_ci_setup:
  - conda-forge-ci-setup<4.8.1

in your conda-forge.yml and things should still keep working for a while a least.

@pgunn
Copy link

pgunn commented Sep 10, 2024

If you're removing these config flags in other projects that don't need it, do you gain anything important in removing the possibility of those options for the projects that need it (and breaking them in the process)? I understand the usefulness for code maintenance in removing unused options, but when you know they're being used, it seems premature.

If this were to be merged it'd really make it hard for us to keep the project on conda (or, knowing that this can happen, for us to continue to recommend conda as a technology for our part of academia). If you really think tensorflow and pytorch are getting good Windows packaging soon, why not hold off?

@h-vetinari
Copy link
Member Author

Conda-forge is a volunteer effort. We've told everyone for years that mixing channels is not something we can support. Following through on that should not be controversial IMO.

If you really think tensorflow and pytorch are getting good Windows packaging soon, why not hold off?

Because the incentives are all skewed towards conda-forge accumulating tech debt left and right so others can have their particular itch scratched (for free). I don't mind delaying in any particular case, but it's something that happens over and over, to our detriment.

@pgunn
Copy link

pgunn commented Sep 10, 2024

You've made good progress with this set of diffs across the feedstocks in removing it where it's not used. I'm sympathetic to concerns of tech debt - it's something we all struggle with as developers. I think it's just not the right call to break things for the sake of cleanliness (you know that unless the tensorflow/pytorch packaging gets fixed on windows before that old version of conda-forge-ci-setup falls off the end of the supportable, it's not a real solution).

That said, I get that you can't make exceptions for every package; if this means that I need to decide between losing our Windows users, hoping for tensorflow packaging to get fixed that's been broken for years, and looking at alternatives to conda, we are after all just one package (and a small number of others we've helped people get onto conda-forge). We've occasionally removed support for data formats that almost nobody used and needed to give the bad news to one user who had a weird microscope that produced data in that format.

@hmaarrfk
Copy link
Contributor

Tensorflow compatibility with conda-forge is somewhat beyond the scope of this PR.

Today, if you add defaults in a recipe, it can lead to a seriously broken output. It has been this way for nearly 3 years as h-vetinari mentioned earlier.

Copy link
Member

@isuruf isuruf left a comment

Choose a reason for hiding this comment

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

There should be a way for feedstocks to override this possibly with a admin-request.

@marcelotrevisani
Copy link
Member

There should be a way for feedstocks to override this possibly with a admin-request.

I don't agree with that, the only way that people should be able to add default channels is if, an it is a big if, all the conda like package managers scream at the user saying that defaults was used and they might be in breach of the TOS
as the wording that people are receiving is that, conda-forge is safe from TOS, which might not be true as it uses the default channels implicitly

@isuruf
Copy link
Member

isuruf commented Sep 14, 2024

This only affects two feedstocks. There are multitude of ways that defaults channel can creep up when using conda-forge packages by accident. We also allow noarch packages that depend on pytorch with the understanding that users would get pytorch from defaults on windows. Similar to tensorflow. Going by your logic, we should then add __unix to all recipes that use pytorch because we don't have a recent pytorch on windows. (This will change soon hopefully)

This is not the forum for making changes in the conda ecosystem. You should open an issue about what your plan is.

@marcelotrevisani
Copy link
Member

marcelotrevisani commented Sep 14, 2024

This is not the forum for making changes in the conda ecosystem. You should open an issue about what your plan is.

that is exactly what you mentioned as a change request.

if someone uses pytorch on windows with only conda-forge as a channel it should fail, that is up to the user to pull pytorch from default and violate the TOS or not, and not a package that is already violating it like tensorflow by adding the default channels in the conda-forge ecosystem.

@hmaarrfk
Copy link
Contributor

There should be a way for feedstocks to override this possibly with a admin-request.

can we leave this as "future work"? Maybe for now we can observe people who pin to an older version conda-forge-ci-setup-feedstock for now to get around this.

@pgunn
Copy link

pgunn commented Sep 15, 2024

There should be a way for feedstocks to override this possibly with a admin-request.

can we leave this as "future work"? Maybe for now we can observe people who pin to an older version conda-forge-ci-setup-feedstock for now to get around this.

As one of those people, I'm stopping releases on conda for now; pinning to an older version of that is walking onto a crumbling foundation and anyone who knows how software maintenance works knows not to even try this solution. Even in this conversation people said it'll stop working at some point in the future.

(in the exact same sense, observing is one of those passive "let's forget about this concern" because of how passive it is. You observe for a bit, then forget you're observing, and the conversational gambit wins, in just the same way that talking a lot about what's in scope for a ticket in order to ignore concerns is common for conda-forge.

@h-vetinari
Copy link
Member Author

[...] talking a lot about what's in scope for a ticket in order to ignore concerns is common for conda-forge.

Conda-forge has ~23'000 feedstocks. Do you feel confident you're in a position to make general statements about what is common or not?

More importantly, there are considerations with Anaconda's enforcement of their Terms Of Service (TOS), that make anything involving the defaults channels a hot button topic, where conda-forge has a strong interest to protect itself and its users from accidentally infringing on the TOS and being presented with a large bill.

Finally, conda-forge is a do-ocracy - problems get tackled and fixed by interested individuals. I suggest you be the change you want to see in the world, whether that's helping with creating the infrastructure for a per-feedstock opt-in, or even better: help unblock tensorflow builds on windows.

@hmaarrfk
Copy link
Contributor

@isuruf did you have a feedstock in mind for one that would be allowed to use the exception you want to introduce. Do you you feel like the comments @pgunn made here and the challenges associated with https://github.com/conda-forge/caiman-feedstock/ warrant an exception for that feedstock?

pinning to an older version

These kind of "infrastructure" additions can only be tested in production. This is really difficult to do, takes alot of resources (human time).

@pgunn
Copy link

pgunn commented Sep 15, 2024

To be clear, I'm not actually sure my project merits an exception. Unless tensorflow/pytorch get good packaging on conda-forge very soon, this does make it impossible for me to keep packaging my package on conda-forge (I only want to support one install route and conda-forge has been great for me for the years it's been here), but it is only one package and if you really think it's that this is an important change, it's probably right for conda-forge to do this even if it screws over a very small number of packages. There's a chance that if I were in your shoes, I'd be pushing this diff. But...

I don't see the point in removing the config knob - almost nobody who packages stuff on conda-forge will ever find it even if it's documented, and if it's not documented then the concern of people stumbling into it and causing trouble doesn't feel real to me, but this is a question of judgement on the future.

Most likely if I have to remove my package from conda-forge because of this, I'll look into what it takes to set up another conda channel where I can still do what I'm doing now. If that looks too hard, I'll have to look again at supporting pip, but that's an ugly choice.

@isuruf
Copy link
Member

isuruf commented Sep 15, 2024

Do you you feel like the comments @pgunn made here and the challenges associated with https://github.com/conda-forge/caiman-feedstock/ warrant an exception for that feedstock?

Yes, I do.

in the exact same sense, observing is one of those passive "let's forget about this concern" because of how passive it is. You observe for a bit, then forget you're observing, and the conversational gambit wins, in just the same way that talking a lot about what's in scope for a ticket in order to ignore concerns is common for conda-forge.

@pgunn, I fully agree with you on this. Lately ignoring other peoples' concerns has been common place in conda-forge and I've had to spend a lot of time make sure that this does not happen.

@hmaarrfk
Copy link
Contributor

hmaarrfk commented Sep 15, 2024

Most likely if I have to remove my package from conda-forge because of this, I'll look into what it takes to set up another conda channel where I can still do what I'm doing now.

Unfortunately, this kind of thing will "always" be the case with whichever channel you will decide to take on.

caiman is a complex project (cython + tensorflow + jupyter + bokeh) and the beauty of conda(-forge) is that you can create your own channels.

You can even use conda-smithy to build your "own feedstock" and then to upload to it. I use this all the time for my own experimentation. Azure gives free CIs to any open source project, so its all basically "free".

My users then have a single instruction:

  1. Install miniforge
  2. Add the channel conda config --add channel caiman
  3. conda/mamba install caiman

I've noticed a few other things about the CaImAn recipe. lets discuss in an issue on the feedstock itself conda-forge/caiman-feedstock#69

edit: Isuru my message is not a response to yours, my draft just got crossed with your message.

@pgunn
Copy link

pgunn commented Sep 15, 2024

I want to also make it clear, because I get that we're on the internet and a lot of emotional connotation is just lost, that I deeply respect all of you and appreciate both the conda-forge and the conda projects; they've been great for us and our users.

@hmaarrfk We're currently working to rewrite the tensorflow bits in pytorch, in the hopes that that will lessen the packaging woes (I don't think Google really wants to maintain tensorflow long-term and they're pushing people towards jax). I was hoping for the switchover to land within the next month or two.

The thing that gave me the idea of running our own channel is that another research group at Janelia (that we collaborate with) has one called flyem, with alternative builds of a lot of things. I don't know enough about the channel system though, having only so far packaged things for conda-forge.

@hmaarrfk
Copy link
Contributor

Lately ignoring other peoples' concerns has been common place in conda-forge

I'm not sure this is appropriate to say Isuru. We are trying to take into account everybody's concerns. Ultimately, the constraint of time exists.

However, I do know that many of the issues I help resolve where users are mixing default+conda-forge go away when users have a clean "conda-forge" environment.

Its sad that we do not have pytorch + tensorflow on windows, but its also unreasonable to allow "workarounds" that essentially guarantee a broken environment for users.

I am really trying to find a way to help Pat with their recipe: conda-forge/caiman-feedstock#70
I didn't send my message until after I had dry run'ed things on my linux box.

I've had to spend a lot of time make sure that this does not happen.

You really have been instrumental in helping recipes all around, thank you for that.

However, I ask the question: Are you really going to help a user troubleshoot their mixed channel environment?

I am not. Will it drive users away? Maybe..... but I'm really trying here.

I think that you should value your own time, and h-vetinari's time much more highly than you let on. If you already do then great!

Personally, I feel like more and more "work" is being put on maintainers (aarch, ppc64le, pypy (which recently got dropped)).

Adding "workarounds" for a channel that we will not help our users troubleshoot seems like an anti-pattern.

In my mind, this PR is good for the conda-forge community as a whole. As with every "change", there is always one "exceptional use case". But the caiman recipe was "red" since March 2024 https://github.com/conda-forge/caiman-feedstock/commits/main/ and the last 3 builds didn't have the windows releases in proper order (1.11.1, 1.11.2, and 1.11.3 have many red builds). So maybe lets not let the exception add more work to this cleanup PR.

@hmaarrfk We're currently working to rewrite the tensorflow bits in pytorch, in the hopes that that will lessen the packaging woes (I don't think Google really wants to maintain tensorflow long-term and they're pushing people towards jax). I was hoping for the switchover to land within the next month or two.

Goodluck, I am very sympathetic. I still have old "tensorflow" imports in my code.... very annoying... especially because of everything you have said.

@isuruf, ultimately, I think an admin request is overkill on this. if this is meant to be a suggestion, then maybe it can be degraded to the level of a warning, and not a error?

@isuruf
Copy link
Member

isuruf commented Sep 16, 2024

I'm not sure this is appropriate to say Isuru. We are trying to take into account everybody's concerns.

I'm not sure what is inappropriate here. Maybe I worded things incorrectly and would like to understand what was wrong.

Ultimately, the constraint of time exists.

Right. That's the whole point here. Asking another maintainer to put time and effort into building tensorflow for windows is not the solution here when it has been working so far.

Its sad that we do not have pytorch + tensorflow on windows, but its also unreasonable to allow "workarounds" that essentially guarantee a broken environment for users.

I'm sure maintainers like @pgunn understand the issues with mixing, but they also help other users in using their packages like caiman. So I don't understand why we should block their workflow.

I am really trying to find a way to help Pat with their recipe: conda-forge/caiman-feedstock#70

That's really good work. Thanks a lot for helping out there and spending your time.

However, I ask the question: Are you really going to help a user troubleshoot their mixed channel environment?

No, I'm not. Mixing channels is guaranteed to work, and the users are on their own. I'm more concerned about driving maintainers away.

Adding "workarounds" for a channel that we will not help our users troubleshoot seems like an anti-pattern.

Yes, but some maintainers do support niche use-cases and we do not add roadblocks for them. For example, some maintainers wanted to support older cuda after we dropped them and we found a way to support that in conda-forge/conda-forge-pinning-feedstock#1657.

@isuruf, ultimately, I think an admin request is overkill on this. if this is meant to be a suggestion, then maybe it can be degraded to the level of a warning, and not a error?

Sure. Or just a whitelist here with the two feedstocks would be very easy to add.

@hmaarrfk
Copy link
Contributor

Sure. Or just a whitelist here with the two feedstocks would be very easy to add.

Great!

@pgunn
Copy link

pgunn commented Sep 18, 2024

FWIW, if there's a way to keep the problem of supporting mixing channels to the package maintainers that opt-in to it, I'm good with continuing to support all the messiness that entails (along with all the other messiness of supporting a scientific software package, as well as the general support for even getting conda installed - I fairly often have a videoconference with users where I'm walking them through the registry edits on Windows to enable long filenames, or helping people where we barely share a spoken language get started with Jupyter).

I'm hopeful that conda-forge/pytorch-cpu-feedstock#32 gets finished soon; if it does, then the next release of Caiman (which will likely switch over entirely to pytorch) will be able to be pure conda-forge and no longer have this problem. In that case, all of this will be moot. Fingers crossed!

@nialov
Copy link
Member

nialov commented Sep 23, 2024

Hey, maintainer of eis_toolkit here. It is the other package requiring tensorflow on Windows. The reason to include the defaults channel "hack" was to test the package in the CI for the Windows platform. This way we did not release versions that were automatically broke for Windows users. I suppose an alternative is to test each pull request locally on a Windows machine before merging but this is of course more prone to errors.

I understand well, after reading the discussion here and in general, why you want to disable these kinds of hacks for maintenance purposes and you should do anything to make your job easier, we are just using (exploiting!) your platform.

However, I am interested in what is the danger to conda-forge here. As I see in https://anaconda.org/conda-forge/eis_toolkit/files, the Windows build version is not uploaded to conda-forge. So all the extra CI is doing is testing the package install (with defaults) and running tests. Or is there something else being done by the eis_toolkit-feedstock (win win_64_) action?

EDIT: Just noticed the whitelist commit, I suppose an exception is being made? Thank you for the understanding and work if that is the case!

@hmaarrfk
Copy link
Contributor

This way we did not release versions that were automatically broke for Windows users. I suppose an alternative is to test each pull request locally on a Windows machine before merging but this is of course more prone to errors.

Typically this kind of testing is done on the source repository, not really the conda-forge repo.

You should be able to be confident without the hack on conda-forge.

But yes, the exception was made to help users that needed it.

@nialov
Copy link
Member

nialov commented Sep 23, 2024

Typically this kind of testing is done on the source repository, not really the conda-forge repo.

You should be able to be confident without the hack on conda-forge.

Hmm, true enough. I believe I tried to implement the CI in the feedstock to avoid reusing environment.yaml code but after doing the "hacking" in the feedstock and getting more familiar with conda build configuration I think I could more confidently just add the environments to the source repo and implement the Windows CI there, as you suggest. Anyway, from the point of view of eis_toolkit, using the feedstock CI is not critical but currently very useful so thank you for the exception.

@pgunn
Copy link

pgunn commented Sep 30, 2024

I've thought about this a bit more (and learned more about the licensing stuff ContinuumIO is doing - very disappointed in the company), and I've changed my mind; from a compliance standpoint, it's important that people who don't have the defaults channel enabled can feel secure in knowing that ContinuumIO's lawyers won't be coming after them.

That's more important than keeping my package working (and in fact many of my package's users may be put in an unfortunate legal circumstance unless pytorch makes its way to conda-forge soon and I can migrate them off of any need for the defaults channel). I'm going to need to pause any new releases from my package for awhile until I can make sure I'm not putting my users at legal risk (not that they're safe with old releases - ContinuumIO is really screwing over the community with this Oracle-with-Java-like stuff).

@hmaarrfk
Copy link
Contributor

@isuruf are you ok with the PR as it is now? It provides an options for those that need it.

@h-vetinari
Copy link
Member Author

This has 5 core approvals, and there's hopefully nothing controversial about the changes anymore. Thanks for adding the allowlist bits @isuruf.

@h-vetinari h-vetinari merged commit 170fdf8 into conda-forge:main Oct 7, 2024
42 checks passed
@h-vetinari h-vetinari deleted the no_defaults branch October 7, 2024 08:04
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.

8 participants