Skip to content

Restructure of BaseModel initialisation flow#1193

Merged
davidorme merged 11 commits intodevelopfrom
1191-restructure-of-basemodel-initialisation-flow
Dec 16, 2025
Merged

Restructure of BaseModel initialisation flow#1193
davidorme merged 11 commits intodevelopfrom
1191-restructure-of-basemodel-initialisation-flow

Conversation

@davidorme
Copy link
Copy Markdown
Collaborator

@davidorme davidorme commented Dec 11, 2025

Description

This PR switches over to the more explicit sequencing of the model __init__ described in #1191:

def __init__(self, data, core_components, static, model_arg) -> None:

    # Set the core stuff including static status flags
    super().__init__(data, core_components, static)
    
    # Declare and type attributes
    self.model_attribute: type
    """Model attribute declarations"""
    
    # Run the setup if the model is not in deep static mode
    if self._run_setup:
        self._setup(model_arg=model_arg)

This:

  • is clearer about what happens when, rather than hiding away the execution of _setup.
  • ditches the kwargs components of the BaseModel arguments and has an explicit list of model arguments. This identified a couple of testing issues where old model arguments were still in test code, because kwargs lets them sneak through.
  • as a result, all the model specific arguments are directly specified in the model __init__ signature.

To do this:

  • I've lightly modified the BaseModel, including changes to the static flagging functions to set model attributes rather than passing around boolean return values. That has included inverting _bypass_setup to _run_setup to sit alongside _run_update and to give the more intuitive if self._run_setup: above.

  • Updates all of the model files to the new structure above. I've re-ordered the sequence of methods to be consistent (__init__, _setup, from_config) across models.

  • Updates the class docstrings to actually document all of the constructor args. It would be nice if ruff validated that class docstrings contained all the __init__ args, but that doesn't seem easy to do. May be we should move Args inside __init__.

  • Provides some minor fixes to the tests:

    • Update to the new _run_setup value from _bypass_setup (True <-> False swaps)
    • Removes some rogue arguments to model __init__

Important

This also revealed a bigger issue with the use of patching of the static settings in model __init__ and _setup testing, which was masking some missing init vars.

I think this is being done because the tests use compiled sets of test data that include variables created by init and update, so the static settings were being patched to avoid raising configuration errors. That's a problem because it breaks the explicit variable checking - for example, in the abiotic model it was masking incomplete required vars lists - the variable might not be in the required list, but is in the data anyway, so the test runs.

I've fixed one case here because it was the easiest way to get tests to pass, but we should separately remove all of this patching and simply subset the more inclusive datasets down to the actual alleged required variables.

Fixes #1161

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

@davidorme davidorme linked an issue Dec 11, 2025 that may be closed by this pull request
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Dec 11, 2025

Codecov Report

❌ Patch coverage is 90.00000% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 94.95%. Comparing base (750a13c) to head (6aa9441).

Files with missing lines Patch % Lines
virtual_ecosystem/models/plants/plants_model.py 72.00% 7 Missing ⚠️
virtual_ecosystem/models/animal/animal_model.py 90.19% 5 Missing ⚠️
virtual_ecosystem/models/testing/testing_model.py 25.00% 3 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1193      +/-   ##
===========================================
- Coverage    94.96%   94.95%   -0.02%     
===========================================
  Files           71       71              
  Lines         7273     7288      +15     
===========================================
+ Hits          6907     6920      +13     
- Misses         366      368       +2     

☔ 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.

@davidorme davidorme requested a review from dalonsoa December 15, 2025 16:30
Copy link
Copy Markdown
Collaborator

@dalonsoa dalonsoa left a comment

Choose a reason for hiding this comment

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

Looks nice. Definitely more consistent across the code. Happy to unmock the mocks in the tests, as that is my mess.

@davidorme
Copy link
Copy Markdown
Collaborator Author

@dalonsoa I realised that the class attributes section of the "defining_new_models.md" process was woefully out of date. I've updated it and gone into some detail about the resolution of execution order and the core Data object. That then makes it sensible to specifically talk about additional data, so I've kept the pond_data_path example but added a tip on why you might do that.

Could you give the most recent commit 5f90307 a quick look to see if it all makes sense?

@davidorme davidorme requested a review from dalonsoa December 16, 2025 10:36
Copy link
Copy Markdown
Collaborator

@dalonsoa dalonsoa left a comment

Choose a reason for hiding this comment

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

Aside from my comment, which if addressed should be done so in a separate PR, this looks good.

@davidorme davidorme merged commit 0696c7f into develop Dec 16, 2025
13 checks passed
@davidorme davidorme deleted the 1191-restructure-of-basemodel-initialisation-flow branch December 16, 2025 20:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Restructure of BaseModel initialisation flow.

4 participants