Proposal 2: PR Approvals #2792
Replies: 5 comments 3 replies
-
|
Please clarify: Why does something labeled "dev" need any approval? Isn't the approval process for code going from "dev" to production? |
Beta Was this translation helpful? Give feedback.
-
|
Beta Was this translation helpful? Give feedback.
-
|
Beta Was this translation helpful? Give feedback.
-
|
if I understand correctly the thread, is the below --> <-- what Dan is suggesting (please correct)? Patches, Bug Fixes and Documentation:
Minor:
Major including DSL (Non technical): (Lyteck: what does "non-technical" mean?)
** from different organizations |
Beta Was this translation helpful? Give feedback.
-
|
Reference previous issue on PR approvals, #2129. |
Beta Was this translation helpful? Give feedback.
-
|
PR Category Classifications.pptx I've updated the original PR approval matrix to address risk based approvals. |
Beta Was this translation helpful? Give feedback.
-
|
A few quick questions on the updated matrix.
|
Beta Was this translation helpful? Give feedback.
-
|
PR Category Classifications.pptx
|
Beta Was this translation helpful? Give feedback.
-
Current: PRs only need one approval from a maintainer and the approving maintainer can be from the same organization.
Proposal: Any PR with a model change or DSL change will require approval by at least one maintainer from each of the three TAs. This process will be implemented using Github actions.
BL:I am concerned that requiring all three TAs to approve every change could lead to delays in the case that a TA is non-responsive. I wonder if we could refine this and distinguish between different scenarios with different risks. For instance, I think that an additive-only, backwardly-compatible change that is on the roadmap approved by the SWG should be able to be approved for deployment into dev by, say, any two maintainers. Where perhaps a breaking change that is being released into production as a new major version should perhaps require all three TAs to approve it. I think it’s worth having a discussion on what we need to balance sufficient control vs. sufficient nimbleness.
The first step of that is probably to categorize the changes by risk level. I see at least two dimensions:
• Release type - Dev vs Prod – presumably a Dev change is less risky than a prod change
• Change type – Breaking change vs. backwardly-compatible – presumably a breaking change should have a higher standard of approval. We might also want to distinguish between small changes (e.g adding a new data field, minor logic change to a function) and major changes (e.g. adding a new product). Minesh had developed a categorization scheme for labeling PRs that could be a basis for that.
You mention DSL changes, but if you mean Rosetta DSL syntax changes, that’s not under approval by CDM itself, at least not formally. Rather, that’s under control of a new group being set up under FINOS, contributed by REGNosys. What CDM can approve is the use of a new DSL version. Perhaps that’s what you meant.
Chris: I agree with Brian that it should not be a blanket requirement that every PR needs approval from each of the 3 TAs. The approval(s) required for any PR was raised very early on, and as Brian has suggested, this needs to be considered on a case by case basis. There was a Triage matrix created which was possibly a little complex, so we should consider updating it to make it simpler.
It also needs to be recognised that we want to encourage non-TA maintainers as this is a community project now. We should be talking about Maintainers rather than TAs. If we need approval from representatives of each of the 3 supported domains – derivatives, stock loan and repo – then we need to start categorising maintainers as being experts in which domain, thus allowing them to approve PRs on behalf of that domain.
My suggestion would be that every PR needs at least 2 maintainers from different domains to approve. Following on from Brian’s suggestion above, it would make sense that any PR recognised as containing a breaking change would need to be reviewed by 3 maintainers, one from each domain.
DSL changes are not overseen by the CDM working groups or maintainers and should not be referenced in our guidelines.
DS: Requiring all three TAs to approve every model or DSL change seems restrictive both in terms of requiring extensive oversight regardless of the impact of the change and in that it removes the opportunity for maintainers who are outside of the TAs to guide CDM's direction.
What about releasing the constraints based on the extent of the change? IE, something like:
• Minor changes: two maintainers at least one of which comes from a different organization than the one proposing the change. This would also apply to a patch release when bundled into the scheduled Minor change
• Urgent defect resolution: this may require additional scrutiny since this type of release might have immediate timing with limited testing. Rollback of a change might be approved with less oversight than forward fixes.
• Major changes: approval of the SWG
David: Agree with Brian’s and Chris’s comments that a risk-based assessment is better, and that singling-out “TAs” works against the objective of broadening the maintainership. As they mention we already have a kind of risk-based matrix. It is worth revisiting / simplifying (using the experience accumulated in the last 12m since contributing to FINOS) rather than throw-away entirely.
One aspect that should be clarified is what counts as “approval”: approvals from multiple people inside the same (maintainer) organization count as 1, and approvals from the same organization as the contributor don’t count.
The role of the community should also be recognised. Beyond the formal safe-guards that are enforced before merging code in GitHub, contributors typically seek to garner consensus around their change through the relevant WGs first. The matrix has some scenarios where such approval is explicitly required.
Ensuring relevant maintainers are lined up to approve changes is challenging today, increasing the number of approvals for each change regardless of risk impact will paralyse development of the CDM
We don’t know if Chris’s “domain” approach is the right one, because CDM domains are much more fluid than the 3 TA domains. In our view a “good” PR balances 3 objectives:
It addresses the use-case that it was designed for. That use-case domain may be “Equity Swaps”, “Commodities”, “Collateral”, “Reporting”, etc.
If the concept has applications in other domains, it is implemented in a way that has reusable value there.
It is implemented consistently with the broader design principles
Typically 1 has tension vs 2 and 3. While 1 can be assessed by experts from that use-case’s domain, 2 should involve experts from other domains and 3 involves agnostic policing of the model’s good design.
Where there are topics/WGs that exist outside of “obvious” domains or across TA domains (e.g. should the proposals to add a digital assets WG) the concept of approving in this manner makes no sense.
On the DSL point: we agree it should not be part of the remit, at least not on a routine basis. That said, there are cases where planned CDM developments may demand DSL changes. In this case, it’s a good idea to give the CDM community some oversight on the changes being made. This will be facilitated by the DSL’s imminent FINOS contribution.
AH: I think Dan's input here is quite on point, but it will require a clear, objective definition of a Minor and major Change (which can be quite subjective to decide between) and what constitutes an Urgent defect (the only variant here that comes to mind would be if there is a party that is being directly affected by a defect in a severe way and reports it as such). The discussion around these definitions can be quite difficult and lengthy to conclude.
I propose starting with a simple approach that fixes the current issue but is not too restrictive: require at least two maintainers to approve and at least one from a different organization.
MG: This seems more restrictive that what we currently have. I think it's already difficult to get the approvals from two TAS. I agree with @dschwartznyc 's suggestion to have a layered approach and have:
Patches, bug fixes, one maintainer is enough
Minor changes: two maintainers at least one of which comes from a different firm than the one proposing the change.
Major changes, including DSL changes: SWG
Maintainers should review that the labelling of the PRs regarding the type of change is accurate.
Result: General agreement that the current process should be changed especially for major changes but also minor changes would require at least two approvers.
A problem here is that we have no way to enforce the rules as they are just labels.
Possible solution:
Based on the above feedback the general opinion is that approval by three TAs is too restrictive and approvals should be based on "risk" which is taken to mean model changes and complexity. There is general agreement on having more than one and from separate organizations.
To further clarify categories of changes:
Patches, Bug Fixes and Documentation:
Dev release patch: 1 maintainer
Production patch: 2 maintainers
Minor:
Dev and Prod - Two maintainers from different organizations
Major including DSL (Non technical):
Dev and Prod - Three maintainers
Beta Was this translation helpful? Give feedback.
All reactions