Stdpopsim: Formalise QC process

[jeromekelleher]

Opening this thread to get discussion going on how we can formalise the QC process. Once we've reached a consensus here we can add this to the developer documentation.

Here's a draft to get things going:

1. Developer A submits PR to add a new model. This must include basic unit tests and full documentation (i.e., the documentation that will be rendered on rtd). The code is checked for any obvious problems/style issues etc by maintainer M and merged when it meets these basic standards. The model is considered 'preliminary' and not linked from the online documentation and not included in the CLI.
2. Maintainer M creates an issue created tracking the QC for the model, and assigns a developer B (after looking for volunteers) to do a blind implementation of the model. M also creates an issue for the CLI implementation of the model.
3. Developer B creates a blind implementation of the model as PR within the unit testing module, and adds the automatic checking of this model for equality with the production model to the suite of unit tests. All being good, this PR is merged and the QC issue is closed.
4. Someone then makes a PR updating the CLI, checking that the documentation, citations etc all work properly, and adds the model to the list of documented models.

There's no mention of an analysis notebook here though: do we need then, or simply implementing the model blind enough?

Arbitration

When developers A and B disagree on the model implementation, the process is to

1. Try to hash out the details between them on the original issue thread
2. If this fails, contact the authors of the original publication to resolve ambiguities.

[andrewkern]

This looks great-- I think we can actually ditch the analysis notebook here which will streamline things.

[ndukler]

I agree, this looks good.

[dschride]

Agreed. This is sufficient as is.

On Wed, Apr 10, 2019 at 1:43 PM Noah Ethan Dukler <[email protected][e-mail]> wrote:

I agree, this looks good.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHubhttps://github.com/popgensims/stdpopsim/issues/55#issuecomment-481791327, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AnjwXk9ixk9vFW-hYJzqfrz3hPdWjH8Iks5vfiLAgaJpZM4cn2wp.

--
Dan Schrider
Assistant Professor
Department of Genetics
University of North Carolina at Chapel Hill
email: [email protected][e-mail]
phone: [phone]
website: https://www.schriderlab.org/

[carjed]

@jeromekelleher can we be more specific about where/how exactly the blind implementation of a model should be introduced? This is what I'm picturing (expanding on point 3 in your outline), but please correct me if you were imagining a different workflow.

