Policy on support tiers, take 2

Project Infrastructure LLVM Dev List Archives
#1

Ok, so after some feedback, here’s an updated version. Separate thread as the previous got split.

People seem to agree on the overall terms, but there was confusion on the difference between tier 2 and 3 as well as clarification on what projects go where.

I have joined tiers 2 and 3 and made explicit the three criteria they fit into, with the requirements more formally explained.

Please review the sub-project lists on the two tiers, I’m not so sure about them.

Once we’re happy with the “what”, I’ll send a review for a new doc so we can discuss the writing and format there (ignore it for now).

Here it goes:

*** Tier 1: the core compiler, officially supported by the community.

Rationale:

  • Common code that supports most projects, upstream and downstream forks and forms the toolchain itself.
  • Includes everything we release on all architectures and OSs we have releases on.

What:

  • LLVM itself, clang/tools, compiler-rt, libcxx/abi/unwind, lld, lldb, openmp, mlir.
  • Basically everything inside the mono-repo that is not in tier 2.
  • Builds on all first class citizen combinations of targets x OSs (incl. test-suite).
  • The CMake build infrastructure and release scripts.
  • Phabricator & Buildbot infrastructure.
  • The test-suite repository.

Requirements:

  • Follow all core development policies on code quality, reviews, reverts, etc.
  • Noisy green buildbots, emailing all developers.
  • Most not be broken by changes in tier 2 (ex. if they require tier 1 changes).
  • Bitrot will downgrade areas to tier 2 or be removed, depending if a sub-community picks up support and has a timeline to fix.

*** Tier 2: side projects that integrate with the compiler and that should work at all times.

Rationale:

  • Code that is making its way into LLVM core (tier 1) via experimental/incubator roadmaps, or;
  • Code that isn’t meant to be in LLVM core, but has a sub-community that maintains it long term, or;
  • Code that is making its way out of LLVM core (legacy) and that is a strong candidate for removal.

What:

  • Experimental targets/options.
  • Experimental mono-repo projects (flang, libc, libclc, parallel-libs, polly, beduginfo-tests?, pstl?)
  • Incubator projects (circt, mlir-npcomp, etc).
  • Legacy tools (lnt).
  • Alternative build systems (gn, bazel).
  • Tool support (gdb scripts, editor configuration, helper scripts).

Requirements:

  • Follow all core development policies on code quality, reviews, reverts, etc.
  • Infrastructure that only notify its sub-community.
  • Most not break tier 1, promptly reverting if it does, with discussions to be done offline before reapply.
  • Leaner policy on bots being red for longer, as long as the sub-community has a roadmap to fix.
  • Leaner policy on bitrot, as long as it doesn’t impact tier 1 or other tier 2 projects.
  • Should be easy to remove (either separate path, or clear impact in code).
  • Must have a document making clear status, level of support and, if applicable, roadmap into tier 1 / out of LLVM.

cheers,
–renato

#2

I’m not super familiar with all the various subprojects, but I’d be a little hesitant to put e.g. libc and flang in the same category as vim bindings or the gn build :smiley: I think I’ve had failing pre-merge checks from both flang and polly before, so at least in practice they don’t seem to be following the guidelines you mention here. I don’t know whether this means that they should just be moved up to tier 1, or whether they are actually less-supported than some of the more established projects. If their current behavior should change, then that seems like a bigger discussion.

I also think it might be better to make this focus specifically on the monorepo. Incubator projects already have a clear policy and a lot of the fine points here only make sense in the context of the monorepo. e.g. bots with blamelists targeting only a subcommunity doesn’t make much sense for a separate repo. If you committed the the mlir-npcomp repo I think you should get notified if you broke something :smiley:

Apologies if the below falls into the category of “writing”, but here are some additional thoughts:

I also think that the distinction we previously had with tier 2 vs tier 3 was useful in terms of differentiating things that need quite a bit of promised support to be allowed in the monorepo because they’re bigger, more complicated, and require constant maintenance (e.g build systems) vs things that can be checked in without much discussion because they are small, simple, and degrade gracefully (e.g. editor bindings). Maybe separate tiers isn’t the right way to do that, but I think we should make that distinction clear. Like if someone wants to check in bindings for their editor of choice, a simple patch seems appropriate, whereas if someone (hypothetically ;-P) wants to propose a secondary build system it should at least be discussed on the mailing list. Maybe we can just include language in the description of tier 2, like:

When adding components intending for tier 2 status, the level of discussion and support commitment required is proportional to the size and complexity of the component. For example:

  1. editor bindings can be just be added as a patch following the normal review process
  2. more complex components like a secondary build system should start as an RFC that details how this component will be supported
  3. Experimental backends have an entire section on their introduction (link)
#3

I think I’d work at a little higher level on the things. Explicit lists are hard to maintain and, in general, I think we’re looking at guidelines rather than hard lists.

-eric

#4

Hi,

Nice proposal Renato! I’m very supportive of the direction in general.

