-
-
Notifications
You must be signed in to change notification settings - Fork 279
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
update infrastructure.rst/compilers #1950
Conversation
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.
Please remove the "things are better than the past" rhetoric.
Done, hope you like this version better. |
@isuruf - It looks like your comments were addressed. Can you reassess your review? Thanks! 🙏 |
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.
Can you also keep just one section without having two for abi compatible and incompatible?
As we saw, no major version upgrade is without abi incompatibilities. It's about the degree of incompatibilities.
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.
@isuruf: As we saw, no major version upgrade is without abi incompatibilities. It's about the degree of incompatibilities.
It's true that no compiler version change is 100% guaranteed ABI-compatible (though GCC tries very hard). But the gulf between the degrees here is vast - one is "ABI-compatible to the best of our knowledge and/or up to extremely rare edge cases" and the other is "ABI will break for fundamental quantities, leading to crashes or worse unless everything is recompiled".
It makes sense to treat these these migrations differently (see details below).
I think we need to:
- reflect reality, i.e. what we've been doing the last few years
- distinguish between "definitely going to break most packages", and "99.99% feedstocks are going to be fine"
I'd be happy to make it clearer that no compiler version change is ever free of risk of ABI-breakage.
src/maintainer/infrastructure.rst
Outdated
* We upgrade them in an ad-hoc manner on a periodic basis as we have the time and energy to do so. | ||
Note that because of the way we enforce runtime constraints, these compiler upgrades will not break | ||
existing packages. However, if you are using the compilers outside of ``conda``, then you may find issues. | ||
* We generally provide notice in the form of an announcement when a compiler is going to be upgraded. | ||
* We generally provide notice in the form of an announcement when an ABI-incompatible compiler change is going to happen. | ||
Note that these changes take a bit of time to complete, so you will generally have time | ||
to prepare should you need to. | ||
* Some of the criteria we think about when considering a compiler migration include | ||
1) the degree of disruption to the ecosystem, 2) the amount of work for the ``core`` team, | ||
and 3) the amount of time it will cost our (volunteer) feedstock maintainers. |
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.
@isuruf: Can you also keep just one section without having two for abi compatible and incompatible?
Let me take an example why I think that's not a good idea.
The highlighted text above IMO really does not apply to the compiler version bumps of the last 3-4 years. What we've done does not need "a bit of time to complete", does not cause noticeable disruption, does not cause substantial work for either the core team or the feedstock maintainers
I'm not claiming that it's completely unnoticeable, but although I cannot look into the minds of the previous authors, it looks pretty obvious that this was written with large-scale migrations in mind. We could split hairs that strictly speaking it remains true1, but the context is further solidified by surrounding statements such as "Our current compiler stack is referred to internally as comp7
. The previous compiler stack [...] was sometimes referred to as comp4
."
Footnotes
-
because it's vague enough about
{bit of time, degree of disruption, amount of work}
↩
src/maintainer/infrastructure.rst
Outdated
For the cases that do not require a complete rebuild of conda-forge (i.e. if the ABI | ||
of a new compiler remains compatible), we can just increase the version in our global | ||
pinning, and it will slowly roll out to the ecosystem as feedstocks get rerendered. |
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.
All this PR is trying to document is what we're already doing, i.e. 👆
✅ Deploy Preview for conda-forge-previews ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Rebased here for compat with Docusaurus, @h-vetinari. Might need an update or two here and there to make sure things are accurate (e.g. the CUDA overhaul is finished now). |
Co-authored-by: h-vetinari <[email protected]>
OK, I rewrote the ABI-compatibility section once more. The rest of my response to @isuruf's review still stands. Here's hoping we can come to an agreement on how to document the status quo! :) |
Reads great to me! The only comment I would have is to add a couple of subheaders to make navigation and linking to a certain section a bit easier, but definitely not a blocker. |
Thanks for the reviews! |
Reopen #1946 with more time for discussion, to ensure we don't accidentally change policy.
I made some small edits of oversights; diff compared to #1946 is below the fold:
OP from #1946 :