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

update infrastructure.rst/compilers #1950

Merged
merged 7 commits into from
Apr 18, 2024
Merged

Conversation

h-vetinari
Copy link
Member

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:

diff --git a/src/maintainer/infrastructure.rst b/src/maintainer/infrastructure.rst
index 6e8d5bdc..c3f5bc35 100644
--- a/src/maintainer/infrastructure.rst
+++ b/src/maintainer/infrastructure.rst
@@ -318,21 +318,26 @@ roll out in a compatible manner to the ecosystem as feedstocks get rerendered.
 For such ABI-compatible upgrades, similar but looser principles apply:

 * The pins are similarly in the global pinning, see :ref:`globally_pinned_packages`.
-* We provide no support of any kind in terms of the long-term availability of a given compiler generation.
+* We provide no support of any kind in terms of the long-term availability of a given compiler version.
 * We generally provide notice in the form of an announcement when a compiler is going to be upgraded.
-* In general, our compilers on Linux and OSX are using very recent compilers, whereas
-  on windows, we generally use the last supported VS version.
+* Without promising any timelines, our compilers on Linux and OSX are normally
+  very recent; on Windows, we generally use the last supported VS version.

 Despite the lack of explicit support, we try to keep the compilers in their various versions
 working also outside of conda-forge, and even provide an easy way to install them
 (through the `compilers <https://github.com/conda-forge/compilers-feedstock>`_ feedstock).

-In more detail, our default compiler stack is made up very differently on each platform.
 More specifically, each compiler uses an _activation_ package that makes the difference
 between it being merely present in a build environment, and it being used by default.
 These will be installed when using ``{{ compiler('xyz') }}`` in ``meta.yaml``, where
 ``'xyz'`` is one of ``'c', 'cxx', 'fortran', 'cuda', 'rust', 'go-cgo', 'go-nocgo'``.

+Our default compiler stack is made up very differently on each platform; each platform
+has its own default compiler, with its own set of feedstocks that provide them. Due to
+historic reasons (the way compilers are integrated with their OS, and the amount of
+software written in them, etc.), the most impactful languages are C & C++ (though
+Fortran is considered part of the default, not least because GCC compiles all three).
+
 Linux (GCC):

 * [C, C++, Fortran] Activation: https://github.com/conda-forge/ctng-compiler-activation-feedstock/

OP from #1946 :

As discussed in conda-forge/conda-forge-pinning-feedstock#4215.

This is only my understanding, consider it a draft proposal.

I removed a lot of the language around the lack of support and how to upgrade, because we're now long past the comp7 days where changing the compilers needed a conda-forge wide migration. [edit: Separated ABI-compatible & ABI-incompatible compiler upgrade after discussion]

I'm sure this can still be improved in a lot of ways, but I'd say it's already an improvement over the (very stale) status quo.

PTAL @isuruf @xhochy @jakirkham @beckermr CC @conda-forge/core

@h-vetinari h-vetinari requested a review from a team as a code owner April 24, 2023 06:51
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.

Please remove the "things are better than the past" rhetoric.

@h-vetinari
Copy link
Member Author

Please remove the "things are better than the past" rhetoric.

Done, hope you like this version better.

@jaimergp
Copy link
Member

@isuruf - It looks like your comments were addressed. Can you reassess your review? Thanks! 🙏

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.

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.

Copy link
Member Author

@h-vetinari h-vetinari left a 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.

Comment on lines 314 to 322
* 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.
Copy link
Member Author

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

  1. because it's vague enough about {bit of time, degree of disruption, amount of work}

Comment on lines 329 to 331
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.
Copy link
Member Author

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. 👆

Copy link

netlify bot commented Feb 28, 2024

Deploy Preview for conda-forge-previews ready!

Name Link
🔨 Latest commit 7ce4017
🔍 Latest deploy log https://app.netlify.com/sites/conda-forge-previews/deploys/65fcfb9a3d846d00080368e8
😎 Deploy Preview https://deploy-preview-1950--conda-forge-previews.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@jaimergp
Copy link
Member

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 <h.vetinari@gmx.com>
@h-vetinari
Copy link
Member Author

h-vetinari commented Mar 22, 2024

I'd be happy to make it clearer that no compiler version change is ever free of risk of ABI-breakage.

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! :)

@h-vetinari h-vetinari requested a review from isuruf March 24, 2024 22:55
@jaimergp
Copy link
Member

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.

@h-vetinari
Copy link
Member Author

Thanks for the reviews!

@h-vetinari h-vetinari merged commit 5003dd8 into conda-forge:main Apr 18, 2024
6 checks passed
@h-vetinari h-vetinari deleted the compilers branch April 18, 2024 02:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

5 participants