|
1 | | -# Developer guidelines |
| 1 | +# Developer Guidelines |
2 | 2 |
|
3 | | -This page is intended for people who wish to contribute to PlantSimEngine, and indicates the various parts to bear in mind when adding in new code. |
| 3 | +This page is for contributors working on PlantSimEngine itself. It focuses on |
| 4 | +the local development workflow, the checks worth running before opening a pull |
| 5 | +request, and a few implementation details that are easy to miss. |
4 | 6 |
|
5 | 7 | ## Working on PlantSimEngine |
6 | 8 |
|
7 | | -Instructions are no different than for any other package. Use git to clone the repository [https://github.com/VirtualPlantLab/PlantSimEngine.jl](https://github.com/VirtualPlantLab/PlantSimEngine.jl). |
| 9 | +Clone the repository from |
| 10 | +[GitHub](https://github.com/VirtualPlantLab/PlantSimEngine.jl) and develop |
| 11 | +against a checked-out local copy, typically through `Pkg.develop(path="...")`. |
8 | 12 |
|
9 | | -When testing your changes, your environement will need to use a command such as `Pkg.develop("PlantSimEngine")` to make use of your code. |
| 13 | +We mostly follow the Julia manual's |
| 14 | +[style guide](https://docs.julialang.org/en/v1/manual/style-guide/). Questions, |
| 15 | +bug reports, and design discussions should go through |
| 16 | +[GitHub issues](https://github.com/VirtualPlantLab/PlantSimEngine.jl/issues) or |
| 17 | +the related pull request. |
10 | 18 |
|
11 | | -We work with VSCode and are most comfortable with that IDE for Julia development. We mostly follow the manual's [Julia style guide](https://docs.julialang.org/en/v1/manual/style-guide/) |
| 19 | +The [Roadmap](@ref) summarizes longer-term work that is not yet complete. |
12 | 20 |
|
13 | | -Once you've made the necessary checks (see the [Checklist before submitting PRs](@ref) listed below), you’ll need to create your pull request and ask to be added to the contributors if you wish to submit new changes. |
| 21 | +## Local environments |
14 | 22 |
|
15 | | -This documentation has a [Roadmap](@ref). The list of known issues and related discussions can be found [here](https://github.com/VirtualPlantLab/PlantSimEngine.jl/issues). Some are outdated, some are discussions related to potential features, but others are genuine bugs or enhancement suggestions. |
| 23 | +PlantSimEngine currently has three main local environments: |
16 | 24 |
|
17 | | -Other details and questions can be posted on our issues page, or as part of your Pull Request. |
| 25 | +- `test/` for the package test suite and doctests run from `test/runtests.jl`; |
| 26 | +- `docs/` for the Documenter build; |
| 27 | +- `benchmark/` for benchmark scripts used to compare performance locally. |
18 | 28 |
|
19 | | -## Quick rundown |
| 29 | +## Running checks locally |
20 | 30 |
|
21 | | -### Testing environments |
| 31 | +### Main test suite |
22 | 32 |
|
23 | | -PlantSimEngine has several developer environements: |
| 33 | +Run the standard test suite from the repository root: |
24 | 34 |
|
25 | | -- `/PlantSimEngine/test`, to check for non-regressions |
26 | | -- `/PlantSimEngine/test/downstream`, whose folder contains a few benchmarks on PlantSimEngine, PlantBioPhysics and XPalm, run as a Github Action, to ensure changes don't cause performance regressions in packages depending on PlantSimEngine. You’ll need to have a version of those packages accessible if you wish to test them locally. Those are distinct from the Github Action that does some integration checks to ensure no unexpected breaking changes occurs. |
27 | | - `/PlantSimEngine/docs`, to build the documentation. The documentation runs code, and some of the functions' documentation for the API are also tested as `jldoctest` instances |
| 35 | +```julia |
| 36 | +julia --project=test test/runtests.jl |
| 37 | +``` |
28 | 38 |
|
29 | | -### Running the standard test suite |
| 39 | +Some tests exercise threaded execution, so it is worth running them with more |
| 40 | +than one Julia thread when validating parallel behavior. |
30 | 41 |
|
31 | | -Simply execute the `/PlantSimEngine/test/runtests.jl` file in the test environment. Note that you'll need to start Julia with multiple threads for the multi-threading tests to successfully run. |
| 42 | +### Documentation |
32 | 43 |
|
33 | | -You'll also need the companion packages PlantMeteo and MultiScaleTreeGraph, as well as other Julia packages such as DataFrames, CSV, Documenter, Test, Aqua and Tables. |
| 44 | +Build the documentation from the repository root with: |
34 | 45 |
|
35 | | -### Downstream tests |
| 46 | +```julia |
| 47 | +julia --project=docs docs/make.jl |
| 48 | +``` |
36 | 49 |
|
37 | | -With XPalm and PlantBioPhysics properly instantiated, execute the `/PlantSimEngine/test/downstream/test/test-all-benchmarks.jl`. You may need to add some packages for the script to run locally. |
| 50 | +The docs environment includes the extra packages needed for examples and API |
| 51 | +documentation, such as `Documenter`, `CairoMakie`, `PlantMeteo`, and |
| 52 | +`MultiScaleTreeGraph`. |
38 | 53 |
|
39 | | -### Building the documentation |
| 54 | +### Benchmarks |
40 | 55 |
|
41 | | -In the `/PlantSimEngine/docs` environment, run `/PlantSimEngine/docs/make.jl`. It requires a couple of packages that aren't compulsory elsewhere (Documenter, CairoMakie, PlantGeom). |
| 56 | +Benchmark scripts live in `benchmark/`. They are useful when a change may alter |
| 57 | +runtime characteristics, but they are not a substitute for the main test suite |
| 58 | +or downstream integration checks. |
42 | 59 |
|
43 | | -### Editing benchmarks |
| 60 | +## CI workflows |
44 | 61 |
|
45 | | -⁃ If you wish for a branch to be benchmarked after every commit, then you need to declare it in the Github Action for benchmarks's yml file : [https://github.com/VirtualPlantLab/PlantSimEngine.jl/blob/main/.github/workflows/benchmarks_and_downstream.yml](https://github.com/VirtualPlantLab/PlantSimEngine.jl/blob/main/.github/workflows/benchmarks_and_downstream.yml) and add your branch to the `on: push:` section. |
46 | | -⁃ You can view benchmarks here: <https://virtualplantlab.github.io/PlantSimEngine.jl/dev/bench/index.html>. They are still somewhat WIP and not yet battle-tested. |
47 | | -⁃ You may occasionally need to update or delete a benchmark, in which case you will need to manually delete it in the **gh-pages** branch, in `dev/bench/index.html` |
48 | | -⁃ The actual benchmark list is located in the `test/downstream` folder. |
| 62 | +The repository currently relies on these GitHub Actions workflows: |
49 | 63 |
|
50 | | -## Things to keep an eye out for |
| 64 | +- `CI.yml` for the main test matrix, docs build, and coverage; |
| 65 | +- `Integration.yml` for downstream checks against packages that depend on |
| 66 | + PlantSimEngine; |
| 67 | +- `Benchmarks.yml` for pull-request benchmark runs; |
| 68 | +- `register.yml` and `TagBot.yml` for release automation. |
51 | 69 |
|
52 | | -### Check downstream tests |
| 70 | +If a change affects public APIs or execution behavior, check both `CI` and |
| 71 | +`Integration` before merging. Benchmark results are useful for regressions, but |
| 72 | +should be interpreted alongside the test results. |
53 | 73 |
|
54 | | -⁃ If your changes affect the API, then they might affect a package depending on PlantSimEngine. Benchmarks can be a way to check, as some benchmarks run other packages. Otherwise, a specific GitHub action, [https://github.com/VirtualPlantLab/PlantSimEngine.jl/blob/main/.github/workflows/Integration.yml] runs other packages’ test suites. If this action fails, then it is likely some breaking change was introduced that hasn’t been accounted for in the downstream package. If you expected a breaking change and labelled your release as such, there will be no action failure |
55 | | -⁃ Note that those tests don’t build the doc (iirc), so they don’t cover that. |
56 | | -⁃ API changes can also affect downstream packages’ documentation and tests... |
| 74 | +## Documentation impact |
57 | 75 |
|
58 | | -### Which documentation pages may be affected by changes |
| 76 | +Changes in PlantSimEngine often require documentation updates beyond the page you |
| 77 | +were editing. |
59 | 78 |
|
60 | | -You may impact several specific documentation pages depending on what you changed. Features and API changes affect whatever they might affect, but there are some less obvious ramifications: |
| 79 | +- User-facing errors often require updates to the troubleshooting pages. |
| 80 | +- New examples should ideally become doctests or rendered examples. |
| 81 | +- API or behavior changes may require updates to the roadmap, migration notes, |
| 82 | + and example pages. |
| 83 | +- If a feature remains experimental, say so clearly in the docs instead of |
| 84 | + letting examples imply stable support. |
61 | 85 |
|
62 | | -⁃ Improving user errors may impact the **Troubleshooting** page. |
63 | | -⁃ Extra features might also expand the **Tips and workarounds** page, as well as the ‘implicit contracts’ page. |
64 | | -⁃ Some experimental features might be worth documenting in the dedicated **API** page, once it's added |
65 | | -⁃ The roadmap "**Planned features**" page needs updating |
66 | | -⁃ Potentially, other pages such as the **Credits** page, **Key Concepts**, etc. If the API makes use of new Julia features or syntax, the **Julia basics** page is probably also worth updating. |
67 | | -⁃ New examples are worth making doctests of. |
| 86 | +## Pull request checklist |
68 | 87 |
|
69 | | -### Previewing documentation |
| 88 | +- Make sure the change is covered by tests. |
| 89 | +- Run the main test suite locally. |
| 90 | +- Build the documentation locally if docstrings, examples, or APIs changed. |
| 91 | +- Review the affected docs pages and update them in the same pull request. |
| 92 | +- Check GitHub Actions after pushing. |
| 93 | +- If the change is breaking or deprecates an old path, document the migration |
| 94 | + path before merging. |
70 | 95 |
|
71 | | -You can preview generated documentation (assuming it was able to build) relating to your PR (example given with #128) by checking the related link: [https://virtualplantlab.github.io/PlantSimEngine.jl/previews/PR128/](https://virtualplantlab.github.io/PlantSimEngine.jl/previews/PR128/) |
| 96 | +## Implementation notes |
72 | 97 |
|
73 | | -## Checklist before submitting PRs |
| 98 | +### Generated models and `eval` |
74 | 99 |
|
75 | | -⁃ Ensure your code, uh, works |
76 | | -⁃ Ensure your major changes are covered by some tests, and new features are documented |
77 | | -⁃ Run the PlantSimEngine test suite locally and check errors |
78 | | -⁃ Check on Github which issues it affects, and update/comment those issues, or link them to your Pull Request |
79 | | -⁃ Check which doc page changes are needed (roadmap, … see further up), and update those |
80 | | -⁃ Build the PSE doc and update whatever doc tests were broken |
81 | | -⁃ Push your commit, and let the Github Actions run their course |
82 | | -⁃ Check the 'CI' GitHub action and fix if necessary |
83 | | -⁃ Check downstream and benchmark GitHub actions: |
84 | | - - If benchmarks tanked, then fix your code. If you need to add/update/delete benchmarks, do so. |
85 | | - - If you broke an integration/downstream test, you’ll need to investigate it |
86 | | - - If API changes were made, also check downstream packages’ documentation |
| 100 | +Some multiscale helpers generate model definitions dynamically so that status |
| 101 | +vectors can be converted into runtime-compatible mappings. This currently relies |
| 102 | +on `eval()`, which means some changes only become visible again after returning |
| 103 | +to top-level scope. |
87 | 104 |
|
88 | | -It’s probably now safe to request a merge. |
| 105 | +The relevant code lives in `src/mtg/mapping/model_generation_from_status_vectors.jl`. |
| 106 | +If you touch that area, be careful about world-age issues and about tests that |
| 107 | +intentionally split setup and execution into separate top-level calls. |
89 | 108 |
|
90 | | -### A few extra things worth doing |
| 109 | +### Coverage gaps to keep in mind |
91 | 110 |
|
92 | | -⁃ You may have some new known issues, some remaining TODOs, document those somewhere, whether in the PR comments or in their own issue, make sure some trace remains |
93 | | -⁃ Finally, update this page and this checklist: If a doc page is added, it may be part of the list of pages you need to keep an eye on. If proper memory allocation tracking and type stability checking is implemented, then that’ll need to be added to the list of things to check prior to a release, etc. |
94 | | - |
95 | | -### Other helpful things |
96 | | - |
97 | | -⁃ In the `/PlantSimEngine/test` folder, there are a few basic helper functions. One of them outputs vectors of ModelMapping, weather data, and output variables, which are used as a test bank/matrix for some tests, and provides wide coverage. If you wrote new models, new combinations of models, or added some new weather data, it helps to add them to the banks. |
98 | | -⁃ New downstream packages are worth adding to the integration and downstream package registry. |
99 | | -⁃ Unusual corner-cases are worth giving their own unit tests. Newly fixed bugs as well, even if the fix is fairly trivial. |
100 | | - |
101 | | -## Noteworthy aspects of the codebase |
102 | | - |
103 | | -### Automatic model generation |
104 | | - |
105 | | -A specific feature requires generating models on the fly, to enable passing vectors to `Status` objects in multi-scale simulations. There may be more features that wish to generate models. |
106 | | - |
107 | | -The solution makes use of a somewhat brittle feature, `eval()`, with some subtleties. You can read more about the related world age problem [here](https://arxiv.org/abs/2010.07516), or [here](https://discourse.julialang.org/t/world-age-problem-explanation/9714/15). |
108 | | - |
109 | | -The related file is `model_generation_from_status_vectors.jl`, which has some additional comments. |
110 | | - |
111 | | -What is important to bear in mind, is that if you call functions which generate models via `eval()`, you will need to return to top-level scope for those changes to become visible. You can see an example in `tests/helper_functions.jl` with the functions `test_filtered_output_begin` and `test_filtered_output`. The first function calls `modellist_to_mapping`, which creates some models on the fly to convert status vectors between a ModelList and its equivalent pseudo-multiscale mapping. The function is split in two so that it is possible to return to global scope and make the `eval()` changes publicly available. The second function then is able to run the simulations on the mapping with its generated models, and complete the test successfully. |
112 | | - |
113 | | -The errors returned by an `eval()`-related issue are very specific, and indicate that a generated model with an UUID suffix does not exist in the Main module, or something along those lines. |
114 | | - |
115 | | -There may be a better approach that avoids those pitfalls, but that's what we have for now. Be cautious when calling functions from that file, and make sure to look out for comments indicating a function was split into two. |
116 | | - |
117 | | -### Weather/timestep/status combinations |
118 | | - |
119 | | -Not all combinations of weather data structure/weather dataset size/status sizes combinations are tested in PlantSimEngine itself. Some are tested in PlantBioPhysics and XPalm. It'd be good to have those structures tested in PSE in the future, but for now it is highly recommended checking those packages' tests when changing the API. |
120 | | - |
121 | | -### Test banks |
122 | | - |
123 | | -They were briefly mentioned earlier in the page, but the test banks to increase the number of combinations tested for in terms of weather data, modellists/mappings and tracked outputs, could definitely be improved upon. |
124 | | - |
125 | | -Some additional work and tests regarding tracking memory allocations, type stability etc. would also be worth implementing/documenting. |
| 111 | +Not every combination of weather structure, status shape, mapping layout, and |
| 112 | +downstream usage is covered directly in PlantSimEngine. When changing the public |
| 113 | +API or runtime semantics, treat downstream integration results as part of the |
| 114 | +validation surface, not as optional extra signal. |
0 commit comments