Add positive_int validation schema for NUM_CPU and MAX_SUBMIT

equinor / ert

ERT - Ensemble based Reservoir Tool - is designed for running ensembles of dynamical models such as reservoir models, in order to do sensitivity analysis and data assimilation. ERT supports data assimilation using the Ensemble Smoother (ES), Ensemble Smoother with Multiple Data Assimilation (ES-MDA) and Iterative Ensemble Smoother (IES).

https://ert.readthedocs.io/en/latest/

GNU General Public License v3.0

101 stars 104 forks source link

Add positive_int validation schema for NUM_CPU and MAX_SUBMIT #8082

Closed xjules closed 3 months ago

xjules commented 3 months ago

Issue Resolves https://github.com/equinor/ert/issues/7294 Resolves https://github.com/equinor/ert/issues/8078

Approach Implement positive_int validation

(Screenshot of new behavior in GUI if applicable)

[x] PR title captures the intent of the changes, and is fitting for release notes.
[x] Added appropriate release note label
[x] Commit history is consistent and clean, in line with the contribution guidelines.
[ ] Make sure tests pass locally (after every commit!)

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 commented 3 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 86.01%. Comparing base (99a29f6) to head (6178aab). Report is 1 commits behind head on main.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #8082 +/- ## ========================================== - Coverage 86.02% 86.01% -0.02% ========================================== Files 382 382 Lines 23621 23632 +11 Branches 620 630 +10 ========================================== + Hits 20321 20326 +5 - Misses 3222 3231 +9 + Partials 78 75 -3 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

oyvindeide commented 3 months ago

Should we also validate it in the scheduler, or is it unlikely to be called stand-alone?

berland commented 3 months ago

NUM_CPU is not the responsibility of the Scheduler, it should be encoded into the built Realization objects, which the drivers should pick information from: https://github.com/equinor/ert/issues/8087

oyvindeide commented 3 months ago

NUM_CPU is not the responsibility of the Scheduler, it should be encoded into the built Realization objects, which the drivers should pick information from: #8087

My comment was related to MAX_SUBMIT, which could be 0, which is the responsibility of the scheduler?

xjules commented 3 months ago

Should we also validate it in the scheduler, or is it unlikely to be called stand-alone?

Hm, do you mean like to raise ValueError when the scheduler starts?

berland commented 3 months ago

My comment was related to MAX_SUBMIT, which could be 0, which is the responsibility of the scheduler?

Ah! It might make sense for the scheduler to allow max_submit == 0, it serves as a "dry-run" feature (even proven to work in the wild!). But it should throw a ValueError if it is negative.