Use job script on disk for LSF driver and use stdin for PBS #7396

berland · 2024-03-08T06:57:17Z

Let PBS and LSF make explicit job scripts for test robustness

For OpenPBS the explicit job script only exists in memory and
is provided to qsub via stdin.

For LSF, the bsub command seemingly cannot read stdin, and the
job script is placed on runpath with a unique filename that
will be left behind.

This enables more complex jobs (e.g. redirection) submitted to the
drivers submit() function.

Align Python LSF driver with C-driver in supplying the runpath as the
last argument. This is needed to pass a legacy test. Note that legacy
LSF and legacy Torque drivers differ in this behaviour, the latter does
not supply runpath.

The type of the runpath object supplied is changed from str to
pathlib.Path

Issue
Resolves #7327

Approach
Short description of the approach

(Screenshot of new behavior in GUI if applicable)

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 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 · 2024-03-08T09:17:49Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.19%. Comparing base (55b9e3a) to head (84d9257).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7396      +/-   ##
==========================================
+ Coverage   84.48%   85.19%   +0.71%     
==========================================
  Files         384      384              
  Lines       22857    22877      +20     
  Branches      884      876       -8     
==========================================
+ Hits        19310    19491     +181     
+ Misses       3435     3277     -158     
+ Partials      112      109       -3

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

berland · 2024-03-08T12:15:41Z

Onprem testing:
pytest --basetemp=/private/havb/pytest-tmp --lsf tests/integration_tests/scheduler -sxv
Azure testing:

export _ERT_TESTS_DEFAULT_QUEUE_NAME=permanent
pytest --basetemp=/private/havb/pytest-tmp --openpbs tests/integration_tests/scheduler  -sxv

berland · 2024-03-11T14:12:12Z

Some clearification:

We do not like leaving temporary files, especially in runpath, where it can become a de-facto API
We want the drivers to behave similarly

Currently, the on-prem /global/bin/bsub cannot take the job script on stdin, which leaves the two goals above impossible to attain.

berland · 2024-03-12T14:34:08Z

Integration tests with --openpbs on Azure and --lsf onprem has been manually verified (error there will not be visible until next komodo build)

ci/testkomodo.sh

src/ert/scheduler/lsf_driver.py

xjules · 2024-03-13T12:19:46Z

src/ert/scheduler/openpbs_driver.py

+            f"cd {shlex.quote(str(runpath))}\n"
+            f"exec -a {shlex.quote(executable)} {executable} {shlex.join(args)}\n"
+        )
+        script_path = runpath / "job_script.sh"


~~Why not to use the same tempfile.NamedTemporaryFile pattern like in lsf_driver since when runpath=None this will not work or ?~~
Actually where is job_script.sh used?

Good catch, it is not used any longer after changing this PR to use stdin

xjules · 2024-03-13T13:10:32Z

tests/integration_tests/scheduler/test_generic_driver.py

-        pass
+        if str(tmp_path).startswith("/tmp"):
+            print(
+                "Please use --basetemp option to pytest, PBS tests needs a shared disk"


Suggested change

"Please use --basetemp option to pytest, PBS tests needs a shared disk"

"Please use --basetemp option to pytest, the real PBS cluster needs a shared disk"

xjules · 2024-03-13T13:11:09Z

tests/integration_tests/scheduler/test_generic_driver.py

-        pass
+        if str(tmp_path).startswith("/tmp"):
+            print(
+                "Please use --basetemp option to pytest, LSF tests needs a shared disk"


Suggested change

"Please use --basetemp option to pytest, LSF tests needs a shared disk"

"Please use --basetemp option to pytest, the real LSF cluster needs a shared disk"

xjules · 2024-03-13T13:18:52Z

tests/integration_tests/scheduler/test_openpbs_driver.py

    with open("poly.ert", mode="a+", encoding="utf-8") as f:
-        f.write("QUEUE_SYSTEM TORQUE\nNUM_REALIZATIONS 2")
+        f.write("QUEUE_SYSTEM TORQUE\nNUM_REALIZATIONS 2\n")
+        if queue_name is not None:


Suggested change

if queue_name is not None:

if (queue_name: = os.getenv("_ERT_TESTS_DEFAULT_QUEUE_NAME")) is not None:

Get one less line also for the other tests since queue_name is used only in the if statements.

used a fixture as well.

xjules

Just small comments. It looks very good otherwise! :)

xjules · 2024-03-13T13:46:27Z

tests/integration_tests/scheduler/test_openpbs_driver.py

@@ -19,12 +20,19 @@ def mock_openpbs(pytestconfig, monkeypatch, tmp_path):
    mock_bin(monkeypatch, tmp_path)


-@pytest.mark.timeout(30)
+@pytest.fixture()


For OpenPBS the explicit job script only exists in memory and is provided to qsub via stdin. For LSF, the bsub command seemingly cannot read stdin, and the job script is placed on runpath with a unique filename that will be left behind. This enables more complex jobs (e.g. redirection) submitted to the drivers submit() function. Align Python LSF driver with C-driver in supplying the runpath as the last argument. This is needed to pass a legacy test. Note that legacy LSF and legacy Torque drivers differ in this behaviour, the latter does not supply runpath. The type of the runpath object supplied is changed from str to pathlib.Path

xjules

Nice job @berland 👍 🚀

berland force-pushed the pbs_leave_script_on_file branch from 6b6a28c to 91fff17 Compare March 8, 2024 07:05

berland force-pushed the pbs_leave_script_on_file branch from 2f898a6 to e9088d3 Compare March 8, 2024 12:11

berland marked this pull request as ready for review March 8, 2024 12:11

berland self-assigned this Mar 8, 2024

berland changed the title ~~Leave job script on disk for PBS driver~~ Leave job script on disk for PBS and LSF drivers Mar 8, 2024

berland added the scheduler label Mar 8, 2024

berland force-pushed the pbs_leave_script_on_file branch from e9088d3 to b312116 Compare March 8, 2024 12:13

berland marked this pull request as draft March 8, 2024 12:43

berland force-pushed the pbs_leave_script_on_file branch 2 times, most recently from 8e13e55 to 5067a06 Compare March 11, 2024 13:36

berland force-pushed the pbs_leave_script_on_file branch from ab861f8 to 0c5db19 Compare March 11, 2024 14:33

berland marked this pull request as ready for review March 11, 2024 14:33

berland added the release-notes:misc Automatically categorise as miscellaneous change in release notes label Mar 11, 2024

berland force-pushed the pbs_leave_script_on_file branch 2 times, most recently from c019c08 to 50063af Compare March 12, 2024 12:31

berland changed the title ~~Leave job script on disk for PBS and LSF drivers~~ Leave job script on LSF driver and use stdin for PBS Mar 12, 2024

berland changed the title ~~Leave job script on LSF driver and use stdin for PBS~~ Use job script on disk for LSF driver and use stdin for PBS Mar 12, 2024

berland force-pushed the pbs_leave_script_on_file branch 4 times, most recently from cca6304 to ca13b08 Compare March 12, 2024 14:32

berland force-pushed the pbs_leave_script_on_file branch from ca13b08 to 33f7472 Compare March 12, 2024 16:36