search

mrc-ide / covid-sim

This is the COVID-19 CovidSim microsimulation model developed by the MRC Centre for Global Infectious Disease Analysis hosted at Imperial College, London.
GNU General Public License v3.0
1.23k stars 256 forks source link

Development status #341

Open zebmason opened 4 years ago

zebmason commented 4 years ago

There appears to be a code freeze on at the moment.

If so does this extend to the build system? I have three such changes waiting to go - #288, #331 and #324. The third is draft as it actually depends on the second to provide unit test support.

weshinsley commented 4 years ago

Thanks/sorry @zebmason - just a bit of a lull this week, as some of our team are on leave, and the rest of us have our hands a bit full at the moment... things will pick up again.

zebmason commented 4 years ago

Thanks, knew you would be busy and did think about holiday myself - my usual trip to Cartmel races at Whit was cancelled.

CloneDeath commented 4 years ago

Maybe @zebmason , @zdroid , and I can help with reviewing pull requests for merge? As us three (at a glance) seem to be the most active.

zebmason commented 4 years ago

@CloneDeath If you look at the commit history you'll notice that Matt, who works for GitHub, is doing a lot of the merging. This suggests to me that the Imperial team are rather new to git - it took me quite a while to get used to git myself and previously I had used (in order) Perforce, Subversion, CVS, Source Off Site, Mercurial and something that emulated the Vax/VMS file system.

This then raises the problem of the core team dealing with merge conflicts whilst they need to deliver new modelling features. Feynstein decided to fork the repository for an experimental version. That fork is currently 270 commits behind this one...

There is a problem of trying to work out whether you are wasting your time. e.g. The reason I started this issue was because ten days ago I finally delivered some build functionality that Matt had asked for. This still hasn't been merged in even though it is the basis for getting a unit test framework in and thus addressing a major criticism of this code as voiced in other issues and the press.

Frustrating.

DavidVernest commented 4 years ago

The only way at present to improve the prod codebase is to implement well-tested incremental changes that get merged in as time allows, given they are very busy with BAU. Anything we can do to speed things up would be appreciated.

insidedctm commented 4 years ago

Is there any news on this? I appreciate that the main committers are very busy but it would be helpful if some sort of timeline for review and merging could be established.

weshinsley commented 4 years ago

Hi all - apologies again - we're doing our best to catch up while spinning a lot of different plates. Appreciating this is frustrating for you all, while you're really trying to help us, and we are very grateful for that.

Among the IC team we're all fairly used to private-project use of git, but most of us less so at this sort of project level. The risks are quite high with the code in live continual use (and somewhat in the spotlight); Matt is keeping us safe (and fixing our mistakes) with this while we're learning, but where we are at the moment, (I think you all know this already) - priority will be on the science features that we need to pull together to answer urgent questions.

I've been chipping away at a few of the really tiny PRs that are either docs/comments, or code that have no real controversy about them, as you might have seen. I am not a code owner, so won't be merging anything much more dramatic than that for now, but I'll be keeping an eye out for improvements that can easily go in.

Some other ideas that might help...

You're very welcome to do a review/approve and see if you spot anything. Someone on the team will also do so, but more eyes is always good. Only approve if the tests passed, and there are no conflicts - the ground may be moving with some of these I know.

It might help to re-request review / ping if you've addressed some requested changes - I can see a couple where that might be good to do (and yes, some where you already have...) as it's tempting looking down the list to skip ones that are still flagged "Changes Requested".

I'll ask particularly about #288 and #331 when we next catch up as I know you've addressed the concerns on them @zebmason and it's been a while. If anyone's got something queuing that's got failing tests/conflicts, a rebase/fix conflict/retest so it's looking good might attract attention (I know, if we'd merged earlier, the ground wouldn't have shifted, etc.... guilty again...)

Ok... you get the idea - keep bearing with us - I know we keep asking you to. Thanks for all your support.

DavidVernest commented 4 years ago

Hi - I’ve rebased against latest master, and will revalidate #316 on Monday - this change is non-trivial but not a particularly complex change, and affects CovidSim.cpp - who is best to contact for a code-owner review?

DavidVernest commented 4 years ago

@weshinsley @matt-gretton-dann Can you give any indication of when the backlog of proposed pull requests can be merged? - I know you're very busy at the moment.

weshinsley commented 4 years ago

Nothing we can promise I'm afraid; the original expectation we set (prior to PR #279, which arguably should not have been merged without a bit more review!), was that we didn't think we would have capacity for handling any external contributions or PRs, and would only be able to keep up with the changes we need to make to satisfy the scientific questions being asked of us... We're doing a little better than that I guess, which is good... but at the moment, there are other things being dealt with...

zlatanvasovic commented 4 years ago

@weshinsley In that PR I removed a sentence which was not factually correct at the given moment. The project certainly accepted contributions, and it accepted a lot of them.

weshinsley commented 4 years ago

Indeed at that time we were, but even so, it was not our original expectation that we'd be able to manage that in a sustained way. (which is why there are ~40 PRs waiting...)

zebmason commented 4 years ago

@weshinsley One thing to bear in mind was that the scientific results were being questioned due to the code lacking structure, documentation and testing. Quite a few of the external PRs were addressing those concerns.

CloneDeath commented 4 years ago

@weshinsley I think we have a lot of people here who want to help, and I think the best way to do that would be to accept someone (from the community) who can help bridge the gap and pick up the slack.

Someone who could work with both the IC Team, as well as the community. This person would not only be responsible for reviewing and merging external pull requests (with small, tested, incremental improvements in mind), but also work with the IC Team and help prioritize their important work and getting their code into master.

With the number of helpful people I've seen in this community, I don't think the IC Team has to do all of this work alone, and on their own.