Proposal: feature freeze for core code

[alexdewar]

I'll start off by saying that I know all of you care about the success of this project (as do we!) so I hope that none of this comes across as criticism of any individuals. However, I do have some concerns about the current direction of travel from an engineering perspective and I thought it might be helpful if I jot down my thoughts here. I'm more interested in provoking a wider discussion than I am in any particular outcome and I hope it'll be taken in that spirit 😄

I think that the work that I'm seeing in the repo is far too much skewed towards development of core code and that it needs to be balanced with other development, namely work on modelling code. I say "skewed", but at the moment actually the only code in there is core code!

Presumably this other work is made up of:

• Code checked into another repo
• Code not checked into any repo
• Paper and pencil-type work

First off, it would be good to know roughly what the ratios of these categories are. But insofar as there is some work in the first two, it would be great to get it merged into the main repo in some form or other. In a sense it almost doesn't matter what gets merged, so long as you each have something in the repo that you're working on. I think the psychological impact of feeling like there's something in there that you truly own is actually really important. The danger with development being skewed heavily towards the work of a couple of individuals is that other people end up feeling like the code isn't really "theirs" and that when they submit code they're somehow treading on someone else's turf. Ideally you want PRs etc. to provoke lively, democratic discussions, at least some of the time. Otherwise code review just becomes a rubber-stamping exercise.

I think there are solid technical reasons for preferring a flatter development model, too. Really you want the core code to co-evolve with the model code; while you will need to adapt your models to fit with the core's needs, the core also needs to adapt to the models' needs -- and I actually think the latter is more important. Don't assume that you'll be able to anticipate the models' needs ahead of time: the limitations of your current APIs will become apparent when you actually try to use them for something. If you carry on building more core code based on top of these APIs, it'll just become more and more difficult to extricate yourselves from these limitations at some future date. This also goes for adding unnecessary features. It isn't just a maintenance burden for the person who writes them, it also means that everyone else has to deal with that additional complexity -- for the duration of the project. Remember, worse is better. This should be the project's new motto 😉.

I'm concerned that the core is already becoming bloated and over-engineered, before work on modelling has even begun. If people were feeling apprehensive about submitting their code before, that's only going to get worse the more that they have to build it on top of this towering behemoth. There are two risks with this sort of project: one is that you have an over-developed core that hardly anyone on the team actually understands and the other is that you have lots of disconnected model code with repetition of routines that really should be factored out into common code. And if I had to choose, honestly I'd prefer the second of these. I'm not kidding: at least if you have to factor out common code, you can have a discussion about what actual needs your actual models have and not what hypothetical needs some hypothetical models might have, which is a much larger problem space!

So, as I've written in the title for this discussion, I'm proposing a feature freeze on core code -- either now or in the very near feature -- until other people's ongoing modelling work has been upstreamed in some form, as I think that's where your priority should be for now. (All right, you might want to make an exception for bug fixes and documentation updates, but you get the idea.) You could make a models folder and then people could keep whatever they're working on in subfolders in there (after submitting it via a PR, of course). Or something. The important thing, as far as I'm concerned, is just that everyone gets to be involved.

I'm conscious that all this work on and discussion around core code has meant that @dalonsoa and I have ended up focusing our attention on a smaller number of you than I'd like. This has been true in meetings as well as in terms of PRs etc. I feel a bit guilty about this and I want to hear from more of you: what you're working on and how we can help you integrate it with the code base. Perhaps the next meeting we have could be more of a roundtable and we could go round people one by one to talk about this?

I hope I haven't sounded too negative here. It goes without saying that there are things to like about the code base too -- there are lots of tests and documentation, for example -- and I'm sure Virtual Rainforest will end up being great. But I thought all this might be worth a discussion.

[davidorme]

Thanks, @alexdewar.

It's important that as a group we all feel able to challenge big picture stuff - and it is hugely important that conversations like this happen in a collaborative and positive manner. So, no, I don't think you're being too negative and equally I don't think any of us would take this personally!

Thoughts on the core 'bloat' 😄

• I agree with you - we should feature freeze the core development ... but I am going to qualify that as 'in the near future' and not 'now'.

• Essentially, the core development is about the framework in which the models operate, and the elements we now have in develop or in PRs are very close to feature complete for current development. We can then provide a common shared frameworks:

  • for the spatial structure (grid)
  • for passing configurations to models (config)
  • for getting data into models (data)
  • for logging (logger) and
  • the underlying basic structure of Model objects (model).

  That's basically it for core. From what we have in develop (and existing PRs), we can share a common API for those main activities, and then build the models.

• I definitely hear your concern about core being 'bloated and over-engineered'. However, I think all of the core stuff that is currently in play is functionality that we want the package to have, and not just in the long term. The v0.1 project is a reasonably short term goal, where we will want to demonstrate vr running from the command line, loading configs and data (probably more than one file format 😈 ) and passing those off to models.

