From 552147211ab1fe9af52520d990f2799f2be71ab0 Mon Sep 17 00:00:00 2001 From: Timothy Nunn Date: Wed, 6 Sep 2023 16:14:16 +0100 Subject: [PATCH 1/3] Update contributing guide to reflect new OS workflow --- CONTRIBUTING.md | 92 ++++++++++++++++++++----------------------------- 1 file changed, 38 insertions(+), 54 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index b6e4bee0d0..67ff2a13dc 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -1,54 +1,38 @@ -# Contributing -To contribute to Process, please follow this procedure. - -## Quick Summary -- Push code to branch -- Unit and integration tests must pass -- Regression tests are allowed to fail -- Reviewer checks source changes and regression differences -- If they pass review, changes are merged to develop - -## Contributing Guide -1. Raise an issue and describe the problem to be solved or the change to be made. -1. On that issue, create a branch and merge request. -1. Pull the branch and implement the solution, adding tests if appropriate. -1. Build the code, run it and check your solution works. -1. Run the tests using `pytest`; this runs the unit, integration and regression tests. - - Unit and integration tests must pass. Any that fail require source or test modification. - - Regression tests may fail; their purpose is to make you aware of significant changes to results as a result of your source changes. A 5% tolerance is applied by default to the regression tests: if any values differ by >5% from the reference, the regression test will fail for that scenario. This new value may be the desired result of the changes, however. Optionally, a 0% tolerance regression test can be run using `pytest --reg-tolerance=0`. - - It is incumbent on the author to check the test results created by their code changes, and modify source or tests if required. Are the regression changes expected and acceptable? -1. Once everything passes (apart from the regression tests, which are allowed to fail), commit and push the changes. This triggers the CI system, which will run the test suite again, including 5% and 0% tolerance regression jobs. The purpose of this is so that the reviewer of the merge request can see from the pipeline's regression job traces what changes >5% and >0% (if any) your code changes create. -1. If the changes are notable and it would benefit other users to be aware, [create a changelog entry](documentation/proc-pages/development/versioning.md). -1. The reviewer reviews the source changes and the results of the regression jobs to see how they change the regression results. Once the changes are approved by the reviewer, the branch can then be merged. - -### Contributing Workflow -```mermaid -flowchart TB -createbranch(Branch created from develop\nwith up-to-date regression\nreferences) -push(Push changes to branch) -ci(CI runs) -ci0("0% tolerance regression test\n(allowed to fail)") -ci5("5% tolerance regression test\n(allowed to fail)") -ciunitint(Unit and integration\ntests) -review("Review regression failures\n(differences)") -furtherchanges(Further changes required) -merge(Merge to develop) -overwrite(Regression references\noverwritten on develop\nby CI) -createbranch --> push -push --> ci -ci --> ci5 -ci --> ci0 -ci5 --> |Pass/Fail|review -ci0 --> |Pass/Fail|review -review --> |Pass review|merge -review --> |Fail review|furtherchanges -furtherchanges --> push -ci --> ciunitint -ciunitint --> |Pass|review -ciunitint --> |Fail|furtherchanges -merge --> overwrite -overwrite --> |Repeat:\nnext issue|createbranch -``` - -### Explanation -For an explanation of the reasoning behind this approach, please read the [testing documentation](http://process.gitpages.ccfe.ac.uk/process/development/testing). +# Contributing to `PROCESS` +There are many valuable contributions that can be made to PROCESS: +* Reporting bugs. +* Requesting/ recommending features. +* Implementing features or bugs raised as issues. +* Updating and improving documentation. + +When contributing to this repository, please first discuss the change you wish to make via issue, email, or any other method with the owners of this repository before making a change. + +Please remember that all contributions and communication regarding PROCESS are subject to our [Code of Concut](https://github.com/ukaea/PROCESS/blob/main/CODE_OF_CONDUCT.md). + +## Creating an issue +Issues can be used to report bugs or request features and improvements. We ask you help us manage our issues by: +* Verifying your issue is not a duplicate of another issue; in this case, we welcome your contribution as a reply to the issue. +* Ensure you completely describe the bug/feature/problem and complete the given templates with appropriate detail. + +## Submitting a pull request +Please discuss any feature ideas you have with the developers before submitting them, as you may not be aware of parallel development work taking place, or implementation decisions / subtleties which are relevant. The ideal workflow for submitting a pull request is as follows: + +* Discuss the feature with a core PROCESS developer. +* Submit an issue (if one does not exist for this feature/ bug) that documents the proposed change. +* Fork our repository. +* Create a branch of `main` with an appropriate name (e.g. `feature-abc`). +* Make the relevant changes for the repository (ensuring the changes do not creep away from the scope of the issue). +* Discuss any problems or development choices in the issue and keep everyone updated on the progress of your development. +* If the changes are notable and it would benefit other users to be aware, [create a changelog entry](https://ukaea.github.io/PROCESS/development/versioning/). +* Finally, submit a pull request onto the `main` branch: + * Link the relevant issue to the pull request. + * Assign the pull request to a maintainer of the code that will have the correct expertise to review the change. + +When making code contributions, we strongly recommend using pre-commit to verify your changes conform to our style requirements, otherwise the pull request will fail the 'quality' section of our GitHub actions. We document how this project uses pre-commit [here](https://ukaea.github.io/PROCESS/development/pre-commit/). + +Please remember that all contributions are made under the [MIT license](https://github.com/ukaea/PROCESS/blob/main/LICENSE.txt). + +### Testing +PROCESS has unit, integration, and regression tests. Any new functionality must be appropriately tested. Sometimes, changes may require other tests to be changed. These changes should be justified in the pull request description. Tests can be run locally by follow [our testing documentation](https://ukaea.github.io/PROCESS/development/testing/). All pull requests will also be run against our GitHub actions which will run all of the tests and report back to the reviewer any failures. **The unit and integration tests must pass on the CI for the changes to be accepted**. + +Regression tests, due to the nature of PROCESS, can change as model changes affect the optima which PROCESS converges to. A reviewer will review these changes to ensure they are minor and justified. We recommend justifying how a regression test is changing in the pull request discussion, a reviewer will likely request this anyway. For convenience, the CI system runs a 0% tolerance job that will highlight all differences between the current version of PROCESS on the `main` branch and your modified version of PROCESS; the 5% job excludes all differences under 5% differences between the two versions. \ No newline at end of file From da15f2e26cbb55b901467455ed7e72024c198816 Mon Sep 17 00:00:00 2001 From: Timothy Nunn Date: Wed, 6 Sep 2023 16:14:41 +0100 Subject: [PATCH 2/3] Remove/reword testing documentation to reflect changes to regression asset overwriting --- .../proc-pages/development/testing.md | 38 +++---------------- 1 file changed, 5 insertions(+), 33 deletions(-) diff --git a/documentation/proc-pages/development/testing.md b/documentation/proc-pages/development/testing.md index d0ed615b6e..e6674a6b5d 100644 --- a/documentation/proc-pages/development/testing.md +++ b/documentation/proc-pages/development/testing.md @@ -28,7 +28,7 @@ Regression tests detect changes in the entire program's behaviour by checking th input, it produces exactly the same output as before those changes. It detects changes in the program's output. Therefore if your code changes the program's output, it will fail the regression test. In this case that output difference will need to be reviewed and if accepted, the expected (or -reference) result will need to be updated in the test suite. +reference) will be updated. ## pytest @@ -159,15 +159,6 @@ using a Docker/Singularity container. It is suggested that PROCESS is run, built, and tested via a container when not using Ubuntu 20. -On older OS's that are detected to have this issue, test over-writing is disabled and pytest will -exit with an error message: - -``` -Test overwriting is NOT allowed on outdated systems. -``` - -In this case, the `pytest --overwrite` command must be run within an Ubuntu 20 container/ subsystem. - ## Reasoning behind the CONTRIBUTING.md method The method in the CONTRIBUTING.md is standard apart from how regression tests are handled; this is @@ -181,11 +172,6 @@ important when assessing the magnitude of intended changes to the output in cert well as detecting large unintended changes or failures to solve. The impact of the code changes on the regression results needs to be visible to the author and reviewer of the merge request. -In order to see the effect a branch's code changes have on the regression results, the regression -references must be kept up-to-date on the develop branch. This means that the regression references -need to be overwritten on the develop branch only; if the references were overwritten on each -branch, there would be merge conflicts in the references when merging the branches into develop. - Typically the solver will arrive at a very slightly different solution when any change is made, and so there are typically large numbers of very small changes with a few more significant changes amongst them. It is therefore useful for the reviewer to be able to filter out those more significant @@ -206,26 +192,12 @@ When those local changes are committed and pushed, the CI system for the branch and 0% tolerance regression jobs, which are allowed to fail. This shows the author and reviewer what the changes to the regression results are as a result of the code changes on that branch. -### Running the CI on the develop branch - -When merging into the develop branch, the CI system builds and tests the code as before, but -doesn't run any regression tests; it only overwrites the references with the results. The keeps the -regression references up-to-date on the develop branch, so that any branch created from develop will -pass a 0% tolerance regression run before any code changes are made. The CI will commit the reference -changes to the develop branch after running the overwrite job. There's little point in performing -regression tests on develop, as the regression tests will have already been run on the branch and -accepted by the reviewer before the merge to develop. - ## Drawbacks to this approach -- A problem can arise if a branch is created immediately after another has been merged, before the - overwrite commit has been pushed to develop by the CI. This new branch will therefore not have - the updated regression references on it, and can therefore fail or provide misleading regression - test results. In this unlikely case, a manual regression regression test overwrite could be - performed (`pytest --overwrite`) and committed, or a new branch created once the regression - overwrite has been committed on develop. -- In time, it may be better to use a data repository for the regression references, removing the - need for the CI to commit to develop when updating the references. +!!! note + + +- In time, it may be better to use a data repository for the regression references. - It's possible that two regression-acceptable branches can be merged to make a regression-unacceptable develop branch, but as regression tests aren't run on develop, only overwritten, this wouldn't produce a failure. Perhaps checking for regression failures to solve or significant changes in certain key From 69a27fd4e1f19e83ad206870d6344b40af3d44ea Mon Sep 17 00:00:00 2001 From: Timothy Nunn Date: Fri, 8 Sep 2023 13:22:09 +0100 Subject: [PATCH 3/3] Clarify sections of the CONTRIBUTING guide --- CONTRIBUTING.md | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 67ff2a13dc..cb00434012 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -5,7 +5,7 @@ There are many valuable contributions that can be made to PROCESS: * Implementing features or bugs raised as issues. * Updating and improving documentation. -When contributing to this repository, please first discuss the change you wish to make via issue, email, or any other method with the owners of this repository before making a change. +When contributing to this repository, please first discuss the change you wish to make via [issue](https://github.com/ukaea/PROCESS/issues), [discussion](https://github.com/ukaea/PROCESS/discussions), [email](https://github.com/ukaea/PROCESS#contacts), or any other method with the owners of this repository before making a change. Please remember that all contributions and communication regarding PROCESS are subject to our [Code of Concut](https://github.com/ukaea/PROCESS/blob/main/CODE_OF_CONDUCT.md). @@ -17,10 +17,10 @@ Issues can be used to report bugs or request features and improvements. We ask y ## Submitting a pull request Please discuss any feature ideas you have with the developers before submitting them, as you may not be aware of parallel development work taking place, or implementation decisions / subtleties which are relevant. The ideal workflow for submitting a pull request is as follows: -* Discuss the feature with a core PROCESS developer. +* Discuss the feature with a core PROCESS developer (as mentioned above). * Submit an issue (if one does not exist for this feature/ bug) that documents the proposed change. * Fork our repository. -* Create a branch of `main` with an appropriate name (e.g. `feature-abc`). +* Create a branch off `main` with an appropriate name (e.g. `feature-abc`). * Make the relevant changes for the repository (ensuring the changes do not creep away from the scope of the issue). * Discuss any problems or development choices in the issue and keep everyone updated on the progress of your development. * If the changes are notable and it would benefit other users to be aware, [create a changelog entry](https://ukaea.github.io/PROCESS/development/versioning/). @@ -33,6 +33,6 @@ When making code contributions, we strongly recommend using pre-commit to verify Please remember that all contributions are made under the [MIT license](https://github.com/ukaea/PROCESS/blob/main/LICENSE.txt). ### Testing -PROCESS has unit, integration, and regression tests. Any new functionality must be appropriately tested. Sometimes, changes may require other tests to be changed. These changes should be justified in the pull request description. Tests can be run locally by follow [our testing documentation](https://ukaea.github.io/PROCESS/development/testing/). All pull requests will also be run against our GitHub actions which will run all of the tests and report back to the reviewer any failures. **The unit and integration tests must pass on the CI for the changes to be accepted**. +PROCESS has unit, integration, and regression tests. Any new functionality must be appropriately tested. Sometimes, changes may require other tests to be changed. These changes should be justified in the pull request description. Tests can be run locally by following [our testing documentation](https://ukaea.github.io/PROCESS/development/testing/). All pull requests will also be run against our GitHub actions which will run all of the tests and report back to the reviewer any failures. **The unit and integration tests must pass on the CI for the changes to be accepted**. Regression tests, due to the nature of PROCESS, can change as model changes affect the optima which PROCESS converges to. A reviewer will review these changes to ensure they are minor and justified. We recommend justifying how a regression test is changing in the pull request discussion, a reviewer will likely request this anyway. For convenience, the CI system runs a 0% tolerance job that will highlight all differences between the current version of PROCESS on the `main` branch and your modified version of PROCESS; the 5% job excludes all differences under 5% differences between the two versions. \ No newline at end of file