APSIM Next Gen Infrastructure Redesign

[hol353]

After several days trying to implement changes to the APSIM infrastructure and encountering many strange issues and coding design patterns, I think its time to plan for a infrastructure redesign. I propose we incrementally move to this rather than down tools and do a rewrite. I'm posting this here as a way to capture my thoughts but equally importantly to get ideas and suggestions from others. Please feel free to comment.

Need better separation between GUI and models.

• I'm not sure GUI should talk directly to Models.
• There is a lot of code in the GUI to do non-GUI things. Have a look in ContextMenu (e.g. ExportDataStoreToEXCEL) or ExplorerPresenter (e.g. IsValidName) or ProfileGridPresnter (lots of code using reflection to discover properties etc.)
• GUI should talk to an API that has methods for each GUI function.
• This API should be unit tested.
• If the API was in another assembly, then we could remove the Models assembly reference from APSIMNG project - complete separation of user interface from the models.
• The API would need to have methods to get and set a property of a model, add a model, delete a model, enable a model, rename a model, get intellisense items, .... This would easily allow the detection of when simulation is dirty and speedup closing of a file in GUI #5536
• The API method to return intellisense items should call SimulationGeneration.ToSimulation() to get a simulation and use reflection on that.
• The API could be callable from other applications e.g. command line or something else.
• Name of assembly: APSIM.Services?

User Interface changes

• Can we remove all View classes (except the widget wrapper classes e.g. ButtonView) and replace with a Glade file and a presenter (e.g. AddModelPresenter and AddModelView.glade)?
• Allow multi-select in the tree view #2763
• Name of assembly: APSIM.UserInterface?

Create an APSIM kernel (assembly) that provides APSIM infrastructure core services but contains no models.

