Skip to content

Commit 3f0dd8d

Browse files
authored
Simplify pull request checklist (#7096)
* Use a simpler pull request checklist, part of the GitHub template. * Updates to Code Formatting page. * Improved formatting of benchmarks docs. * Updates to Contributing a Whats New page. * Correct PR number curl command. * What's New entry. * More clarity about pre-commit further reading. * Friendlier checklist. * Better triggering guidance. * Better line break.
1 parent bd4e3cb commit 3f0dd8d

10 files changed

Lines changed: 143 additions & 161 deletions

File tree

.github/pull_request_template.md

Lines changed: 22 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,14 +1,27 @@
1-
## 🚀 Pull Request
1+
## Description
22

3-
### Description
4-
<!-- Provide a clear description about your awesome pull request -->
5-
<!-- Tell us all about your new feature, improvement, or bug fix -->
3+
_Please describe your awesome pull request_
64

5+
## Checklist
76

8-
---
9-
[Consult Iris pull request check list]( https://scitools-iris.readthedocs.io/en/latest/developers_guide/contributing_pull_request_checklist.html)
7+
> [!IMPORTANT]
8+
> **The Iris core developers are here to help!** If anything below is unclear, just post a comment asking for help 😊
109
11-
---
12-
Add any of the below labels to trigger actions on this PR:
10+
- [ ] Included a [**What's New**](https://scitools-iris.readthedocs.io/en/latest/developers_guide/documenting/whats_new_contributions.html) entry
11+
- [ ] Incorporated [**type hints**](https://scitools-iris.readthedocs.io/en/latest/developers_guide/contributing_code_formatting.html#type-hinting) in any changed code
12+
- [ ] Checked if [**tests**](https://scitools-iris.readthedocs.io/en/latest/developers_guide/contributing_tests.html) need updating
13+
- [ ] Checked if [**benchmarks**](../benchmarks/README.md#writing-benchmarks) need updating
14+
- [ ] Checked if **documentation** needs updating
15+
- [Docstrings](https://scitools-iris.readthedocs.io/en/latest/developers_guide/documenting/docstrings.html) (we love code examples!)
16+
- [User Manual](https://scitools-iris.readthedocs.io/en/latest/user_manual/) pages
17+
- [ ] Checked if [**dependencies**](https://scitools-iris.readthedocs.io/en/latest/developers_guide/contributing_ci_tests.html#gha-test-env) need updating
18+
- [ ] Confirmed that the GitHub **'checks'** on this PR are passing :white_check_mark:
19+
_([further reading](https://scitools-iris.readthedocs.io/en/latest/developers_guide/contributing_ci_tests.html))_
1320

14-
- https://github.com/SciTools/iris/labels/benchmark_this
21+
---
22+
> [!TIP]
23+
> Things you can trigger on this PR:
24+
>
25+
> - Add this label to trigger benchmarks: https://github.com/SciTools/iris/labels/benchmark_this
26+
> - Visit this URL - swapping `9999` for this PR's number - to re-trigger the [CLA check](https://github.com/SciTools/.github/wiki/Contributor-Licence-Agreement):
27+
> `https://cla-assistant.io/check/SciTools/iris?pullRequest=9999`

benchmarks/README.md

Lines changed: 32 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -13,13 +13,17 @@ shifts in performance being flagged in a new GitHub issue.
1313

1414
On GitHub: a Pull Request can be benchmarked by adding the
1515
https://github.com/SciTools/iris/labels/benchmark_this
16-
label to the PR (to run a second time: just remove and re-add the label).
16+
label to the PR.
1717
Note that a benchmark run could take an hour or more to complete.
1818
This runs a comparison between the PR branch's ``HEAD`` and its merge-base with
1919
the PR's base branch, thus showing performance differences introduced
2020
by the PR. (This run is managed by
2121
[the aforementioned GitHub Action](../.github/workflows/benchmark.yml)).
2222

23+
> [!TIP]
24+
> To run the benchmarks a second time: remove the
25+
> https://github.com/SciTools/iris/labels/benchmark_this label and add it again.
26+
2327
To run locally: the **benchmark runner** provides conveniences for
2428
common benchmark setup and run tasks, including replicating the benchmarking
2529
performed by GitHub Actions workflows. This can be accessed by:
@@ -48,42 +52,17 @@ are not already. This can be done in several ways:
4852

4953
### Environment variables
5054

51-
* `OVERRIDE_TEST_DATA_REPOSITORY` - required - some benchmarks use
52-
`iris-test-data` content, and your local `site.cfg` is not available for
53-
benchmark scripts. The benchmark runner defers to any value already set in
54-
the shell, but will otherwise download `iris-test-data` and set the variable
55-
accordingly.
56-
* `DATA_GEN_PYTHON` - required - path to a Python executable that can be
57-
used to generate benchmark test objects/files; see
58-
[Data generation](#data-generation). The benchmark runner sets this
59-
automatically, but will defer to any value already set in the shell. Note that
60-
[Mule](https://github.com/MetOffice/mule) will be automatically installed into
61-
this environment, and sometimes
62-
[iris-test-data](https://github.com/SciTools/iris-test-data) (see
63-
`OVERRIDE_TEST_DATA_REPOSITORY`).
64-
* `BENCHMARK_DATA` - optional - path to a directory for benchmark synthetic
65-
test data, which the benchmark scripts will create if it doesn't already
66-
exist. Defaults to `<root>/benchmarks/.data/` if not set. Note that some of
67-
the generated files, especially in the 'SPerf' suite, are many GB in size so
68-
plan accordingly.
69-
* `ON_DEMAND_BENCHMARKS` - optional - when set (to any value): benchmarks
70-
decorated with `@on_demand_benchmark` are included in the ASV run. Usually
71-
coupled with the ASV `--bench` argument to only run the benchmark(s) of
72-
interest. Is set during the benchmark runner `cperf` and `sperf` sub-commands.
73-
* `ASV_COMMIT_ENVS` - optional - instruct the
74-
[delegated environment management](#benchmark-environments) to create a
75-
dedicated environment for each commit being benchmarked when set (to any
76-
value). This means that benchmarking commits with different environment
77-
requirements will not be delayed by repeated environment setup - especially
78-
relevant given the [benchmark runner](bm_runner.py)'s use of
79-
[--interleave-rounds](https://asv.readthedocs.io/en/stable/commands.html?highlight=interleave-rounds#asv-run),
80-
or any time you know you will repeatedly benchmark the same commit. **NOTE:**
81-
SciTools environments tend to large so this option can consume a lot of disk
82-
space.
55+
| Name | Required | Description | Notes |
56+
|------|----------|-------------|-------|
57+
| `OVERRIDE_TEST_DATA_REPOSITORY` | **required** | Some benchmarks use `iris-test-data` content, and your local `site.cfg` is not available for benchmark scripts. The benchmark runner defers to any value already set in the shell, but will otherwise download `iris-test-data` and set the variable accordingly. | |
58+
| `DATA_GEN_PYTHON` | **required** | Path to a Python executable that can be used to generate benchmark test objects/files; see [Data generation](#data-generation). The benchmark runner sets this automatically, but will defer to any value already set in the shell. | [Mule](https://github.com/MetOffice/mule) will be automatically installed into this environment, and sometimes [iris-test-data](https://github.com/SciTools/iris-test-data) (see `OVERRIDE_TEST_DATA_REPOSITORY`). |
59+
| `BENCHMARK_DATA` | optional | Path to a directory for benchmark synthetic test data, which the benchmark scripts will create if it doesn't already exist. Defaults to `<root>/benchmarks/.data/` if not set. | Some of the generated files, especially in the 'SPerf' suite, are many GB in size so plan accordingly. |
60+
| `ON_DEMAND_BENCHMARKS` | optional | When set (to any value): benchmarks decorated with `@on_demand_benchmark` are included in the ASV run. Usually coupled with the ASV `--bench` argument to only run the benchmark(s) of interest. Is set during the benchmark runner `cperf` and `sperf` sub-commands. | |
61+
| `ASV_COMMIT_ENVS` | optional | Instruct the [delegated environment management](#benchmark-environments) to create a dedicated environment for each commit being benchmarked when set (to any value). This means that benchmarking commits with different environment requirements will not be delayed by repeated environment setup - especially relevant given the [benchmark runner](bm_runner.py)'s use of [--interleave-rounds](https://asv.readthedocs.io/en/stable/commands.html?highlight=interleave-rounds#asv-run), or any time you know you will repeatedly benchmark the same commit. | **SciTools environments tend to be large so this option can consume a lot of disk space.** |
8362

8463
## Writing benchmarks
8564

86-
[See the ASV docs](https://asv.readthedocs.io/) for full detail.
65+
[See the ASV docs](https://asv.readthedocs.io/en/stable/) for full detail.
8766

8867
### What benchmarks to write
8968

@@ -96,19 +75,21 @@ positive regressions.
9675
We therefore recommend writing benchmarks representing scripts or single
9776
operations that are likely to be run at the user level.
9877

99-
The drawback of this approach: a reported regression is less likely to reveal
100-
the root cause (e.g. if a commit caused a regression in coordinate-creation
101-
time, but the only benchmark covering this was for file-loading). Be prepared
102-
for manual investigations; and consider committing any useful benchmarks as
103-
[on-demand benchmarks](#on-demand-benchmarks) for future developers to use.
78+
> [!CAUTION]
79+
> The drawback of this approach: a reported regression is less likely to reveal
80+
> the root cause (e.g. if a commit caused a regression in coordinate-creation
81+
> time, but the only benchmark covering this was for file-loading). Be prepared
82+
> for manual investigations; and consider committing any useful benchmarks as
83+
> [on-demand benchmarks](#on-demand-benchmarks) for future developers to use.
10484
10585
### Data generation
10686

107-
**Important:** be sure not to use the benchmarking environment to generate any
108-
test objects/files, as this environment changes with each commit being
109-
benchmarked, creating inconsistent benchmark 'conditions'. The
110-
[generate_data](./benchmarks/generate_data/__init__.py) module offers a
111-
solution; read more detail there.
87+
> [!WARNING]
88+
> Be sure not to use the benchmarking environment to generate any
89+
> test objects/files, as this environment changes with each commit being
90+
> benchmarked, creating inconsistent benchmark 'conditions'. The
91+
> [generate_data](./benchmarks/generate_data/__init__.py) module offers a
92+
> solution; read more detail there.
11293
11394
### ASV re-run behaviour
11495

@@ -136,9 +117,10 @@ detail.
136117

137118
### Scaling / non-Scaling Performance Differences
138119

139-
**(We no longer advocate the below for benchmarks run during CI, given the
140-
limited available runtime and risk of false-positives. It remains useful for
141-
manual investigations).**
120+
> [!CAUTION]
121+
> We no longer advocate the below for benchmarks run during CI, given the
122+
> limited available runtime and risk of false-positives. It remains useful for
123+
> manual investigations.
142124
143125
When comparing performance between commits/file-type/whatever it can be helpful
144126
to know if the differences exist in scaling or non-scaling parts of the
@@ -160,16 +142,16 @@ suites for the UK Met Office NG-VAT project.
160142

161143
## Benchmark environments
162144

163-
We have disabled ASV's standard environment management, instead using an
145+
We have disabled ASV's standard environment management[^1], instead using an
164146
environment built using the same scripts that set up the package test
165147
environments.
166148
This is done using ASV's plugin architecture - see
167149
[`asv_delegated.py`](asv_delegated.py) and associated
168150
references in [`asv.conf.json`](asv.conf.json) (`environment_type` and
169151
`plugins`).
170152

171-
(ASV is written to control the environment(s) that benchmarks are run in -
153+
[^1]: ASV is written to control the environment(s) that benchmarks are run in -
172154
minimising external factors and also allowing it to compare between a matrix
173155
of dependencies (each in a separate environment). We have chosen to sacrifice
174156
these features in favour of testing each commit with its intended dependencies,
175-
controlled by the test environment setup script(s)).
157+
controlled by the test environment setup script(s).

benchmarks/custom_bms/README.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,7 @@
22

33
To be recognised by ASV, these benchmarks must be packaged and installed in
44
line with the
5-
[ASV guidelines](https://asv.readthedocs.io/projects/asv-runner/en/latest/development/benchmark_plugins.html).
5+
[ASV guidelines](https://asv.readthedocs.io/projects/asv-runner/en/stable/development/benchmark_plugins.html).
66
This is achieved using the custom build in [install.py](./install.py).
77

88
Installation is into the environment where the benchmarks are run (i.e. not

benchmarks/custom_bms/install.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@
55
"""Install the SciTools custom benchmarks for detection by ASV.
66
77
See the requirements for being detected as an ASV plugin:
8-
https://asv.readthedocs.io/projects/asv-runner/en/latest/development/benchmark_plugins.html
8+
https://asv.readthedocs.io/projects/asv-runner/en/stable/development/benchmark_plugins.html
99
"""
1010

1111
from pathlib import Path

benchmarks/custom_bms/tracemallocbench.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -192,5 +192,5 @@ def too_slow(num_samples) -> bool:
192192
return samples, 1
193193

194194

195-
# https://asv.readthedocs.io/projects/asv-runner/en/latest/development/benchmark_plugins.html
195+
# https://asv.readthedocs.io/projects/asv-runner/en/stable/development/benchmark_plugins.html
196196
export_as_benchmark = [TracemallocBenchmark]

docs/src/developers_guide/contributing_ci_tests.rst

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,20 @@ If any CI tasks fail, then the pull-request is unlikely to be merged to the
9292
Iris target branch by a core developer.
9393

9494

95+
Data Repositories
96+
-----------------
97+
98+
Two other SciTools repositories support the CI tasks:
99+
100+
* `iris-test-data`_ is a github project containing all the data to support the tests.
101+
* `iris-sample-data`_ is a github project containing all the data to support the gallery and examples.
102+
103+
If new files are required by tests or code examples, they must be added to
104+
the appropriate supporting project via a suitable pull-request. This pull
105+
request should be referenced in the main Iris pull request and must be
106+
accepted and merged before the Iris one can be.
107+
108+
95109
.. _testing_cla:
96110

97111
`CLA Assistant`_

docs/src/developers_guide/contributing_code_formatting.rst

Lines changed: 16 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -7,14 +7,15 @@ Code Formatting
77

88
.. readingtime::
99

10-
To ensure a consistent code format throughout Iris, we recommend using
11-
tools to check the source directly.
10+
Also known as 'linting'. This is used to ensure a consistent code format
11+
throughout Iris, maximising the maintainability and quality. Code formatting
12+
is checked using the `pre-commit`_ tool, and the full list current formatting
13+
tools is defined in Iris' `pre-commit-config.yaml`_ file. Read more about
14+
linting on the `SciTools wiki page`_.
1215

13-
* `black`_ for an opinionated coding auto-formatter
14-
* `flake8`_ linting checks
15-
16-
The preferred way to run these tools automatically is to setup and configure
17-
`pre-commit`_.
16+
``pre-commit`` compliance is automatically checked on all Iris pull requests
17+
(more info: :ref:`pre_commit_ci`), but you can also run pre-commit locally as
18+
Git hooks - every time you make a commit. Read on to learn more about local running.
1819

1920
You can install ``pre-commit`` in your development environment using ``pip``::
2021

@@ -34,15 +35,14 @@ the root directory of Iris::
3435

3536
$ pre-commit install
3637

37-
Upon performing a ``git commit``, your code will now be automatically formatted
38-
to the ``black`` configuration defined in our ``pyproject.toml`` file, and
39-
linted according to our ``.flake8`` configuration file. Note that,
38+
Upon performing a ``git commit``, your code changes will be automatically
39+
checked against all Iris' ``pre-commit`` hooks. For some hooks this includes
40+
automated edits of your code e.g. formatting or sorting of imports; these new
41+
edits are not staged for you - i.e. you need to run ``git add`` again on that
42+
file. Note that,
4043
``pre-commit`` will automatically download and install the necessary packages
4144
for each ``.pre-commit-config.yaml`` git hook.
4245

43-
Additionally, you may wish to enable ``black`` for your preferred
44-
`editor/IDE <https://black.readthedocs.io/en/stable/integrations/editors.html#editor-integration>`_.
45-
4646
With the ``pre-commit`` configured, the output of performing a ``git commit``
4747
will look similar to::
4848

@@ -56,8 +56,7 @@ will look similar to::
5656
2 files changed, 10 insertions(+), 9 deletions(-)
5757

5858

59-
.. note:: You can also run `black`_ and `flake8`_ manually. Please see the
60-
their officially documentation for more information.
59+
.. _type_hinting:
6160

6261
Type Hinting
6362
------------
@@ -69,3 +68,5 @@ add/improve them.
6968

7069

7170
.. _pre-commit: https://pre-commit.com/
71+
.. _pre-commit-config.yaml: https://github.com/SciTools/iris/blob/main/.pre-commit-config.yaml
72+
.. _SciTools wiki page: https://github.com/SciTools/.github/wiki/Linting

docs/src/developers_guide/contributing_pull_request_checklist.rst

Lines changed: 4 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -7,57 +7,11 @@ Pull Request Checklist
77

88
.. readingtime::
99

10-
All pull request will be reviewed by a core developer who will manage the
11-
process of merging. It is the responsibility of the contributor submitting a
12-
pull request to do their best to deliver a pull request which meets the
13-
requirements of the project it is submitted to.
10+
.. attention::
1411

15-
This check list summarises criteria which will be checked before a pull request
16-
is merged. Before submitting a pull request please consider the following:
12+
The checklist has moved!
1713

14+
The latest checklist is now part of the `GitHub pull request template`_.
1815

19-
#. **Provide a helpful description** of the Pull Request. This should include:
2016

21-
* The aim of the change / the problem addressed / a link to the issue.
22-
* How the change has been delivered.
23-
24-
#. **Include a "What's New" entry**, if appropriate.
25-
See :ref:`whats_new_contributions`.
26-
27-
#. **Check all tests pass**. This includes existing tests and any new tests
28-
added for any new functionality. For more information see
29-
:ref:`developer_running_tests`.
30-
31-
#. **Check all modified and new source files conform to the required**
32-
:ref:`code_formatting`.
33-
34-
#. **Check all new dependencies added to the** `requirements`_ **yaml
35-
files.** If dependencies have been added then new nox testing lockfiles
36-
should be generated too, see :ref:`gha_test_env`.
37-
38-
#. **Check the source documentation been updated to explain all new or changed
39-
features**. Note, we now use numpydoc strings. Any touched code should
40-
be updated to use the docstrings formatting. See :ref:`docstrings`.
41-
42-
#. **Include code examples inside the docstrings where appropriate**. See
43-
:ref:`contributing.documentation.testing`.
44-
45-
#. **Check the documentation builds without warnings or errors**. See
46-
:ref:`contributing.documentation.building`
47-
48-
#. **Check for any new dependencies in the** `readthedocs.yml`_ **file**. This
49-
file is used to build the documentation that is served from
50-
https://scitools-iris.readthedocs.io/en/latest/
51-
52-
#. **Check for updates needed for supporting projects for test or example
53-
data**. For example:
54-
55-
* `iris-test-data`_ is a github project containing all the data to support
56-
the tests.
57-
* `iris-sample-data`_ is a github project containing all the data to support
58-
the gallery and examples.
59-
60-
If new files are required by tests or code examples, they must be added to
61-
the appropriate supporting project via a suitable pull-request. This pull
62-
request should be referenced in the main Iris pull request and must be
63-
accepted and merged before the Iris one can be.
17+
.. _GitHub pull request template: https://github.com/SciTools/iris/blob/main/.github/pull_request_template.md

0 commit comments

Comments
 (0)