CDM PR and Release Governance

[tomhealey-icma]

This issue consolidates previous discussions and meetings on the release governance proposals that have been put forth. Please refer to these past issues:

2423
2305
2422

Latest proposal and comments:
2665

The comment period on the latest proposal closed on March 11. A summary of comments will be posted here.

[lolabeis]

@tomhealey-icma I wanted to suggest that we port this issue into a GitHub "Discussion", but maybe we don't need a new Issue (or Discussion) at all

The proposals being debated in #2665 can be categorised as:

1. PR labelling, review and approval (Proposals 1 and 2) --> Already covered in CDM - Development Operating Model - PR Labels and processing #2423
2. Release meaning and frequency (Proposals 3 and 4) --> Already covered in Is the CDM Versioning working? #2305
3. Definition and process for backward-compatibility (Proposal 5) --> Somewhat covered in CDM Design Principles #2422, although initially as an off-topic digression. Maybe we should repurpose that discussion to be specifically about "backward compatibility" rather than "design principles"? @brianlynn2's latest deck that was presented at the SWG definitely belongs there.

Personally I would keep these 3 topics separate rather than folding them into a big issue, because it would allow us to selectively close some while others are still debated. A summary of the comments that have been provided in response to the SWG discussion can be posted in each relevant discussion topic rather than here.

[dschwartznyc]

To the extent that it's possible, the reuse and independent resolution of existing issues makes a lot of sense.

Many of these concerns are interrelated. The definition of the types of change is an obvious example as it determines to what type of release a change belongs, and, by implication, whom should be able to approve that change and the frequency/schedule of each category of each type of release.

Perhaps a way forward is:

1. Agreement on the definition for each type of change (urgent defect resolution, minor change, major / breaking). Arguably this is the first part of Alternative CDM product representation  #2 above (Release meaning and frequency) and part of Adjustments to the composite cdm model #3 (Definition and process for backward compatibility)
2. Agreement on the approval process for each type of change. updated maven group id #1 (PR labeling, review, and approval)
3. Agreement on the frequency of release for each type of change. The second part of Alternative CDM product representation  #2.

FWIW, I suggest parking a comprehensive discussion of backward compatibility as it is broader than the governance questions.

[tomhealey-icma]

Based on all the meetings on this topic there are currently five separate proposals that need to be agreed upon. There may be a need to further define elements of each proposal as it relates to Dan's comment. I've summarized the feedback that was received as of March 11th.

Proposal 1: Draft PRs and Ready for Review

Current: Most PRs are submitted as “review ready” even if they are not labeled.
Proposal: All PRs will be submitted as Draft. A contributor/editor can move from Draft after all labels are properly set. This process will be implemented using Github actions.

Feedback:

BL:This seems ok to me. We’ll need to define who can change the labels, and what labels should be used to trigger reviews and then implementation.

CR: What is the definition of a Draft PR please? We’re waiting on FINOS to give us Triage permissions on the Editor role which will allow contributors to add the labels themselves which should help here. We still need to cater for general Participants submitting PRs though so a Draft status would make sense. The more that can be done automatically the better.

Result: General agreement. Draft is defined by Github. Moving to Ready state would require proper labelling.

Proposal 2: PR Approvals

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:

1. It addresses the use-case that it was designed for. That use-case domain may be “Equity Swaps”, “Commodities”, “Collateral”, “Reporting”, etc.

2. If the concept has applications in other domains, it is implemented in a way that has reusable value there.

3. 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:

1. Patches, bug fixes, one maintainer is enough
2. Minor changes: two maintainers at least one of which comes from a different firm than the one proposing the change.
3. 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:
Proceed with the more restrictive approach using just the labels. If we find that it’s not working we will have two options:

1. A volunteer would be assigned to review and approve the labelling.
2. We develop a script to check for model changes and test file changes.

Proposal 3: Dev Release Cycle

Current: Dev minor and patch release are ad-hoc.
Proposal: Dev minor releases and unreleased patches will be automatically released weekly unless there are no approved PRs. Patches can be released as needed. Minor and patch releases must be backward compatible.

