Skip to content

2863 review contributing guide #2892

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
jonmaddock merged 3 commits into from
Sep 12, 2023
Merged

2863 review contributing guide #2892

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5521472
Update contributing guide to reflect new OS workflow
timothy-nunn Sep 6, 2023
da15f2e
Remove/reword testing documentation to reflect changes to regression …
timothy-nunn Sep 6, 2023
69a27fd
Clarify sections of the CONTRIBUTING guide
timothy-nunn Sep 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
92 changes: 38 additions & 54 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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](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).

## 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 (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 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/).
* 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 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.
38 changes: 5 additions & 33 deletions documentation/proc-pages/development/testing.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand Down