[bstefanuk]

Add documentation on the proper process to cherry pick changes from the monorepo into pre-monorepo branches. In particular, release-staging/rocm-rel-x.y branches.

[stanleytsang-amd]

Hmm, I can imagine a corner case where this policy would be problematic, if I'm reading this correctly. Let's use 7.0 as a toy example

1. release-staging/rocm-rel-7.0 branch gets created from develop branch, contains 7.0 commits.
2. Work now begins on 7.1 stuff. 7.1 PRs get merged into develop branch. Eventually, there might be a 7.1 PR which causes the code to be fundamentally incompatible with 7.0 code in the release-staging/rocm-rel-7.0 branch.
3. If I want to cherry-pick a fix for 7.0 into the release-staging/rocm-rel-7.0 branch, with the new policy it would have to land in develop first, but the issue is, the code contained the 7.0 cherry-pick PR may not be compatible with the 7.1 code in develop

I suppose we could do 7.1 development work in a different branch during the 7.0 cycle, but that's a divergence from our current workflow and that's a pretty significant amount of time. Maybe I misread something.

[malcolmroberts]

We need to have the context of how repos are moved over (eg, if history is preserved, and, if so, how) in order for this procedure to make sense. I'd suggest making a PR that documents how libraries are moved from separate to combined repositories first.

[evetsso]

Hmm, I can imagine a corner case where this policy would be problematic, if I'm reading this correctly. Let's use 7.0 as a toy example

Maybe a more concrete example is if something needed to be reverted only in the release-staging branch, like something accidentally got into the branch that wasn't supposed to be there and needed to be taken out. You wouldn't want to have to take that out of develop as well.

[bstefanuk]

Hmm, I can imagine a corner case where this policy would be problematic, if I'm reading this correctly. Let's use 7.0 as a toy example

1. release-staging/rocm-rel-7.0 branch gets created from develop branch, contains 7.0 commits.
2. Work now begins on 7.1 stuff. 7.1 PRs get merged into develop branch. Eventually, there might be a 7.1 PR which causes the code to be fundamentally incompatible with 7.0 code in the release-staging/rocm-rel-7.0 branch.
3. If I want to cherry-pick a fix for 7.0 into the release-staging/rocm-rel-7.0 branch, with the new policy it would have to land in develop first, but the issue is, the code contained the 7.0 cherry-pick PR may not be compatible with the 7.1 code in develop

I suppose we could do 7.1 development work in a different branch during the 7.0 cycle, but that's a divergence from our current workflow and that's a pretty significant amount of time. Maybe I misread something.

Please see the comment I have posted regarding an addition to the FAQs.

[bstefanuk]

Hmm, I can imagine a corner case where this policy would be problematic, if I'm reading this correctly. Let's use 7.0 as a toy example

Maybe a more concrete example is if something needed to be reverted only in the release-staging branch, like something accidentally got into the branch that wasn't supposed to be there and needed to be taken out. You wouldn't want to have to take that out of develop as well.

This is a fair edge case, we can add it to the FAQs. Please recommend a specific question and provide an answer so I can get a feel for how you would like the provide guidance to developers on how to overcome this. @evetsso @stanleytsang-amd.

[bstefanuk]

We need to have the context of how repos are moved over (eg, if history is preserved, and, if so, how) in order for this procedure to make sense. I'd suggest making a PR that documents how libraries are moved from separate to combined repositories first.

@malcolmroberts I hear you, but isn't your question more related to a component's migration to the monorepo? The current document is only intended to provide a best practice reference for how to reflect cherry-picks to release-staging in pre-monorepo projects. If you haven't migrated to the monorepo, this document is irrelevant until that time.

