diff --git a/docs/source/index.rst b/docs/source/index.rst index 7d9631cb9..8345698d3 100644 --- a/docs/source/index.rst +++ b/docs/source/index.rst @@ -28,7 +28,7 @@ InnerEye-DeepLearning Documentation :maxdepth: 1 :caption: Further reading for contributors - md/pull_requests.md + md/software_design_overview.md md/testing.md md/contributing.md md/deploy_on_aml.md diff --git a/docs/source/md/contributing.md b/docs/source/md/contributing.md index a48beb630..773ebe9dd 100644 --- a/docs/source/md/contributing.md +++ b/docs/source/md/contributing.md @@ -1,20 +1,27 @@ # Contributing -This document describes guidelines for contributing to the InnerEye-DeepLearning repo. +This document contains detailed guidelines for contributing to the InnerEye-DeepLearning repo. + +For the overarching software development process, refer to the [process description](software_development_process.md). ## Submitting pull requests -- DO submit all changes via pull requests (PRs). They will be reviewed and potentially be merged by maintainers after a peer review that includes at least one of the team members. +- DO submit all changes via pull requests (PRs). They will be reviewed and potentially be merged by maintainers after + a peer review that includes at least one of the team members. +- DO NOT mix independent and unrelated changes in one PR. PRs should implement one change, and hence be small. If + in doubt, err on the side of making the PR too small, rather than too big. It reduces the chance for you as the + author to overlook issues. Small PRs are easier to test and easier to review. - DO give PRs short but descriptive names. - DO write a useful but brief description of what the PR is for. - DO refer to any relevant issues and use keywords that automatically close issues when the PR is merged. - DO ensure each commit successfully builds. The entire PR must pass all checks before it will be merged. -- DO address PR feedback in additional commits instead of amending. +- DO link the correct Github issue. +- DO address PR feedback in additional commits instead of amending existing commits. +- DO NOT add any changes that go beyond what is requested by the reviewers. - DO assume that Squash and Merge will be used to merge the commits unless specifically requested otherwise. -- DO NOT submit "work in progress" PRs. A PR should only be submitted when it is considered ready for review. -- DO NOT mix independent and unrelated changes in one PR. -To enable good auto-generated changelogs, we prefix all PR titles with a category string, like "BUG: Out of bounds error when using small images". +To enable good auto-generated changelogs, we prefix all PR titles with a category string, like +"BUG: Out of bounds error when using small images". Those category prefixes must be in upper case, followed by a colon (`:`). Valid categories are - `ENH` for enhancements, new capabilities @@ -36,11 +43,17 @@ your dev box: - DO write unit tests for each new function or class that you add. - DO extend unit tests for existing functions or classes if you change their core behaviour. - DO ensure that your tests are designed in a way that they can pass on the local box, even if they are relying on -specific cloud features. + specific cloud features. +- DO add tests for each bug you fix. When fixing a bug, the suggested workflow is + to first write a unit test that shows the invalid behaviour, and only then start to code up the fix. - DO run all unit tests on your dev box before submitting your changes. The test suite is designed to pass completely -also outside of cloud builds. + also outside of cloud builds. If you are not + a member of the core InnerEye team, note that you may not be able to run some of the unit tests that access the team's + AzureML workspace. A member of the InnerEye team will be happy to assist then. - DO NOT rely only on the test builds in the cloud. Cloud builds trigger AzureML runs on GPU -machines that have a higher CO2 footprint than your dev box. + machines that have a higher CO2 footprint than your dev box. + +More details, in particular on tests that require GPUs, can be found in the [testing documentation](testing.md). ## Creating issues diff --git a/docs/source/md/pull_requests.md b/docs/source/md/pull_requests.md deleted file mode 100644 index 1d6bb0c5a..000000000 --- a/docs/source/md/pull_requests.md +++ /dev/null @@ -1,19 +0,0 @@ -# Suggested Workflow for Pull Requests - -* Pull Requests (PRs) should implement one change, and hence be small. If in doubt, err on the side of making the PR -too small, rather than too big. It reduces the chance for you as the author to overlook issues. Small PRs are easier -to test and easier to review. -* Ensure that the code you add or change is covered by unit tests. When fixing a bug, the suggested workflow is -to first write a unit test that shows the invalid behaviour, and only then start to code up the fix. -* It is best to run all unit tests on your dev box, and check that they pass, before publishing the PR. If you are not -a member of the core InnerEye team, note that you may not be able to run some of the unit tests that access the team's -AzureML workspace. A member of the InnerEye team will be happy to assist then. -* Before publishing your PR, please run PyCharm's code cleanup tools. You can either do that after editing your file, -by pressing Ctrl+Alt+L, or selecting "Reformat code" in the context menu of the file(s) in the project explorer window. -Alternatively, you should tick all of "Reformat code", "Rearrange code", "Optimize imports", "Cleanup", "Scan with mypy" -in the PyCharm version control check-in dialog. -* Only publish your PR for review once you have a build that is passing. You can make use of the "Create as Draft" -feature of GitHub. -* Link the correct Github issue. -* Once you have obtained approval for your change, do not add any changes that go beyond what is requested by the -reviewers. Any additional pushes to the branch will invalidate the approvals you have obtained. diff --git a/docs/source/md/software_design_overview.md b/docs/source/md/software_design_overview.md new file mode 100644 index 000000000..22a86b00b --- /dev/null +++ b/docs/source/md/software_design_overview.md @@ -0,0 +1,108 @@ +# Software Development Process + +This document provides a high-level overview of the software development process that our team uses for their +repositories (most importantly [InnerEye-DeepLearning](https://github.com/microsoft/InnerEye-DeepLearning) and +[InnerEye-Gateway](https://github.com/microsoft/InnerEye-Gateway)). + +## Version Control + +Software code versioning is done via GitHub. The code with the highest level of quality control is in the "main" branch. +Ongoing development happens in separate branches. Once development in the branch is finished, a Pull Request (PR) +process is started to integrate the branch into "main". + +## Development Process + +The design and development of the software in this repository is roughly separated into an **initiation**, +**prototyping**, and a **finalization** phase. The initiation phase can be skipped for minor changes, for example an +update to documentation. + +### Initiation + +During the initiation phase, the following steps are carried out: + +- Collection of a set of requirements. +- Creating a suggested design for the change. +- Review of the design with member of the core team. + +The deliverables of this phase are a detailed design of the proposed change in a GitHub Issue or a separate document. + +### Prototyping + +The engineering owner of the proposed change will create a branch of the current codebase. This branch is separate from +the released (main) branch to not affect any current functionality. In this branch, the engineer will carry out the +changes as proposed in the design document. + +The engineer will also add additional software tests at different levels (unit tests, integration tests) as appropriate +for the design. These tests ensure that the proposed functionality will be maintained in the future. + +The deliverable of this phase is a branch in the version control system that contains all proposed changes and a set of +software tests. + +### Finalization + +At this point, the engineering owner of the proposed change has carried out all necessary changes in a branch of the +codebase. They will now initiate a Pull Request process that consists of the following steps: + +- The code will be reviewed by at least 2 engineers. Both need to provide their explicit approval before the proposed + change can be integrated in the "main" branch. +- All unit and integration tests will start. +- All automatic code checks will start. These checks will verify the following: + + - Consistency with static typing rules. + - Ensure that no passwords or other types of credentials are revealed. + - Ensure that none of the used third-party libraries contains high severity software vulnerabilities that could affect + the proposed functionality. + +For code to be accepted into the "main" branch, the following criteria need to be satisfied: + +- All unit and integration tests pass. +- The code has been reviewed by at least 2 engineers who are members of the core development team. This review will + also assess non-quantifiable aspects of the proposed change, such as clarity and readability of the code and quality + of the documentation. +- Any comments that have been added throughout the review process need to be resolved. +- All automated checks pass. + +Once all the above criteria are satisfied, the branch will be merged into "main". + +## Software Configuration Management + +Third party libraries (sometimes called Software of Unknown Provenance, SOUP) are consumed via two +package management systems: + +- Conda +- PyPi + +Both of those package management systems maintain strict versioning: once a version of a package is published, it +cannot be modified in place. Rather, a new version needs to be released. + +Our training and deployment code uses Conda environment files that specify an explicit version of a dependent package to +use (for example, `lightning_bolts==0.4.0`). The Conda environment files are also version controlled in GitHub. Any +change to a version of a third party library will need to be carried out via the same change management process as a code +change, with Pull Request, review, and all tests passing. + +The list of third party software is maintained in GitHub in the Conda configuration file, that is `environment.yml` for +Linux environments and `environment_win.yml` for Windows environments. For example, [this is the latest version of the +environment file for the InnerEye-DeepLearning +repository](https://github.com/microsoft/InnerEye-DeepLearning/blob/main/environment.yml). + +## Defect Handling + +The handling of any bugs or defects discovered is done via GitHub Issues. + +When an Issue is created, the team members will get notified. Open Issues are kept in a list sorted by priority. For +example, [this is the list for all of the InnerEye-related Issues](https://github.com/orgs/microsoft/projects/320). +Priorities are re-assesed regularly, at least once a week. + +The Issue(s) with highest priority are assigned to an engineer. The engineer will then analyze the problem, and +possibly request more information to reproduce the problem. Requesting more information is also handled in the GitHub +Issue. Once the problem is clearly reproduced, the engineer will start to write a fix for the Issue as well as a test that +ensures that the fix does indeed solve the reported problem. + +Writing the fix and test will follow the process outlined above in the [Prototyping](#prototyping) and +[Finalization](#finalization) sections. In particular, the fix and test will undergo the Pull Request review process +before they are merged into the main branch. + +## Updates to the Development Process + +The development process described here is subject to change. Changes will undergo the review process described in this +very document, and will be published on GitHub upon completion.