TL/DR. I think there is a line in the core dev that we are nearly over and should cross but it is hugely useful to remind us that we should put a formal marker down and move on.

Thoughts on imbalance

• Development on core is fast - there's no theory to resolve into structure - so it has been high volume.
• However, there are now published branches with initial development of science modules for abiotic and animals.
• Given those branches, we should have a discussion about...

How we are using PRs

We need to think about how we are using Pull Requests, particularly at the start where big conceptual chunks are being developed for the first time. I think the data system PR (#122) is a shining example of the following process:

1. Here is a new system that the codebase needs to do
2. Here is an implementation on my new branch.
3. But no, that doesn't work: this code division is illogical and the flow isn't right.
4. Here's a revision
5. Goto 3 until..
6. Now we have something that actually works as intended and provides the functionality works.
7. But it is large.

So what is better practice? I can think of two options:

• Obviously (duh!) make smaller PRs for partial functionality and build things incrementally.

  The mental block I have here for actual PRs is that writing a mergeable PR for a partial solution (tests, etc) can feel a lot like makework. I certainly tend to think

  This isn't complete and doesn't 'work' - it just does a thing that doesn't fail tests at some point on the path towards what I actually want this code to do.

• Make more use of the draft PR mechanism. As I understand it, the code in a draft PR doesn't have to pass tests - it can be fragmentary and incomplete - so we can use the mechanism to get reviews on a sketch.

Does that sound like a more sensible approach?

TL/DR: The dividing line between "worse is better" and "yes but that there 👉 is shit" is a tricky one to learn! Doing a pull request at Step 3 often does not feel worthwhile.

[alexdewar]

Hi David,

Thanks for your response. I'll give a longer reply soon, but I want to give other people a chance to pipe up first.

In short though, I think it is possible to have more minimal PRs, without just merging something half-baked. The trick partly lies in reworking code you've already written into a reviewable form, as discussed on #122, but I think it also helps to have a more limited scope in mind for what you're working on before you even get started.

I think all this could make for a valuable discussion though.

[jacobcook1995]

I'm pretty happy with a feature freeze on the core code. The timing loop in main.py was basically the last thing I was intending to add to the code for core for the moment anyway. (I wanted to add some more documentation, but I'm assuming docs and bug fixes are not covered by a feature freeze?)

I don't have all that much together in terms of the model yet, more just a sketch of which pools the soil model should probably include. Transfer between these pools will happen through a system of differential equations, but I'm far from sure which equations to use. This probably will remain the case throughout the development of the model, as there's not much agreement in the literature in terms of how transfer processes should be represented, so we might want to try out different sets to see what works best for our purposes.

With that in mind I was thinking of adding some basic equations in to describe some soil processes, and then trying to develop a method to switch between different sets of process representations in a sensible manner. If that fits with the idea of a feature freeze on core code?

[davidorme]

At the risk of derailing this thread - if there are a small number (2 or 3?) of fundamentally different approaches, then having more than one soil model might be easier than the architecture for switching between them within one model?

[dalonsoa]

Let's have a separate conversation for this, but I think this calls for using Dependency Injection, implemented in the factory method.

[alexdewar]

@jacobcook1995 Yes, that sounds like the kind of thing I had in mind: I'd love to see some scientific code in the repo, even if it's only a rough sketch at this stage. And yes, I think documentation updates are fine (as are small bug fixes to core code), because other people don't have to build their work on top of the documentation in the same way they do with the code.

The details are obvs up to you guys, but I think that decisions like this should be made democratically. And if you go ask the team and people say, "What's happening with the core code again? I lost track ages ago." then that's a sign that your current process isn't working.

And the process is always more important than the code. You can always fix up code, but if members of your team don't feel comfortable contributing to the repo, then that's much harder to fix and will lead to crummy code in the long run. That's why I think it would be helpful to try to get everyone doing something or other in the main repo as a matter of urgency. Every month that people aren't working on anything in the repo because "we're still waiting until Feature X is done" is a month of learning that they're missing out on. It really doesn't have to be anything perfect. No one out there is looking into your repo and judging you guys, I promise. People have to tinker with things and make mistakes in order to learn.

Good code is always written to do what the user needs, not what the developer wants. The "users" of the core code are the rest of the team on the project, who'll need it for their models, so it really does need to be driven by them to a large extent. A "build it and they will come" attitude always leads to crappy software. That's how we got Windows Vista.

That's why I'd focus on the social/personal development aspects of the project more at this stage: once everyone's got something rudimentary up and running it'll be more apparent what they need from the core code and then you'll all be able to have a more meaningful conversation about what additional features, if any, it needs.

[dalonsoa]

Hi all! Here I am, finally, putting my grain of sand.

As it is often the case, there are good points on both sides of the discussion. I'll try to summarise my view in bullet points - rather straight to the point, don't take any comment personal!