I share the sentiment of Eric and Geoffrey in general though.
In particular, it wasn’t clear to me that something like flang for example would be tier 2, I told the flang maintainer to feel free to revert my patches when I break their bot for example. I assumed that every subproject in the monorepo was supported as long as they have public bots. The only explicit unsupported things are the experimental LLVM backends I believe?

Thanks for iterating on this! :slight_smile:

Cheers,

#5

Hi,

Nice proposal Renato! I’m very supportive of the direction in general.

I share the sentiment of Eric and Geoffrey in general though.
In particular, it wasn’t clear to me that something like flang for example would be tier 2, I told the flang maintainer to feel free to revert my patches when I break their bot for example. I assumed that every subproject in the monorepo was supported as long as they have public bots. The only explicit unsupported things are the experimental LLVM backends I believe?

Thanks for iterating on this! :slight_smile:

Cheers,

–
Mehdi

I think I’d work at a little higher level on the things. Explicit lists are hard to maintain and, in general, I think we’re looking at guidelines rather than hard lists.

I think an explicit list is useful right now for framing our discussion, but the final artifact should not include them (although perhaps might point to a few examples).

#6

I’m not happy about making lists either, but there was some confusion around what goes in and what doesn’t.

I’m happy to drop the lists altogether if people don’t think they help. I personally don’t think they do.

#7

Right, that’s a good split, I think. Only monorepo, minus experimental, unsupported build systems and helper files.

I’ll send another update tomorrow morning.

Thanks everyone!

#8

Sounds good, thanks! :slight_smile:

-eric

#9

Hi Renato,

Thank you for working on this! When looking at the topic from the pre-merge testing perspective 2 questions came to my mind:

  1. How should pre-merge testing behave differently on tier 1 vs. 2 projects?
    Do we want the builds/tests to pass on broken tier 2 projects?

  2. How can we automatically detect the tiers of the projects in the pre-merge build scripts? If we want to treat them differently we somehow need to be able to distinguish these in the repo.

#10
  1. How should pre-merge testing behave differently on tier 1 vs. 2 projects?
    Do we want the builds/tests to pass on broken tier 2 projects?

Hi Christian,

My view is that we shouldn’t change how we test, either pre merge or post merge. If we have official tests for something already, those must pass.

The idea of tiers is to reduce the burden of maintenance of sections of the code for both the sub-community that cares about it and the rest of the community that doesn’t.

So, whatever we already have a pre-merge test for, is IMO tier 1. If they are causing grief and are not seen to be core components by the large LLVM community, then as we “demote” them to tier 2, we also remove them from the noisy testing (on both pre/post merge).

Tier 2 testing is the responsibility of the sub-community that cares about it. If they want pre-merge tests for those, they’re responsible for making sure the fixes are proposed as soon as possible for the patches in flight. None of those pre-commit tests should warn the rest of the community, or it would get hard to know which bot results we should “respect”, especially for newcomers.

  1. How can we automatically detect the tiers of the projects in the pre-merge build scripts? If we want to treat them differently we somehow need to be able to distinguish these in the repo.

It should be very simple to do that, but my second take had the mistake of listing projects by name. Others have suggested we don’t do that and let the boundaries be more subtle. I very much like that.

The idea is that there are concepts of tiers, and what goes in/out will change with time, depending on how big a sub-community a particular piece of code has, or on its experimental status, or if/when incubator projects move into the monorepo.

Initially, everything outside of the mono-repo is tier 2, which matches the incubator policy quite well. Inside the monorepo, there are a few rules of what’s tier 2 (experimental, non-CMake build systems, editor/tools config, non-essential scripts), with everything else in tier 1.

With the requirement of “being tested with a noisy bot” in tier 1, everything that is already noisy-tested now is in tier 1. If we want to demote something, we need to be explicit about it and allow the creation of a sub-community to form.

Makes sense?

cheers,
–renato

#11

Hi Renato,

Thx for clarifying this!

My view is that we shouldn’t change how we test, either pre merge or post merge. If we have official tests for something already, those must pass.

Just to be clear: If a patch modified something in /llvm and causes a failing test (in ninja check-all) in something Tier 2 (e.g. polly), this would count as a failed pre-merge test. Is that what you wanted to have?

Tier 2 testing is the responsibility of the sub-community that cares about it. If they want pre-merge tests for those, they’re responsible for making sure the fixes are proposed as soon as possible for the patches in flight. None of those pre-commit tests should warn the rest of the community, or it would get hard to know which bot results we should “respect”, especially for newcomers.

How would someone from a Tier 2 project find out that an in-flight patch breaks their Tier 2 project? How should they be notified? How long should the owner of the patch wait before they land it anyway?

At the moment pre-merge testing is only reporting results back to Phabricator. AFAIK there is no notification mechanism beyond that.

  1. How can we automatically detect the tiers of the projects in the pre-merge build scripts? If we want to treat them differently we somehow need to be able to distinguish these in the repo.