Proposed rewording : Approved minor changes and patches will be automatically released into dev weekly (unless there are no approved PRs). Urgently needed patches can be released as needed on approval from two maintainers. Minor and patch releases must be backward compatible.
Chris: I’m struggling to understand the need for scheduling these. I’m guessing the issue is that once a PR is merged there can often be a delay before it is put into a dev release i.e. the build process is run? In which case I think Brian’s wording is better. I’m also trying to understand what a dev minor release is? So if we have version 6.0.0-dev.nnn as the dev major version, are we referring to 6.1.0-dev.nnn as the minor release? For there to be a 6.1.0-dev though we would have to have already put 6.0.0 into production. So I think this is actually just referring to dev upgrades in general e.g. 7.0.0-dev.123, 6.1.0-dev.321?

AH: I do not understand what we want to achieve by having a weekly cycle instead of the current way. We would have fewer dev releases, and multiple changes would be bundled together in a release. This comes with the downside that @dan mentioned above, but I don't see the upsides.

I agree with @ahutusoru123. Not sure why we need to establish a weekly cycle. Is this a cost issue?

David: Agree with Chris that there is no such thing as a minor or patch release for Dev. We only increment one “x” tag in 6.0.0-dev.x. We don’t see the use-case for enforcing a release schedule for these. Also no backward-compatibility requirement (that’s the point of having a dev branch to make those changes)

Result: Seems fewer participants support regularly scheduled releases and instead prefer an ad-hoc approach. While this serves the needs of the contributors it may not be the best approach for the wider community. To be discussed.

Proposal 4: Dev to Prod Release Cycle

Current: Prod releases occur approximately once per year but date is not fixed
Proposal: Prod minor releases will be based on contributor request and approval by CRWG. Prod major releases will occur once per year on a fixed date set by the Steering Committee. Where a non-backward compatible change is approved, an ad-hoc prod release can be requested.

BL:>> I would propose the following:
Prod minor releases:
Planned changes will be promoted from dev to prod on a frequency/schedule to be agreed by the CRWG based on the planned work. The planned minor releases should be made no more than weekly and ideally within one month of promotion to dev. Unplanned changes (patches) to correct issues can be made more quickly, with approval by two maintainers, depending on the severity of the issue. Critical and severe errors (those preventing CDM to be used for its core mission) can be corrected without first deploying to dev, with approval of the maintainers. Less severe issues should first be deployed to dev prior to promotion to prod.

Chris: I would suggest that anything going into production should be agreed by the SWG and not just the CRWG. The CRWG can review individual PRs, but agreement of when to cut a new prod release (major/minor/patch) should be approved by the SWG. This in turn may mean that ad-hoc meetings of the SWG are required to approve delivery of a new prod release. In general I’m likening the SWG to the Product Owner of the CDM, and in my experience anything going into production would need to be approved by the product owner.

DS: Please clarify what is meant by "Dev minor releases" as it seems to imply constraints on releases of code in development and one would think that these should take place with no constraint to facilitate collaboration and testing.
WRT changes to production code:

Patches and Minor changes: please confirm that patches and minor releases will be released to Production on at least a weekly basis with the potential that patches can be released immediately to production depending on the severity of the defect to be resolved.
Major changes: is the proposal to restrict major releases to once annually? If so, it would seem to limit CDM's adoption and development. Instead, perhaps it would make sense to have at least one annual release with more at the discretion of the SWG.

MG: If we want to expand the adoption, we cannot have only one Production version per year. Since we are generating new content rapidly, production versions will need to be generated more frequently than when the standard is more mature. We've seen this in other financial standards such as FpML and FIX. The CDM Roadmap based on content should define the timeline for Prod releases based on the timeline for adding new content that is needed to implementers. For example, the Collateral WG may want to have a Prod release in the summer and the Products and Events sooner than that. The CDM Roadmap should include an attempt to schedule the expected Prod releases for the year based on the book of work of each working group.

David:We don’t agree with the proposed change, we broadly agree with Brian’s proposal, but we don’t agree with Chris’s proposed alternative 🙃.

Prod release once per year at a fixed date: It should be roadmap-based instead and driven by the SWG – see Brian’s suggestion for prod major release below.
Agreement of when to cut a new prod release (major/minor/patch) should be approved by the SWG: We think that should only apply to major (see roadmap comment above), not minor or patch. While I agree that the SWG acts as a PO, it should not be in the minutiae of minor or patch changes to Prod, which by design should be backward-compatible anyway – it delegates authority to the sub WGs for that. Where the SWG should be invoked ad-hoc is if a minor or patch release is required that breaks backward-compatibility – which should be on an exception basis. Effectively it’s equivalent to cutting a new major earlier than anticipated – Brian also covers that case below.
In summary SWG should agree and set the road-map but CRWG should be in charge of collating content for and executing the release. Similar to the manner in which it has recently started to do so.