• There was a clear (and I guess also agreed) plan to get to VR v0.1, so unless there is a reason for not going through it, that is what should be completed first. If that's mostly core functionality, so be it. Let's take that out of the way before moving on.
• Only @davidorme and @jacobcook1995 are contributing to this plan, which risk the rest of the team from being not just disconnected from the development, which is obviously bad, but not familiar enough with why things are the way they are (in practical terms) to be able to contribute to the tool in the future. Please, do not monopolise this development and, instead, support other members of the team (@vgro @LivDaniel @jaideep777 @TaranRallings @robewers01 ) contributing to this - maybe breaking down big tasks into smaller ones, working with them in the implementation, using pair programming, etc. It will be time very well spent in the long run.
• PR must be small in size, there is no negotiation about this, if just for consideration to your fellow developers who need to understand what you are doing and provide meaningful feedback. My suggestion would be to open a minimal PR agains the develop branch - draft or otherwise, I don't mind - from, let's call it BIG_FEATURE branch, and then open small, incremental PR from SMALL_FEATURE branches against BIG_FEATURE. These PR are small, specific, easy to review and understand by everyone, and all together will build the massive PR against develop. By the time BIG_FEATURE is complete, the PR has already been reviewed bit by bit, so if all the tests pass, the PR can be merged without much fuss.
• We mention this in every meeting, but it is good to remind everyone about it: no matter how much time you spent thinking on how to implement something, chances are it will need to be revised in the future, so do not overthink things. There is an arbitrary 30 min rule that says that if you think more than 30 min on how to code something, then you should ask someone else for advise.

As a final note, from the project management perspective, the time we (the RSE Team) are supposed to be involved in the project is very much underused. So, please, get in touch if you need specific support on something, one to one sessions or whatever because we can certainly invest more time in you!

And that's all from my side. Happy coding!

[davidorme]

Well that seems like a useful working practice 😄 In particular, it means we don't have one model for 'mergeable'. There is:

• develop mergeable: No, really, fix everything and write tests and docs.
• feature mergeable: What do you think of this solution/text?

[alexdewar]

Good points @dalonsoa.

Just to be clear though, as I said I don't really mind whether you actually do this proposal or not! I think proceeding with the v0.1 plan is fine (though I'd be skeptical if you were planning to do a lot more core development on top of this without any model code in the repo).

My (slightly cheeky) goal in suggesting this was to try to get you all to stop, take stock and have a conversation about how everyone feels about the current development process, because I'm worried that at present much of the team is rather alienated from it. The fact that this thread only has responses from the same people I always interact with concerns me. We want to try to get the whole team working in this space.

Incidentally, I don't think it's enough just to get everyone to sign off on a plan of action. If you talk at people for long enough they'll agree to anything, mostly just because they want to leave and go get lunch. The whole team needs to be involved in a) formulating plans and b) implementing them -- not just ratifying them. I suggest you try to get everyone doing something they want in the repo, sooner rather than later. Go have a conversation about how you can make that happen.

[alexdewar]

And as @dalonsoa says, please do make use of us! We want to help you guys as much as we can.

[vgro]

Hi all,
thanks for bringing up some very good points that I think will bring us forward as a group, I'll try and summarize my thoughts as objectively as possible, I know everyone is working hard and doing their best here:

• I agree that we need to communicate more to ensure that everyone in the team knows what is currently going on in the project. I don't think everyone needs to know all the code inside out and but we should all understand the big picture, what the core/config does and why and make sure that it serves all modules. I think this is actually true for all modules.

• We are all at different stages of the individual modules which means that we are working on multiple things, not only one writing code. At least for me, this is part of the reason why I haven't added much to the repo. The code I added was for the module I lead which is my main focus at the moment, I want to get to a dummy stage by the end of Feb. Since the plant module has a complete version, the lead has chosen to focus on the core. The soil lead has written the config system, but no 'scientific' code (sorry, no offense).

• I'm now at a point where I know what processes I want to include and I could definitely need some help with the realization of these concepts and bringing structure in.

[TaranRallings]

Hi folks,
Very interesting discussion. To echo Vivienne's point, I think we've run into some bumps partially as a function of the science being in different places. On my end, having just wrapped up my PhD and found somewhere to live, I'm in a much better position to be properly engaged with development.

I put some time into developing a basic dummy module of how the animal model could work but, alas, I got carrier away and built too much! This is sitting in a feature branch. I was intending to bother you folks on the best way to untangle the PR situation but it appears the basic strategy is outlined above.

The speculation that I might be out of the loop on core functionality is broadly correct. I think this can be remedied by a couple of hand-holding walk-throughs in the new year. Once I'm conceptually caught up, it will be much easier for me to weave the core into my workflow and my actual work. I will endeavor to do a better job of bothering people for help when I don't understand something. The structural changes being suggested sound helpful for minimizing how often I get turned around by the number of different things happening at the same time.