Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
26 changes: 17 additions & 9 deletions .github/pull_request_template.md

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ESadek-MO we might consider some messaging in here to avoid scaring new contributors? Some form of "we can help, don't worry".

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I'd agree, I think that'd be nice. I can't think of phrasing though 😂

Original file line number Diff line number Diff line change
@@ -1,14 +1,22 @@
## 🚀 Pull Request
## Description

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

## Checklist

---
[Consult Iris pull request check list]( https://scitools-iris.readthedocs.io/en/latest/developers_guide/contributing_pull_request_checklist.html)
(In addition to a full set of [passing checks](https://scitools-iris.readthedocs.io/en/latest/developers_guide/contributing_ci_tests.html) :white_check_mark:)

---
Add any of the below labels to trigger actions on this PR:
- [ ] Included a [**What's New**](https://scitools-iris.readthedocs.io/en/latest/developers_guide/documenting/whats_new_contributions.html) entry
- [ ] Incorporated [**type hints**](https://scitools-iris.readthedocs.io/en/latest/developers_guide/contributing_code_formatting.html#type-hinting) in any changed code
- [ ] Checked if [**tests**](https://scitools-iris.readthedocs.io/en/latest/developers_guide/contributing_tests.html) need updating
- [ ] Checked if [**benchmarks**](../benchmarks/README.md#writing-benchmarks) need updating
- [ ] Checked if **documentation** needs updating
- [Docstrings](https://scitools-iris.readthedocs.io/en/latest/developers_guide/documenting/docstrings.html) (we love code examples!)
- [User Manual](https://scitools-iris.readthedocs.io/en/latest/user_manual/) pages
- [ ] Checked if [**dependencies**](https://scitools-iris.readthedocs.io/en/latest/developers_guide/contributing_ci_tests.html#gha-test-env) need updating

- https://github.com/SciTools/iris/labels/benchmark_this
---
> [!TIP]
> Add any of the below labels to trigger actions on this PR:
>
> - https://github.com/SciTools/iris/labels/benchmark_this
82 changes: 32 additions & 50 deletions benchmarks/README.md
Comment thread
ESadek-MO marked this conversation as resolved.
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,17 @@ shifts in performance being flagged in a new GitHub issue.

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

> [!TIP]
> To run the benchmarks a second time: remove the
> https://github.com/SciTools/iris/labels/benchmark_this label and add it again.

To run locally: the **benchmark runner** provides conveniences for
common benchmark setup and run tasks, including replicating the benchmarking
performed by GitHub Actions workflows. This can be accessed by:
Expand Down Expand Up @@ -48,42 +52,17 @@ are not already. This can be done in several ways:

### Environment variables

* `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.
* `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. Note that
[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`).
* `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. Note that some of
the generated files, especially in the 'SPerf' suite, are many GB in size so
plan accordingly.
* `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.
* `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. **NOTE:**
SciTools environments tend to large so this option can consume a lot of disk
space.
| Name | Required | Description | Notes |
|------|----------|-------------|-------|
| `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. | |
| `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`). |
| `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. |
| `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. | |
| `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.** |

## Writing benchmarks

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

### What benchmarks to write

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

The drawback of this approach: a reported regression is less likely to reveal
the root cause (e.g. if a commit caused a regression in coordinate-creation
time, but the only benchmark covering this was for file-loading). Be prepared
for manual investigations; and consider committing any useful benchmarks as
[on-demand benchmarks](#on-demand-benchmarks) for future developers to use.
> [!CAUTION]
> The drawback of this approach: a reported regression is less likely to reveal
> the root cause (e.g. if a commit caused a regression in coordinate-creation
> time, but the only benchmark covering this was for file-loading). Be prepared
> for manual investigations; and consider committing any useful benchmarks as
> [on-demand benchmarks](#on-demand-benchmarks) for future developers to use.

### Data generation

**Important:** be sure not to use the benchmarking environment to generate any
test objects/files, as this environment changes with each commit being
benchmarked, creating inconsistent benchmark 'conditions'. The
[generate_data](./benchmarks/generate_data/__init__.py) module offers a
solution; read more detail there.
> [!WARNING]
> Be sure not to use the benchmarking environment to generate any
> test objects/files, as this environment changes with each commit being
> benchmarked, creating inconsistent benchmark 'conditions'. The
> [generate_data](./benchmarks/generate_data/__init__.py) module offers a
> solution; read more detail there.

### ASV re-run behaviour

Expand Down Expand Up @@ -136,9 +117,10 @@ detail.

### Scaling / non-Scaling Performance Differences

**(We no longer advocate the below for benchmarks run during CI, given the
limited available runtime and risk of false-positives. It remains useful for
manual investigations).**
> [!CAUTION]
> We no longer advocate the below for benchmarks run during CI, given the
> limited available runtime and risk of false-positives. It remains useful for
> manual investigations.

When comparing performance between commits/file-type/whatever it can be helpful
to know if the differences exist in scaling or non-scaling parts of the
Expand All @@ -160,16 +142,16 @@ suites for the UK Met Office NG-VAT project.

## Benchmark environments

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

(ASV is written to control the environment(s) that benchmarks are run in -
[^1]: ASV is written to control the environment(s) that benchmarks are run in -
minimising external factors and also allowing it to compare between a matrix
of dependencies (each in a separate environment). We have chosen to sacrifice
these features in favour of testing each commit with its intended dependencies,
controlled by the test environment setup script(s)).
controlled by the test environment setup script(s).
2 changes: 1 addition & 1 deletion benchmarks/custom_bms/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

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

Installation is into the environment where the benchmarks are run (i.e. not
Expand Down
2 changes: 1 addition & 1 deletion benchmarks/custom_bms/install.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
"""Install the SciTools custom benchmarks for detection by ASV.

See the requirements for being detected as an ASV plugin:
https://asv.readthedocs.io/projects/asv-runner/en/latest/development/benchmark_plugins.html
https://asv.readthedocs.io/projects/asv-runner/en/stable/development/benchmark_plugins.html
"""

from pathlib import Path
Expand Down
2 changes: 1 addition & 1 deletion benchmarks/custom_bms/tracemallocbench.py
Original file line number Diff line number Diff line change
Expand Up @@ -192,5 +192,5 @@ def too_slow(num_samples) -> bool:
return samples, 1


# https://asv.readthedocs.io/projects/asv-runner/en/latest/development/benchmark_plugins.html
# https://asv.readthedocs.io/projects/asv-runner/en/stable/development/benchmark_plugins.html
export_as_benchmark = [TracemallocBenchmark]
14 changes: 14 additions & 0 deletions docs/src/developers_guide/contributing_ci_tests.rst
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,20 @@ If any CI tasks fail, then the pull-request is unlikely to be merged to the
Iris target branch by a core developer.


Data Repositories
-----------------

Two other SciTools repositories support the CI tasks:

* `iris-test-data`_ is a github project containing all the data to support the tests.
* `iris-sample-data`_ is a github project containing all the data to support the gallery and examples.

If new files are required by tests or code examples, they must be added to
the appropriate supporting project via a suitable pull-request. This pull
request should be referenced in the main Iris pull request and must be
accepted and merged before the Iris one can be.


.. _testing_cla:

`CLA Assistant`_
Expand Down
31 changes: 16 additions & 15 deletions docs/src/developers_guide/contributing_code_formatting.rst
Comment thread
ESadek-MO marked this conversation as resolved.
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,15 @@
Code Formatting
===============

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

* `black`_ for an opinionated coding auto-formatter
* `flake8`_ linting checks

The preferred way to run these tools automatically is to setup and configure
`pre-commit`_.
``pre-commit`` compliance is automatically checked on all Iris pull requests
(more info: :ref:`pre_commit_ci`), but you can also run pre-commit locally as
Git hooks - every time you make a commit:

@ESadek-MO ESadek-MO Jun 10, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The punctuation feels a bit janky here, but I'm realising I may just have a bias against colons.


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

Expand All @@ -32,15 +33,14 @@ the root directory of Iris::

$ pre-commit install

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

Additionally, you may wish to enable ``black`` for your preferred
`editor/IDE <https://black.readthedocs.io/en/stable/integrations/editors.html#editor-integration>`_.

With the ``pre-commit`` configured, the output of performing a ``git commit``
will look similar to::

Expand All @@ -54,8 +54,7 @@ will look similar to::
2 files changed, 10 insertions(+), 9 deletions(-)


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

Type Hinting
------------
Expand All @@ -67,3 +66,5 @@ add/improve them.


.. _pre-commit: https://pre-commit.com/
.. _pre-commit-config.yaml: https://github.com/SciTools/iris/blob/main/.pre-commit-config.yaml
.. _SciTools wiki page: https://github.com/SciTools/.github/wiki/Linting
54 changes: 4 additions & 50 deletions docs/src/developers_guide/contributing_pull_request_checklist.rst
Original file line number Diff line number Diff line change
Expand Up @@ -5,57 +5,11 @@
Pull Request Checklist
======================

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

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

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

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

* The aim of the change / the problem addressed / a link to the issue.
* How the change has been delivered.

#. **Include a "What's New" entry**, if appropriate.
See :ref:`whats_new_contributions`.
Comment thread
ESadek-MO marked this conversation as resolved.

#. **Check all tests pass**. This includes existing tests and any new tests
added for any new functionality. For more information see
:ref:`developer_running_tests`.
Comment thread
ESadek-MO marked this conversation as resolved.

#. **Check all modified and new source files conform to the required**
:ref:`code_formatting`.
Comment thread
ESadek-MO marked this conversation as resolved.

#. **Check all new dependencies added to the** `requirements`_ **yaml
files.** If dependencies have been added then new nox testing lockfiles
should be generated too, see :ref:`gha_test_env`.
Comment thread
ESadek-MO marked this conversation as resolved.

#. **Check the source documentation been updated to explain all new or changed
features**. Note, we now use numpydoc strings. Any touched code should
be updated to use the docstrings formatting. See :ref:`docstrings`.
Comment thread
ESadek-MO marked this conversation as resolved.

#. **Include code examples inside the docstrings where appropriate**. See
:ref:`contributing.documentation.testing`.
Comment thread
ESadek-MO marked this conversation as resolved.

#. **Check the documentation builds without warnings or errors**. See
:ref:`contributing.documentation.building`
Comment thread
ESadek-MO marked this conversation as resolved.

#. **Check for any new dependencies in the** `readthedocs.yml`_ **file**. This
file is used to build the documentation that is served from
https://scitools-iris.readthedocs.io/en/latest/
Comment thread
ESadek-MO marked this conversation as resolved.

#. **Check for updates needed for supporting projects for test or example
data**. For example:

* `iris-test-data`_ is a github project containing all the data to support
the tests.
* `iris-sample-data`_ is a github project containing all the data to support
the gallery and examples.

If new files are required by tests or code examples, they must be added to
the appropriate supporting project via a suitable pull-request. This pull
request should be referenced in the main Iris pull request and must be
accepted and merged before the Iris one can be.
Comment thread
ESadek-MO marked this conversation as resolved.
.. _GitHub pull request template: https://github.com/SciTools/iris/blob/main/.github/pull_request_template.md
Loading
Loading