1. Dev B implements the model in qc/{species}_qc.py (this will have a similar structure to stdpopsim/{species}.py) and writes a new test in tests/test_{species}.py calling the verify_equal method (#54) to compare the production and QC versions of the model. Note that tests/test_{species}.py will need to be updated to import the corresponding QC module.

tests/test_homo_sapiens.py will look something like this:

import unittest
import io

import msprime
import numpy as np

from stdpopsim import homo_sapiens
from stdpopsim import genetic_maps

# add QC imports
from qc import homo_sapiens_qc

# other tests
# ...
# ...

class TestTennessenOutOfAfrica(unittest.TestCase):
    """
    Basic tests for the TennessenTwoPopOutOfAfrica model.
    """

    def test_simulation_runs(self):
        model = homo_sapiens.TennessenTwoPopOutOfAfrica()
        ts = msprime.simulate(
            samples=[msprime.Sample(pop, 0) for pop in range(2)],
            **model.asdict())
        self.assertEqual(ts.num_populations, 2)

    def test_debug_runs(self):
        model = homo_sapiens.TennessenTwoPopOutOfAfrica()
        output = io.StringIO()
        model.debug(output)
        s = output.getvalue()
        self.assertGreater(len(s), 0)

    # new test for model QC
    def test_equal(self):
        model = homo_sapiens.TennessenTwoPopOutOfAfrica()
        model.verify_equal(homo_sapiens_qc.TennessenTwoPopOutOfAfrica())

1. Dev B pushes their blind implementation to the DevB:ModelToBeTested_QC branch on their fork and creates a PR with these changes. CircleCI reports any exceptions raised by the verify_equal test.

2. If the models differ and Dev A & B agree the __blind__ implementation needs to be changed, Dev B makes necessary changes and commits to existing PR.

3. If the models differ and Dev A & B agree the __production__ version needs to be changed, Dev A creates a hotfix PR for their changes to the production version (in stdpopsim/{species}.py), which gets merged into stdpopsim:master by maintainer M. (Will this automatically trigger a new CircleCI build on the PR opened by Dev B to check if the verify_equal test now passes? Otherwise, Dev B will need to rebase to master and push again).

4. Once the two models are in agreement, Dev B's PR is merged into stdpopsim:master and closed.

[andrewkern]

this sounds clever, although i'm unsure about the the hotfix idea here-- that would be a merge without review real review? I guess that's okay if only the model is changed in the PR.

I do think it will trigger a new CircleCI review on the open PR from Dev B too, but there would need to be a further commit from Dev B, basically pulling in the upstream changes if I follow the whole scenario

[jeromekelleher]

This all seems very sensible to me @carjed. It's arguable whether we need the qc module (we could just implement the QC blind model in the test module), but I think your approach is better. Having the QC module will make it easier to describe the workflow and make it easier to find the QC models.

WRT to point 4, Dev B will need to rebase and force-push after Dev A's hotfix is merged. Circle will rerun the tests then.

One tiny nitpick on the test:

    def test_qc_model_equal(self):
        model = homo_sapiens.TennessenTwoPopOutOfAfrica()
        self.assertTrue(model.equals(homo_sapiens_qc.TennessenTwoPopOutOfAfrica())

This way it'll be reported as a test failure rather than an error (verify_equals raises exceptions, and any uncaught exception is regarded as an error in the testing code).

[ndukler]

Sounds good, but this really reinforces how much more I need to learn about git. I guess I'll convert my qc script over to the beginning of a the homo_sapiens_qc module. The only other question I have is what to do about the fact that we kinda envisioned qc as a being part of a well documented notebook. Do we keep it that way with markdown formatting or just revert everything to comments?

[carjed]

@andrewkern Perhaps a better option for production model hotfixes would be for Dev A to add a code suggestion to Dev B's open PR, and either Dev B or Maintainer M commits the changes to that PR: https://help.github.com/en/articles/incorporating-feedback-in-your-pull-request

With this, we'd simultaneously avoid opening trivial PRs and having to go through the rebase/force-pushing dance. I don't really see any downsides, except that with a separate hotfix PR, Dev A is more likely to have validated their changes locally.

[andrewkern]

there you go-- that sounds perfect.

[carjed]

Looking at @ndukler's PR #60, it seems that my proposed workflow for hotfixes using code suggestions will only work if changes to the production model are committed in Dev B's PR, which shouldn't typically be happening. As it stands, there's no way for Dev A to independently introduce hotfixes to the production model directly in the QC PR.

Instead, I think what we can do is have Dev A submit a hotfix PR on Dev B's QC branch, rather than stdpopsim:master. If this behaves how I expect, once Dev B merges the PR on their branch, the hotfix commit(s) from Dev A should propagate to the main QC PR on stdpopsim:master. I'll give this a try with #60 to see how it goes.

[jeromekelleher]

Copying my comment on #60 here so it's in the right place:

I'm not sure I like having the fix for the production model included with the QC PR though: from a reviewer's perspective it's not obvious that the QC dev didn't go in and modify the original code to make it all agree. If the fix is committed separately, then it's clear the production model isn't touched. The Git history is also simpler to follow if we separate the two.

It seems pretty unlikely that the QC PR is going to go through without having to rebase, so I don't really see the issue behind committing the hotfix to master

[ndukler]

I'm going to document this up over the next day or so and submit a PR to add it to the dev docs.

[ndukler]

I think the documentation is in good shape in the process is fairly well established at this point so lets close this down @jeromekelleher ?

[andrewkern]

i can close this down. thanks @ndukler and @carjed