Refactor BatchSimulator

[verveerpj]

Issue
Resolves #8753

We probably will replace the BatchSimulator in the future with other ERT functionality, so this PR might not be entirely necessary. Nevertheless, I think the refactored code is clearer which may be useful when we do the work to replace BatchSimulator.

Approach

• Add some GEN_DATA keys to the generated ert config
• Add some code to handle ExtParamConfig parameter configs

Note
I handle ExtParamConfig by parsing them in ensemble_config.py. Currently it seems that ExtParamConfig does not have a corresponding keyword, which would make sense if we assume this is only used in the ert configs generated by everest. For this reason I just drop the data to be added in the generated ert config and generate the corresponding ExtParamConfig objects in ExtParamConfig.from_dict. We should discuss if that is an acceptable solution.

One consequence of this PR is that a lot of tests break that assume that the BatchSimulator can be initialized from an ERT config file. That is not possible anymore, presumably the GEN_DATA could be added to ERT config files, but the ExtParamConfig data cannot. Rather than changing the tests, I chose to patch the BatchSimulator class in the file to recover the old behavior, so that the batch simulator itself can still be tested.

• PR title captures the intent of the changes, and is fitting for release notes.
• Added appropriate release note label
• Commit history is consistent and clean, in line with the contribution guidelines.
• Make sure unit tests pass locally after every commit (git rebase -i main --exec 'pytest tests/ert/unit_tests -n logical -m "not integration_test"')

When applicable

• When there are user facing changes: Updated documentation
• New behavior or changes to existing untested code: Ensured that unit tests are added (See Ground Rules).
• Large PR: Prepare changes in small commits for more convenient review
• Bug fix: Add regression test for the bug
• Bug fix: Create Backport PR to latest release

[codecov-commenter]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.66%. Comparing base (3dfec31) to head (3f5465a).

✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #8899      +/-   ##
==========================================
+ Coverage   91.63%   91.66%   +0.02%     
==========================================
  Files         343      343              
  Lines       21162    21149      -13     
==========================================
- Hits        19392    19386       -6     
+ Misses       1770     1763       -7     

Flag	Coverage Δ
cli-tests	39.83% <57.14%> (+0.02%)	⬆️
gui-tests	73.83% <57.14%> (+<0.01%)	⬆️
performance-tests	50.42% <57.14%> (+0.03%)	⬆️
unit-tests	80.30% <100.00%> (+0.02%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

[yngve-sk]

I think this is a good change and makes sense, and it does not touch the ERT config code either.

[yngve-sk]

I think overall it makes sense, the GEN_DATA part I would approve. But I'm not quite sure about putting some of the ExtParam logic into EnsembleConfig. Depends a bit on how we want to set things up, and I think (not totally sure on this) that if we can make the ExtParam do its thing without touching the EnsembleConfig that is most ideal, though it also isn't doing any harm by being in there (as far as I can see right now). Might become more clear when we refactor the detached server, perhaps it could be initialized separately there. I'm not totally sure, might want to discuss that part some more.

[verveerpj]

But I'm not quite sure about putting some of the ExtParam logic into EnsembleConfig. Depends a bit on how we want to set things up, and I think (not totally sure on this) that if we can make the ExtParam do its thing without touching the EnsembleConfig that is most ideal, though it also isn't doing any harm by being in there (as far as I can see right now). Might become more clear when we refactor the detached server, perhaps it could be initialized separately there. I'm not totally sure, might want to discuss that part some more.

I agree, I was also not completely comfortable with it. I have now pushed a commit that undoes the changes to EnsembleConfig. This means that it is still necessary to inject some things after making the ERT config. But I am now doing that in the Simulator class instead of the BatchSimulator class and that helps to keep most of the refactored code. Let me know what you think.

[yngve-sk]

Should probably remove the TODO: but leave the rest of the comment in place

[yngve-sk]

LGTM, nice change. I think that adding the ExtParamConfig manually into the config dict makes more sense than doing it when initializing the Simulator instance