Fixing ve_run command line configuration

[davidorme]

Description

The ve_run command is supposed to take configuration overrides as ve_run ... --param plants.constants.value=0.6 but those values are currently silently swallowed: the dictionary passed into _parse_command_line_params was intended to be updated but was redefined within the function and so the updates never propagated back out into the configuration.

This PR:

• Fixes that bug - the CLI configuration parsing function now explicitly returns a dictionary of command line configuration overrides and the handling of the output path is now included as part of the CLI config string parsing, rather than as a pre-existing dictionary entry.
• Updates the naming of functions and arguments aways from params to config to make it more obvious that this is modifying the model configuration.
• Fixes broken tests - surprisingly few.

It also updates test_ve_run to only run 2 months of simulation. We simply don't need a 24 month simulation as part of the unit testing: two updates is enough to verify that the basic model flow is working and drastically reduces the run time of that test (on my laptop: 180 seconds down to 25 seconds). This was the motivating case for the command line override that revealed the bug.

The Actions testing runtime for the OS/Python combos drops by about 2/3rds: worst case is Windows down from 10' to 3'30". The docs build is now the bad citizen in the actions runtime.

@alexdewar This likely breaks ImperialCollegeLondon/virtual_ecosystem_snakemake_template?

Fixes #1241

Type of change

• New feature (non-breaking change which adds functionality)
• Optimization (back-end change that speeds up the code)
• Bug fix (non-breaking change which fixes an issue)

Key checklist

• Make sure you've run the pre-commit checks: $ pre-commit run -a
• All tests pass: $ poetry run pytest

Further checks

• Code is commented, particularly in hard-to-understand areas
• Tests added that prove fix is effective or that feature works
• Relevant documentation reviewed and updated

[codecov-commenter]

Codecov Report

❌ Patch coverage is 94.11765% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 95.05%. Comparing base (359c3b6) to head (bde680c).
⚠️ Report is 20 commits behind head on develop.

Files with missing lines	Patch %	Lines
virtual_ecosystem/entry_points.py	92.30%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1242      +/-   ##
===========================================
+ Coverage    94.95%   95.05%   +0.09%     
===========================================
  Files           71       71              
  Lines         7291     7293       +2     
===========================================
+ Hits          6923     6932       +9     
+ Misses         368      361       -7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[alexdewar]

Makes sense to me, although I've got a couple of questions:

1. Is there a test that checks that this override functionality works? If not, would it be worth adding one?
2. Have you got any idea how long this has been broken for?

The snakemake template has sadly been broken for a while now 😞 and I haven't got round to fixing it... I can though, if that would be useful to you. I had a PR with mysterious CI failures on Windows that I could debug easily (ImperialCollegeLondon/virtual_ecosystem_snakemake_template#51) and then I got distracted with other things. I suppose it might be worth checking whether it works with the version of VR in this current PR though!

[davidorme]

1. Is there a test that checks that this override functionality works? If not, would it be worth adding one?

There isn't a test - tests/core/test_configuration_builder.py::test_ConfigurationLoader_load_configuration_data passes in the parsed dictionary but there isn't something that checks the data makes it from the CLI to the final configuration. Would be worthwhile, I agree.

2. Have you got any idea how long this has been broken for?

I think... always? The dictionary merging function (was config_merge, now merge_configuration_dicts) has always started with:

    dest = deepcopy(dest)
    source = deepcopy(source)

And I think that is where it goes off the rails and breaks the mutation of the original passed in dictionary. We just never tested it and the snakemake template was at a stage where we wouldn't spot if a config change made any differences?

[alexdewar]

And I think that is where it goes off the rails and breaks the mutation of the original passed in dictionary. We just never tested it and the snakemake template was at a stage where we wouldn't spot if a config change made any differences?

Ah ok. Yes, that's what I was curious about!

[davidorme]

@alexdewar I've added a really simple test that just checks that the CLI input makes it through to the ve_run call. The integration of that data is already tested elsewhere, and this is a short and clean check that the input makes it.

[alexdewar]

LGTM!

Btw @dalonsoa is still away this week, so you might not want to hold out for a review from him, depending on how soon you want this merged.