• This API should be unit tested.
• Would contain what is in the 'Core' folder of Models assembly.
• Separate assembly would allow us to use 'internal' on classes, properties, methods to hide detail from other assemblies.
• Tighten up when OnCreated() gets called in models. It is called in many places currently.
• Rename SimulationDescription class to SimulationGenerator. Currently it generates simulations but also runs them. Rework the ToSimulation method to build a simulation by applying replacements in the correct order. Should just be a few lines of code. It should do model replacements before property replacements.
• Provide a c# script compile service that models can call to compile code. The service would collect all script into a single c# script and then compile together. When should compilation occur? Probably at startup, and whenever a script is changed. #4859
• Rework (rewrite?) the location (Find, Get) services. Have a single way of getting a property, property value, a model ... using a path or expression, using scope if necessary (without needing square brackets). Also allow method calls (#3830). Not sure we need Variable, VariableComposite ... classes. If so, then they should be hidden. At the vert least, they need a refactor.
• Change scope rules to handle more use cases e.g. manager models in a folder in a simulation. #4423, #2862
• Refactor the documentation service. To have all code for creating autodoc in one place. Should use <remarks> and not <summary> Also see #480
• Create an api for discovering fields, properties, methods of models along with their [Description], [Units], <summary>, <remarks>... This will remove a lot of reflection code from everywhere else.
• Create an api for converting a simple object (e.g. int, double, DateTime, arrays) to a string and visa versa. There is a lot of code to do this in various places.
• PostSimulationTools should be run async where possible. Need some way to define async vs sync in GUI and .apsimx file. Also need to ability to cancel running of PostSimulationTools #5257
• Name of assembly: APSIM.Core?

Models assembly

• Should we have a separate command line executable? i.e. APSIM.CLI?
• All [Link]s should be to an interface. Provides loose coupling between models and allows much more flexibility.
• Need to rethink how ModelCollectionFromResource works. Don't read resource and add child models in OnCreated. Instead only do it in SimulationGeneration.ToSimulation(). This will speed up GUI load times. Rename IOptionallySerialiseChildren to IHaveResource which has a single property string ResourceName { get; set; } #5118
• Don't need Replacements model. Just use normal Folder and have convention of naming it 'Replacements'
• Models assembly shouldn't reference MigraDoc, PdfSharp, Newtonsoft, SharpZipLib assemblies
• Remove all Document methods from all models. Instead use <remarks>. Move all autodoc content from <summary> to <remarks>. Can then get rid of ICustomDocumentation.
• Determine what [Description] actually means and whether to use the <summary> of a property instead. #5456
• Have all PMF classes include [Link] to child IFunctions. #5290. This will help with intellisense from manager components. This might mean that we need to introduce a bit of inheritance and create a Cereal class and a Legume class.
• Name of assembly APSIM.Models?

[hol430]

I'm not sure GUI should talk directly to Models.

Agreed, maybe it's even worth separating the views from the presenters as well, as I suspect there may be a few gtk api calls scattered throughout apsimng outside of the view code

Create an api for converting a simple object (e.g. int, double, DateTime, arrays) to a string and visa versa. There is a lot of code to do this in various places.

This already exists, although it's probably not used everywhere. See ReflectionUtilities.StringToObject() and ReflectionUtilities.ObjectToString()

Need some way to define async vs sync in GUI and .apsimx file

What does this mean exactly? Are you talking about making many of the UI actions/callbacks asynchronous?

Should we have a separate command line executable?

Models is a good name for a class library where our models live...but I've always thought it was a little odd for the name of our main entrypoint program. ApsimNG.exe is much more intuitive, and many users make the mistake of trying to run their .apsimx file from the command line by invoking ApsimNG.exe. I also think there's something to be said for keeping all of the models by themselves in a class library, isolated from everything else. That being said, I wouldn't want to have to invoke APSIM.CLI.exe every time I want to run simulations

Remove all Document methods from all models. Instead use . Move all autodoc content from

to . Can then get rid of ICustomDocumentation.

Don't some models require the extra functionality provided by a Document() method? An alternative option here might be to remove the ICustomDocumentation interface, and have a virtual Document() method in the Model base class, which provides the default implementation of reading the remarks/summary tag, but can be overriden when more specialised behaviour is required.

[hol353]

Need some way to define async vs sync in GUI and .apsimx file

What does this mean exactly? Are you talking about making many of the UI actions/callbacks asynchronous?

I'd like to find a way to run the post simulation tools in parallel where possible. To do this we need some way to visualise, in the GUI, tools that can be run at the same time as other tools i.e. perhaps a directed graph might work.

[jbrider]

That all looks good - I agree with the name suggestions. It's a long list, so before you start an area is it worth putting up more detail around the proposed changes for the next lot of infrastructure changes for any more discussion. Once you start getting into more detail people will probably be more likely to contribute to the discussion.

With respect to the CLI option - wouldn't the CLI and the GUI be calling the same APSIM.Core code to run the simulations? So you wouldn't need to call the CLI from the GUI.

With links to interfaces, I don't mind that as a general guide, but if the first thing you do is cast the link to the class you need, then it is still going to be closely coupled so you haven't really gained very much except an extra line of code for a cast. Might need some more specific examples of the problem this will fix.

[lie112]

I agree with all stated. Implementing coding practices can be annoying across all developers, but I see the benefits. It would be nice to be able to separate Models from UI as this would mean each environment could be taking advantage of the latest coding and framework versions and Models not held back by GTK requirements. Models could effectively be .Net Standard and used by any .Net environment. I can see niggly issues arising with some of the suggestions due to the previous standards and quick implementation of functionality that remains for ages with some areas easier to implement than others. Its's a question of whether the benefits of the clean standards outweigh the cost of implementing. There is often an extra overhead in implementing communication through interfaces or having separation of presenters to run UI widgets. Some full visualisation of parts of APSIM would be useful, such as all Presenters and Views to know what is available and how everything fits together. It has taken me a long time to understand the explorer link in Presenters and I am sure I don't conform properly to all memory release and the undo commands functionality. Removing views does remove one level of separation as the presenters then need to be able to talk to widgets etc and need UI references such as GTK. The presenters are then tied to the display environment and hard to move from GTK in future. We need better communication to developers. A separate discussion stream on coding principles for those who have not had a long history of APSIM development or close relationship with current coders. For example, I am not fully aware of what is required from the

vs tags in APSIM, due to developing my own external documentation well before the in-model documentation matured to where it is now. I do like the way Next Gen has been built and the use of most standard approaches available in .net/SQLite etc if available before making custom implementations. For example if

is not used for documentation, this could replace the Description attribute, but in doing so we need to be able to determine what public properties are not included in UI (e.g. properties) as this can currently be achieved by not including the description attribute whereas leaving the

tag off is not an option and leads to compilation errors. I also like the idea of getting all APSIM infrastructure core services outside the models code. This also provides a one-stop-place for developers to go and check the available methods. The cleaning up of GetChildren and GetAncestors etc has been good. A command line control of APISM would be great for detailed model workflows especially given CLEM uses the tree structure so heavily in determining model functionality and we have to do a lot of JSON manipulation to build custom models in code. This would also allow a R style workflow of building APSIM simulations, but then I am trying to think of a current need. I have still not implemented any unit testing and don't fully understand how to use the Jenkins validation. CLEM also needs Jenkins validation on the UI, not just Model as changes to the UI often influence CLEM users when things stop working for other change reasons.

[hol353]

Thanks @lie112 for the considered response. I agree with what you've said.

Removing views does remove one level of separation as the presenters then need to be able to talk to widgets etc and need UI references such as GTK. The presenters are then tied to the display environment and hard to move from GTK in future.

The presenters wouldn't be tied to GTK. We currently have some views that simply wrap GTK widgets e.g. ButtonView wraps the GTK button class. If our presenters talk to instances of these wrapper views then we aren't coupling the presenters to GTK. AddModelPresenter is an example of a presenter that does this. It doesn't have a using statement to GTK. Instead it uses a Glade file and has code like:

addModelButton = (view as ViewBase).GetControl<ButtonView>("button");

This gets an instance of a ButtonView that wraps the GTK 'Add Model' button. The class then traps the button click like this:

addModelButton.Clicked += OnAddButtonClicked;

[hol430]

I actually don't like these views which wrap the gtk widgets. If we wrap the widgets too closely then they will resemble the gtk api too much making it hard to move away from gtk if we decide to do so in the future, but on the other hand if we don't wrap the gtk widgets closely then we lose many nice customisations possible with gtk. The other problem that I frequently run into is that it's hard to reuse these views inside other views. E.g. it's not clear which of our views require the Initialise() method to be called and which do not. Many of our views have several constructor overloads and some views only work if a particular constructor is used, and they may or may not work if the Initialise() method is or is not called.

The obvious solution to this mess might be to just make all views rely on glade files so that all views require Initialise() to be called. The problem is that some views - such as PropertyView, which needs code to construct widgets dynamically - are fundamentally incompatible with this approach. I really question whether these glade-based views are necessary and what advantage they bring us other than forcing all logic to occur in the presenter. I would suggest at the very least that we come up with a different name for views which wrap a glade file to alleviate some of the confusion, and change the inheritance slightly so that views which don't have a backing glade file don't have an Initialise() method.

[hol430]

Following discussion with @hol353 about an autodocs refactor:

Plant is to add a

IEnumerable<ITag> Document();

method to class Model. The base implementation will just parse the markdown in the class' summary tag. The ICustomDocumentation interface will cease to exist (existing implementations will override the Document() method), and the tags will live in a new class library (APSIM.Services?) which has minimal dependencies. The export to pdf functionality will be separated from the auto-documentation and will live in its own assembly to remove the dependency on PDFSharp/Migradoc from the Models assembly. Ultimately Models will become a class library and will contain only the Models and should have minimal dependencies.

[hol430]

@hol353 what do you think of having many of the model interfaces inherit from IModel? E.g. IFunction, IPlant, IOrgan, etc. As far as I know, all implementations of these interfaces are already models. I'm not sure why we would want to have, say, an IFunction that's not an IModel. This would also remove a large number of casts/type checking code from the autodocs; e.g. now that Document() is a function on class Model, any model which wants to document all of its IFunction children needs to cast those IFunctions to IModels.

[hol353]

I'm ok with that if it removes the casts.