Proposal 5: Definition of Backward Compatibility

Current: Like other types of software, backward compatibility in the context of a domain model means that an implementor of that model would not have to make any change to update to such version.
• Prohibited changes:
o Change to the structure (e.g. the attributes of a data type or the inputs of a function) or removal of any model element
o Change to the name of any model element (e.g. types, attributes, enums, functions or reporting rules)
o Change to any condition or cardinality constraint that makes validation more restrictive
o Change to the DSL that results in any existing expression becoming invalid
o Change to the DSL that results in change to any of the generated code's public interfaces
• Allowed changes:
o Change that relaxes any condition or cardinality constraint
o Change to any synonym that improves, or at least does not degrade, the mapping coverage
o Addition of new examples or test packs
o Change to the user documentation or model descriptions
o Addition of new data types, optional attributes, enumerations, rules or functions that do not impact current functionality
Proposed:

Prohibited changes:
All the above plus:

o Any DSL change that results in any prohibited change or change in a test case input or output file.
o A public interface shall be defined as any generated API or function that can be called from software or service outside the CDM.

BL: >> Back in September I had developed a more comprehensive set of proposals around change control. See in the attached PowerPoint. The intention is to provide some clearer definitions of what is to be controlled and how, while also providing a certain amount of wiggle room so that non-critical changes don’t force a major release. David asked me to present this to the SWG next month.

Chris: We cannot stop another project/application (the DSL) from performing releases so the first of the two new bullet points cannot be added. I also wonder whether we should revisit the 2 Prohibited Changes associated to the DSL in this context?
I don’t follow what the second bullet point is suggesting should be prohibited? Can you elaborate please?
I would also probably make it clearer in the Prohibited Changes list that existing elements from within an enum cannot be removed

DS:
• The terminology of this proposal is also confusing. For example, is the intent for using the term "prohibited change" to describe those that must be migrated to the next Major release and cannot be included in a Minor or Patch release? If so, there is an urgent need for definitions that are very clear since "prohibited changes" must fall under the proposed annual schedule for Major releases.
• What is the categorization for the migration of data from one field to another? Is it "prohibited" for releases labeled "Minor" / "Bug-fix"?
• Are we confident that any addition of a test case should be a breaking change? What if the addition increases the test coverage of the existing model and the test passes?
• How do we classify changes to the machinery associated with the DSL such as the code-generators?
• How do we classify changes to the build process for CDM and who is responsible for its maintenance?

David: We agree that the definition of backward compatibility and the process around it should be tightened. It critically underpins many of the above proposals. It’s a topic onto itself and merits a dedicated discussion. However, rather than trying to define it by listing what it is and isn’t (incomplete), we should drive it from first principles and then illustrate it with some examples of allowed vs disallowed changes. This approach would also allow us to implement proper regression testing of backward compatibility.

There are 2 types of model entities: objects (types and enums) and functions.

For objects, we propose the following definition of backward compatible. It is based on 2 users or systems (1 producer and 1 consumer) trying to exchange information about those objects.

A change (n --> n+1) is backward compatible if a consumer on version n+1 can consume and validate any object X that has been produced and validated by a producer on version n

In this definition:

“validate” means passing all the condition and cardinality constraints
“any object X” could be restricted to objects annotated as rootType and all of their attributes, recursively.
For functions, backward compatible simply means preserving the function’s signature (inputs and outputs). And indeed we should distinguish between “public” functions (that are exposed to model consumers) and “private” ones (that are internal to the model workings) – something that we don’t have in the CDM right now.

[tomhealey-icma]

Since we have five proposals to review and it was getting confusing I've separated each proposal to a separate discussion under the Governance category:
Proposal 1: Draft PRs and Ready for Review: 2789
Proposal 2: PR Approvals: 2792
Proposal 3: Dev Release Cycle: 2793
Proposal 4: Dev to Prod Release Cycle: 2794
Proposal 5: Definition of Backward Compatibility: 2795

Please vote, thumbs up/down or comment so I can see where we stand.

[lolabeis]

Split makes sense, thanks @tomhealey-icma

[lolabeis]

Closing as superseded by the 5 sub-discussions that have been spun-out ^^