Skip to content

Make Engine instances reusable#106

Merged
SCiarella merged 12 commits intomainfrom
issue_21
Apr 3, 2026
Merged

Make Engine instances reusable#106
SCiarella merged 12 commits intomainfrom
issue_21

Conversation

@SCiarella
Copy link
Copy Markdown
Collaborator

Closes #21 (and #20).

This PR separates engine construction from runtime setup so the same Engine instance can be reused across multiple parameter sets without re-instantiating the full model stack each time.

The main goal is to support workflows where parameters are updated repeatedly, especially in differentiable and optimization use cases, while avoiding the current need to deep-copy the parameter provider before every run.

Changes

  • Added a reusable setup(... to Engine
  • run setup when Engine is initialize
  • add a _reset_runtime_state method so an engine instance can be safely reused
  • define our custom _finish_cropsimulation to avoid calling clear_override() that would pop away variables from the Engine
  • Use our new VariableKiosk to support external states the main engine
  • Simplified EngineTestHelper so it relies on the same engine setup logic
  • Updated differentiable tests to instantiate EngineTestHelper once and call setup(...) for each forward pass, so that we can avoid the combo of pop + deepcopy
  • Fixed existing tests
  • added new tests to cover new methods

So now we basically go from this (before):

for params_dict in parameter_sets:
    crop_model_params_provider = copy.deepcopy(parameters)

    for name, value in params_dict.items():
        crop_model_params_provider.set_override(name, value, check=False)

    model = Engine(
        crop_model_params_provider,
        weather_data,
        agromanagement_data,
        config=config,
    )
    model.run_till_terminate()
    results = model.get_output()

to this (after):

model = Engine(config=config)

for params_dict in parameter_sets:
    for name, value in params_dict.items():
        parameters.set_override(name, value, check=False)

    model.setup(parameters, weather_data, agromanagement_data)
    model.run_till_terminate()
    results = model.get_output()

@SCiarella SCiarella requested a review from SarahAlidoost March 27, 2026 08:30
@SCiarella SCiarella marked this pull request as ready for review March 27, 2026 08:34
Copy link
Copy Markdown
Collaborator

@SarahAlidoost SarahAlidoost left a comment

Choose a reason for hiding this comment

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

@SCiarella nice implementation, thanks! 👍 I left some comments. The main point is to not call the setup in init method, see my comment. If something isn't clear, we can discuss it offline.

Also, sonar complains that there are not enough tests for Engine. Perhaps, after implementing the changes, this will be solved. Otherwise, can you please add more tests?

One more thing, the build action was failed due to FileNotFoundError: [Errno 2] No such file or directory: '/home/runner/work/diffWOFOST/diffWOFOST/tests/physical_models/test_data/test_phenology_wofost72_24.yaml'. But this test passes locally. I re-ran the action but still the same. Can you please check?

SCiarella and others added 11 commits April 1, 2026 11:11
Co-authored-by: SarahAlidoost <55081872+SarahAlidoost@users.noreply.github.com>
Co-authored-by: SarahAlidoost <55081872+SarahAlidoost@users.noreply.github.com>
Co-authored-by: SarahAlidoost <55081872+SarahAlidoost@users.noreply.github.com>
Co-authored-by: SarahAlidoost <55081872+SarahAlidoost@users.noreply.github.com>
@SCiarella
Copy link
Copy Markdown
Collaborator Author

@SarahAlidoost Thanks for the feedback!
I have now removed all the setup operations from init so now the tasks are clearly separated.
Then, I have fixed the configuration used by the tests (24 is not in the --fast subset) and upped the coverage.

Unfortunately sonarcloud is still not satisfied. I was thinking that maybe sonarcloud has problems in calculating the coverage when using the multiprocess version of pytest because after that change it started complaining 🤔 (food for issue #101).

@SCiarella SCiarella requested a review from SarahAlidoost April 1, 2026 11:36
@sonarqubecloud
Copy link
Copy Markdown

sonarqubecloud bot commented Apr 1, 2026

Quality Gate Failed Quality Gate failed

Failed conditions
15.9% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

Copy link
Copy Markdown
Collaborator

@SarahAlidoost SarahAlidoost left a comment

Choose a reason for hiding this comment

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

@SCiarella thanks for addressing the comments, looks great! 👍

This was referenced Apr 3, 2026
@SCiarella SCiarella merged commit 839795c into main Apr 3, 2026
10 of 11 checks passed
@SCiarella SCiarella deleted the issue_21 branch April 3, 2026 10:52
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.

[Task] Running a model with different parameter sets

2 participants