metoppv / improver

IMPROVER is a library of algorithms for meteorological post-processing.
http://improver.readthedocs.io/en/latest/
BSD 3-Clause "New" or "Revised" License
101 stars 84 forks source link

Migrate CLI functionality to plugin layer subset 1 #2000

Closed cpelley closed 1 month ago

cpelley commented 2 months ago

First subset of CLI modifications to IMPROVER required by EPP. This is intended to test the water against the types of change that I would make to the remainder CLI's.

CLI layers modified:

High-level summary of changes:

Issues

Mock/patch usage explanation

In this test, we patch both the as_cube and as_cubelist functions, replacing them with mock objects. This allows us to verify how they are called within the code. Specifically, we define a side_effect for the as_cube mock object to raise a custom HaltExecution exception when invoked. This setup enables us to check that the as_cube and as_cubelist functions are called with the correct arguments and ensures that the remainder of the plugin code (which isn't under test) does not execute.

The use of sentinel helps create unique, identifiable objects for testing purposes. By passing a dummy template cube object (a sentinel), we can verify that the mocked as_cube function is called with it as its argument. This approach simplifies the test setup by eliminating the need to create real iris cubes or handle the complexities associated with them. Thus, our unit test remains focused and isolated, only testing the behavior under consideration without external dependencies.

class HaltExecution(Exception):
    pass

@patch("improver.utilities.copy_attributes.as_cube")
@patch("improver.utilities.copy_attributes.as_cubelist")
def test_as_cubelist_called(mock_as_cubelist, mock_as_cube):
    mock_as_cubelist.return_value = sentinel.cubelist
    mock_as_cube.side_effect = HaltExecution
    try:
        CopyAttributes(["attribA", "attribB"])(
            sentinel.cube0, sentinel.cube1, template_cube=sentinel.template_cube
        )
    except HaltExecution:
        pass
    mock_as_cubelist.assert_called_once_with(sentinel.cube0, sentinel.cube1)
    mock_as_cube.assert_called_once_with(sentinel.template_cube)
cpelley commented 2 months ago

@gavinevans, @bayliffe

I think we want might eventually want to move away from the use of the term 'Plugin', so any ideas for suitable alternative naming to MetaPluginCloudCondensationLevel are welcome. Processing module is being thrown around as a way of people describing what IMPROVER currently calls plugins (EPP, otherwise). So perhaps MetaProcModuleCloudCondensationLevel?

cpelley commented 2 months ago

Thanks for your thorough review @bayliffe ❤️ I'll take action now. I'll also update the PR description with details around the use of mock, sentinel etc. (apologies for this oversight).

TomekTrzeciak commented 2 months ago

I think we want might eventually want to move away from the use of the term 'Plugin',

Hallelujah!

cpelley commented 2 months ago

@bayliffe, I have updated the ticket description with an explanation of my usage of mock/path/sentinel.

cpelley commented 2 months ago

EDIT: I have just generalised as_cube and as_cubelist support for inputs to better reflect my current interpretation of the reviewer feedback (I reimplemented flatten since we are reliant on the handling of this function).

cpelley commented 1 month ago

@bayliffe, appologies for making additional changes. I looked back at this again after our brief chat this morning and felt that perhaps what I had done wasn't quite aligned with your review feedback. Now, in summary:

cpelley commented 1 month ago

The second half of EPP required IMPROVER plugin changes are captured in https://github.com/metoppv/improver/pull/2003 (that PR builds on this for the remaining plugins).

cpelley commented 1 month ago

Thanks @bayliffe, @gavinevans I have made changes in reflection of the reviews given 👍

cpelley commented 1 month ago

Thanks @gavinevans, I think all feedback has been actioned.

People have yet to take the bait on my request for discussion around naming convention to these meta 'plugins'. In absence of discussion, I have now gone with simply Meta<name>... (motivated by avoiding the discussion around semantics of the term plugin by removing it from the name). I think 'processing module' is potentially more useful term but taking this decision to rename should hopefully allow us to leave that discussion for another time.

New issues spawned from this work (out of scope of this PR):

gavinevans commented 1 month ago

@cpelley I'm not sure that I'm best placed to comment on the use of the word "plugin" to describe the IMPROVER "plugins", but if you'd like to not include this term within your Meta "plugins", then that seems fine from my perspective.