-
-
Notifications
You must be signed in to change notification settings - Fork 53
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
Conversation
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 ( I do have some suggestions for making it better though... For recipe/meta.yaml:
|
For some extra visibility: I found 9 feedstocks that manually add the defaults channel in CBC. Of those: Don't need defaults anymore
Need tensorflowFeedstock broken / dead |
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 ( |
I hope this is considered blocked from merging until there are recent tensorflow and pytorch builds on conda-forge for Win+Mac+Linux |
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 |
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? |
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.
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. |
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. |
Tensorflow compatibility with conda-forge is somewhat beyond the scope of this PR. Today, if you add |
There was a problem hiding this 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.
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 |
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 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. |
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. |
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 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. |
@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?
These kind of "infrastructure" additions can only be tested in production. This is really difficult to do, takes alot of resources (human time). |
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. |
Yes, I do.
@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. |
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:
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. |
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. |
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
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.
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? |
I'm not sure what is inappropriate here. Maybe I worded things incorrectly and would like to understand what was wrong.
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.
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.
That's really good work. Thanks a lot for helping out there and spending your time.
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.
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.
Sure. Or just a whitelist here with the two feedstocks would be very easy to add. |
Great! |
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! |
Hey, maintainer of 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 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! |
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. |
Hmm, true enough. I believe I tried to implement the CI in the feedstock to avoid reusing |
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). |
@isuruf are you ok with the PR as it is now? It provides an options for those that need it. |
This has 5 core approvals, and there's hopefully nothing controversial about the changes anymore. Thanks for adding the allowlist bits @isuruf. |
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