It should be very simple to do that, but my second take had the mistake of listing projects by name. Others have suggested we don’t do that and let the boundaries be more subtle. I very much like that.

If you want people (and tools) to behave differently on tier 1 vs. tier 2 projects, it should be clear in which category something falls. I agree, the policy should be something generic and high-level so we don’t have to change it all the time. However at any given time it should be clear on what the tiers are. If I look at a file in the monorepo, I would like to have an easy way to find out which tier it belongs to.

Initially, everything outside of the mono-repo is tier 2, which matches the incubator policy quite well. Inside the monorepo, there are a few rules of what’s tier 2 (experimental, non-CMake build systems, editor/tools config, non-essential scripts), with everything else in tier 1.

For pre-merge testing, we’re only using CMake in the monorepo, so that already has a very narrow scope and excludes some tier 2 projects.
With “experimental” you mean code that matches a “./experimental/.” path regex?

Best,
Christian

#12

Just to be clear: If a patch modified something in /llvm and causes a failing test (in ninja check-all) in something Tier 2 (e.g. polly), this would count as a failed pre-merge test. Is that what you wanted to have?

I think that’s the consensus, yes. My take 2 separated projects in an attempt to clarify things and only made it worse, ignore that.

Polly and others I listed in tier 2, are a core part of LLVM, even if not many people care, and we should not break it.

The reason why I included Polly is because it has some non-LLVM components (ex. ISIL) which are not part of LLVM, but that’s an orthogonal issue.

How would someone from a Tier 2 project find out that an in-flight patch breaks their Tier 2 project? How should they be notified? How long should the owner of the patch wait before they land it anyway?

That’s up to each sub-community. If they want to set a pre-merge check on their own areas they need to make sure that, first, it won’t stop patches from landing (ie. not noisy) and second, their concerns should be voiced promptly and we may decide to merge it anyway if the costs of not doing so would be much greater.

I think this is why we need a tier 2 policy in the first place.

For example, an important fix needs to land and it builds fine on CMake, but not on Bazel, and the fix isn’t trivial. Merging the important fix is more pressing than having a stable Bazel build. The Bazel sub-community will then work to fix that, after the merge.

There may be core (tier 1) fixes in the subsequent Bazel fix, which is perfectly fine unless the fix breaks other tier 1 buildbots. The sub-community will work and most other people won’t even look at the reviews, but the buildbots going red means they have to revert the patch and try again.

In some cases the sub-community is very responsive and can perhaps fix things on the fly, which is fine as per current post-merge review policy, but others may be slow, so revert is the safest thing to do. No new policies here.

At the moment pre-merge testing is only reporting results back to Phabricator. AFAIK there is no notification mechanism beyond that.

That’s perfectly fine. We don’t want to change anything that we already do with this policy.

If you want people (and tools) to behave differently on tier 1 vs. tier 2 projects, it should be clear in which category something falls. I agree, the policy should be something generic and high-level so we don’t have to change it all the time. However at any given time it should be clear on what the tiers are. If I look at a file in the monorepo, I would like to have an easy way to find out which tier it belongs to.

Right, that’s why I started listing things, but that doesn’t scale. Some people suggested we separate by directory, which is also nice, but could end up dividing in weird ways, like “tier1_scripts” vs “tier2_scripts”.

One easy way to do that is to link the definition of having “noisy checks” to be tier1 and “silent checks” to be tier 2, while silent for the whole community doesn’t mean silent for all sub-communities.

We already have the policy to create buildbots and have code owners for new parts of the code, or when moving things out of experimental, so there’s a clear promotion into tier 1. And if a sub-community wants to add a new noisy check, they need to convince the rest of the community to accept that, which essentially means it’s not supported by all, and therefore, tier 1.

For pre-merge testing, we’re only using CMake in the monorepo, so that already has a very narrow scope and excludes some tier 2 projects.
With “experimental” you mean code that matches a “./experimental/.” path regex?

Not exclusively. Also things that don’t build or run by default, like new back-ends, new passes, new command line options.

Those things usually have silent buildbots curated by their sub-communities. To move the silent checks to noisy, the sub-community needs to promote them out of experimental by consensus of the rest of the community, as per the experimental policy, so it’s easier to define.

I know it sounds like soft borders will make it hard to understand what is what, but this is the actual intent. We can use our other existing policies to define how much we collectively care about things, and this policy to make explicit what we mean by “care”.

Makes sense?
–renato

#13

Hi Renato,

Thx for the clarification. I think I get your point and I see how challenging it is to find a good policy there. Thank you for working on this!

#14

Just want to chime in with support. I like the general direction and agree with the suggest tweaks mentioned so far in this thread.

Philip

#15

Thanks Philip!

First draft at: https://reviews.llvm.org/D90761

Feel free to copy more people.

cheers,
–renato

#16

I took a look and left some comments, but overall it’s looking good to me. I just wanted to say thank you very much for driving this!

Thank you,

Christopher Tetreault