More robust PR template and contributing guidelines#827
Conversation
|
@genedan @henrydingliu @jbogaardt please share your thoughts, also open to everyone watching this thread. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #827 +/- ##
==========================================
+ Coverage 86.40% 87.33% +0.92%
==========================================
Files 86 86
Lines 4959 5202 +243
Branches 642 718 +76
==========================================
+ Hits 4285 4543 +258
+ Misses 478 466 -12
+ Partials 196 193 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
I'd prefer keeping the community as open as possible, so I'd rather not have the credentialed actuary checkbox in there. Once we expand our Contributing guide to include detailed information about our review process, I think we'll be in a better place. I'm just brainstorming here but I would include things such as:
I would recommend softening / shortening the language a bit. Words like "personally responsible" and "attest" might be too strong. Perhaps: We're the reviewers, so it's really us who are responsible for what winds up in the code base. What we can do though is simply hit the close button on any overly-verbose, AI-generated PRs, a practice which would encourage people to fill out the templates in their own words and keep the PRs short. We can also include in the contributing guide that although AI tools are allowed, it's the people submitting the PRs will have their (pseudo) names and emails in the commit history, so they are encouraged to review every line of code that they submit. I'm not sure how to succinctly put this bit on the checkbox, but would be open if you found a way to say it elegantly. |
|
Thanks so much for your feedback. I really like the revised checkbox and I think that makes a lot of sense. I'll make a revision. I also thought we should acknowledge those that make meaningful contributions, and not only that, we also want the public to know that this package is maintained by actuaries that know what they are doing (credentialed, etc). |
|
I'd also recommend moving the Contributing Guide to be a top-level section in the toctree. Using Pandas as a model, theirs is very detailed: https://pandas.pydata.org/docs/development/contributing.html Ours is a lot shorter now, but eventually I plan to add (or review) analogous pages on the review process, best practices, style guides, etc. |
|
That looks really good, but I also feel like it's so much that no one will read it... @henrydingliu can you take a look at this and give some thoughts? |
|
Good point, I think having the credits in the release notes is a good step. I'm trying think of a good way to word the credentials checkbox in a way that makes it clear that the submitter does not have to be credentialed, but can disclose that they are if they want. Something like: That's kind of a mouthful though. Still open to further brainstorming on this. |
|
How about: |
|
wait, did you not receive a notification for my comment yesterday? did i forget to click submit again? |
|
To clarify, I’m entirely fine with PRs from uncredentialed collaborators. My main concern was to make sure we maintain strict professional standards, as rendering actuarial services without proper disclosure is technically a no no. I might be overthinking from the professionalism angle, and I definitely don't want to discourage contributions. We can probably do better with the wording. Also, regarding the PR checklist: it doesn't need to be 100% checked off. Many repos actually include unchecked "trapdoor" items (like "I am a robot") specifically to make sure you are thoroughly reading the list rather than just blindly checking boxes. @henrydingliu what is your comment, I still don't see anything. |
|
lost the tab :( just gonna retype it i have an entirely different mental model when it comes to software development. i literally killed myself in my first year at amazon because i was the only actuary over a book of a few billion dollars and therefore thought i had to personally inspect every piece of ETL that we used. in the end i realized the more mature thing to do is to write better requirement, establish clearly defined entry and exit points, in order to leverage all the developers around me without needing them to understand an ounce of insurance. business professionals all over the world are bad at explaining specs to developers but actuaries are just so conceited that we end up coding stuff ourselves. i don't know how much an actuary was involved in actually coding resq. but i do know that actuaries were definitely not involved in developing excel, python, or the BA II. basically we as a profession have always used tools developed by non-actuaries. this package doesn't have to break from that precedent. when we require, or just encourage, PR submitter or reviewer to be actuary, what we are functionally doing is to skip explicitly documenting specs and requirements, and entirely relying on our common CAS general education to align on requirement on a de facto basis. in theory, the cas can just hire a bunch of developers with a staff actuary in charge to develop the package. that actuary doesn't have to review a single line of code. if anyone can explain how their feature request related to the practice of reserving, and how their pr faithfully implement that feature request, it shouldn't matter who that person is. just because that person is likely to be a practicing actuary, doesn't mean being a practicing actuary is why their request or PR is valid. along those same veins, using our personal credibility or credential as the collateral on a PR review is another form of over-reliance on the social contract around the CAS general education. none of us have ever looked at a single line of excel's source code, yet we have no problem using it, because we have a whole different mental model around how to vet something for which we can't see the source code. none of us looked at the underlying xml for the last excel file we render actuarial service in. yet we were all proud to have delivered that file as our work. again, because we have a different mental model for vetting that file. but that mental model should apply regardless of whether we can see, and therefore attempt to understand, the source code. i want us to be able to collaborate with someone who vibe codes a whole polars backend for this package. (if you think about it, the more organized and explicit we are about that superclass, the easier it would be to vibe code any backend). our test suite should give us the confidence to be able to accept that PR without reviewing a line of code. take my review of the geomean implementation. the package relying on my experience as an actuary to point out a concern around logging negative numbers is actually a very unserious way of software development. ceded/deductible/salvage-subro is a common occurrence of negative triangles in practice. having those in the test suite is how we should be reviewing a geomean implementation. we should be able to articulate why we feel personal review of every diff is critical. whether that is speed, accuracy, or style, once articulated, we can develop mechanical tests to objectively measure every PR accordingly. |
|
Thanks for sharing... I actually resonate a lot with what you said... "Vibe coding" is such a weird term, and it isn't black and white. The general definition from what I've read is implementing code without writing any of it yourself. However, this really sells the concept short; if you engineering prompts carefully and guide the AI with intension and good thoughts, the output can actually be very impressive, if not better than when someone actually writes the code themselves. That said, what are your thoughts on my current proposal? I am open to removing the checklist on being an ACAS/FCAS if you think we can maintain the same amount of care when reviewing PRs, rather than relying on a social contract that credentialed actuaries know what they are doing. My goal is simply to establish guardrails so when the package becomes more popular in a few years, people will look at it and say "ya, I trust this". |
Summary of Changes
Improved the contribution guidelines and the PR template
Related GitHub Issue(s)
N/A
Additional Context for Reviewers
To better protect the quality of work of this package, I'd like to propose we put a few more guardrails on PRs so that anyone looking at this package knows there's a certain level of quality that is expected.
The CAS Primer says that we should have a human in the loop and discloses AI tools used.
The code says only Qualified actuarial on the with basis of basic, continuing ed, and experience met shall perform actuarial services. I think we should ask pull requests to make a self declaration if that's not the case.
There's also a bunch more relevant ASOPs that I skimmed through, like ASOP 23 Data Quality, ASOP 41 on Actuarial Communications, and ASOP 56 on Modeling that we should all consider, but I don't think it's necessary to bring them all in as a checklist in the PR template.
Anyway, this is coming in as a PR, and I don't want to just introduce red tape, I'm also not married to these at all. Open to discussion.
uv run pytest) and documentation changes (uv run jb build docs --builder=custom --custom-builder=doctest)Note
Low Risk
Low risk documentation-only changes that adjust contributor expectations and checklists without affecting runtime code or releases.
Overview
Updates the PR template to add inline guidance for writing summaries, linking issues, and providing reviewer context, plus a new checklist covering local test runs, CAS credential self-attestation (ACAS/FCAS), and confirmation that the author reviewed all diffs.
Revises
docs/library/contributing.mdto explicitly require using the PR template, running tests viauv, disclosing AI coding agent usage, and obtaining independent peer review, while also tightening the wording and commands around doc/test execution.Reviewed by Cursor Bugbot for commit f469869. Bugbot is set up for automated code reviews on this repo. Configure here.