Edit: Either way, thank you for providing an actionable suggestion in your comment! I actually think this is a great idea so that we can clarify what the process looks like for devs that see this coming down the pipe. For now (though I don't think this is expressly what you were asking for) here's an example of a PR that was migrated from Tensile into the monorepo: #212

[stanleytsang-amd]

Apologies in advance if I'm missing something here, but I do have a fundamental question about this:

Why are we even instigating this change in procedure at all? From what I understand from the existing accepted software release process set out for Math Libs, we are allowed and are supposed to target PRs directly into the release branches (as opposed to targeting develop and then subsequently cherry-picking them into the release branch). Then we merge back the PRs into develop as necessary. In my opinion, the existing method is less problematic (ie. the issue I brought up, which you did address, but it is still extra work compared to the existing workflow) and it would still be compatible with the requirements of updating the original public repo branches as outlined in this PR. Is there some technical reason why we're doing this change? What am I missing?

[bstefanuk]

Apologies in advance if I'm missing something here, but I do have a fundamental question about this:

Why are we even instigating this change in procedure at all? From what I understand from the existing accepted software release process set out for Math Libs, we are allowed and are supposed to target PRs directly into the release branches (as opposed to targeting develop and then subsequently cherry-picking them into the release branch). Then we merge back the PRs into develop as necessary. In my opinion, the existing method is less problematic (ie. the issue I brought up, which you did address, but it is still extra work compared to the existing workflow) and it would still be compatible with the requirements of updating the original public repo branches as outlined in this PR. Is there some technical reason why we're doing this change? What am I missing?

From my perspective the process being outlined here reflects the current recommended approach within math libraries: (i) merge to develop, (ii) cherry pick to release-staging or release, with merge for release being gated by CQE. We must have been given conflicting advice about this procedure. We've been using this pattern in Tensile and hipBLASLt for several releases. However, we are aware that the standard for this differs between components.

[stellaraccident]

Apologies in advance if I'm missing something here, but I do have a fundamental question about this:

Why are we even instigating this change in procedure at all? From what I understand from the existing accepted software release process set out for Math Libs, we are allowed and are supposed to target PRs directly into the release branches (as opposed to targeting develop and then subsequently cherry-picking them into the release branch). Then we merge back the PRs into develop as necessary. In my opinion, the existing method is less problematic (ie. the issue I brought up, which you did address, but it is still extra work compared to the existing workflow) and it would still be compatible with the requirements of updating the original public repo branches as outlined in this PR. Is there some technical reason why we're doing this change? What am I missing?

No practices at any other eng shop of this scale agrees with the current ad-hoc approach. Most of them that I have been involved with have a strict policy that changes land on main and get cherry-picked to release branches. And then there is usually a high level/senior/central set of people who can override that in an emergency. It's kind of a foundational principle of CI/CD. If there are very short term needs based on our current "development process", then we can make exceptions, but everyone is going to start marching towards a unified CI/CD methodology.

[stellaraccident]

Let's land this. You can label it as DRAFT if people want to discuss more. But it is a step in the right direction.

[YvanMokwinski]

@jsandham

[stanleytsang-amd]

Apologies in advance if I'm missing something here, but I do have a fundamental question about this:
Why are we even instigating this change in procedure at all? From what I understand from the existing accepted software release process set out for Math Libs, we are allowed and are supposed to target PRs directly into the release branches (as opposed to targeting develop and then subsequently cherry-picking them into the release branch). Then we merge back the PRs into develop as necessary. In my opinion, the existing method is less problematic (ie. the issue I brought up, which you did address, but it is still extra work compared to the existing workflow) and it would still be compatible with the requirements of updating the original public repo branches as outlined in this PR. Is there some technical reason why we're doing this change? What am I missing?

No practices at any other eng shop of this scale agrees with the current ad-hoc approach. Most of them that I have been involved with have a strict policy that changes land on main and get cherry-picked to release branches. And then there is usually a high level/senior/central set of people who can override that in an emergency. It's kind of a foundational principle of CI/CD. If there are very short term needs based on our current "development process", then we can make exceptions, but everyone is going to start marching towards a unified CI/CD methodology.

If the order comes from the top I won't argue, but I just wanted to describe a scenario which is not uncommon at AMD, in which I hope would fall under the "emergency override category":

Let's say for some release (7.0 as an example), post creation of the release-staging/rocm-rel-7.0 branch, there is a bug somewhere in the ROCm stack, which results in say, a performance regression in one of our math libs (rocPRIM, for example). The ROCm stack bug is fixed in 7.1, but did not make it in time for 7.0. As such, to regain the lost performance in 7.0, a code workaround is required for rocPRIM.

In this scenario, it would make little to no sense to me to land this workaround in develop branch (which is now effectively for 7.1 work) since the ROCm stack bug does not exist in 7.1. In this case, are we allowed to submit the code workaround PR directly to the release-staging/rocm-rel-7.0 branch?

[amd-jnovotny]

Skimmed for obvious errors. Looks good. Will do a more complete review of the final copy.

[bstefanuk]

The document will be merged in a draft state. I have created a discussion issue to collect follow up thoughts, see: #234