Add test for the propagation of queue_options from ertconfig to cluster #10147

jonathan-eq · 2025-02-25T13:09:22Z

Issue
Resolves #10141

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 unit tests pass locally after every commit (git rebase -i main --exec 'just rapid-tests')

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

codspeed-hq · 2025-02-25T13:32:43Z

CodSpeed Performance Report

Merging #10147 will not alter performance

_{Comparing jonathan-eq:fix-tests (9a8ec0e) with main (3d3894a)}

Summary

✅ 25 untouched benchmarks

tests/ert/unit_tests/scheduler/test_slurm_driver.py

JHolba

the new tests are good :)

berland · 2025-02-27T08:24:33Z

tests/ert/unit_tests/scheduler/bin/bhist.py

@@ -83,18 +83,16 @@ def main() -> None:

    jobs_output: list[Job] = []
    for job in args.jobs:
-        job_name: str = read(jobs_path / f"{job}.name") or "_"
+        job_name: str = read(jobs_path / job / "name") or "_"


Are these changes related to "Add tests for the propagation" or does these changes serve some other purpose (was looking for a longer description in the commit message but could not find it).

(two years down the road we will be looking at the git blame for this line and dont get why this was changed based on reading the log message)

This was just refactoring. In the cases we run multiple realizations with the mocked binaries, I think it makes sense that each job has its own directory instead of prefixing the files with the job_id.

Updated the commit message to include this^

If refactoring (and not needed for being able to add tests), it would make sense to have it in a separate commit.

berland · 2025-02-27T08:45:52Z

tests/ert/unit_tests/scheduler/test_lsf_driver.py

+@pytest.mark.usefixtures("copy_poly_case")
+def test_queue_options_are_propagated_from_config_to_bsub(monkeypatch):
+    """
+    This end to end test is here to verify that queue_options are correctly propagated all the way from ert config to the cluster.


(this could be line formatted, I speculate it extends 88 characters)

berland · 2025-02-27T08:48:39Z

tests/ert/unit_tests/scheduler/test_lsf_driver.py

+    job_dir = next(
+        mock_jobs_dir.iterdir()
+    )  # There is only one realization in this test
+    complete_command_invocation = (job_dir / "complete_command_invocation").read_text(


Isn't this the same as the capturing_bsub fixture? If it is, maybe consider using that, or is this way better?

berland · 2025-02-27T08:50:49Z

tests/ert/unit_tests/scheduler/test_lsf_driver.py

+            )
+        )
+    run_cli(ENSEMBLE_EXPERIMENT_MODE, "--disable-monitoring", "poly.ert")
+    mock_jobs_dir = Path(f"{os.environ.get('PYTEST_TMP_PATH')}/mock_jobs")


it smells a little bit to use this environment variable. Will capturing_bsub solve it or can't you find it from cwd?

berland · 2025-02-27T08:51:17Z

tests/ert/unit_tests/scheduler/test_openpbs_driver.py

+    job_dir = next(
+        mock_jobs_dir.iterdir()
+    )  # There is only one realization in this test
+    complete_command_invocation = (job_dir / "complete_command_invocation").read_text(


check capturing_qsub

capturing_qsub and capturing_bsub does not actually run what is submitted, so it wouldn't be a end-to-end test

But scheduler/bin/bsub.py is just a mock, so this is not an end-to-end test in any case.

If the real bsub command from LSF gets a breaking change requiring a change in our code, this code will not catch it, so I am unsure if it adds value to this test to use scheduler/bin/bsub.py over capturing_bsub.

You are right. I am only interested in the part from ErtConfig to bsub/qsub/sbatch, so I can probably rewrite the test to be an integration test for ertconfig to driver

We have some unit tests for this which I could implement for all queue_options like this

@pytest.mark.usefixtures("capturing_bsub") async def test_submit_with_project_code(): queue_config_dict = { "QUEUE_SYSTEM": "LSF", "FORWARD_MODEL": [("FLOW",), ("ECLIPSE",), ("RMS",)], } queue_config = QueueConfig.from_dict(queue_config_dict) driver: LsfDriver = create_driver(queue_config.queue_options) await driver.submit(0, "sleep") assert f"-P {queue_config.queue_options.project_code}" in Path( "captured_bsub_args" ).read_text(encoding="utf-8")

But this part might not be accurate to how we do it in ensemble.py, and any breaking changes there would not be picked up by the tests.

queue_config = QueueConfig.from_dict(queue_config_dict) driver: LsfDriver = create_driver(queue_config.queue_options)

berland · 2025-02-27T08:52:30Z

tests/ert/unit_tests/scheduler/test_openpbs_driver.py

+    assert parsed_options.get("-q") == expected_queue
+    assert parsed_options.get("-A") == expected_project_code
+
+    # -l was not parsed correctly by getopt, so we read it manually instead.


Do you really need to parse the command line or just rely on certain strings being present in the command?

Yes, you are right. Very good!

This commit refactors the mocked binaries to have each job use its own sub-directory instead of prefixing the files with job_id.

This commit adds tests that verify that the queue_options are all propagated correctly to the slurm, openpbs, and lsf queue system, using our mocked binaries.

jonathan-eq · 2025-02-27T12:36:41Z

tests/ert/unit_tests/scheduler/test_lsf_driver.py

@@ -223,7 +239,7 @@ async def test_submit_sets_stderr():


 @pytest.mark.usefixtures("capturing_bsub")
-async def test_submit_with_realization_memory_with_bsub_capture():
+async def test_submit_with_realization_memory_with_bsub_capture():  # JONAK - CAN THIS BE REMOVED?


This is a unit test, but we are testing the same thing in an integration test. Should we keep both as the integration_tests are not run with just rapid-tests?

xjules · 2025-02-27T13:49:02Z

General note: it looks to be very complicated. I thought something similar to this test?

ert/tests/ert/unit_tests/test_tracking.py

Line 147 in 7642464

def test_tracking(

Which essentially create dedicated models and check that the scheduler got the parameters correctly.

jonathan-eq added the release-notes:misc Automatically categorise as miscellaneous change in release notes label Feb 25, 2025

jonathan-eq force-pushed the fix-tests branch from 6772ae3 to 36041df Compare February 25, 2025 13:10

jonathan-eq force-pushed the fix-tests branch 2 times, most recently from 9e9fb21 to 3c87d49 Compare February 25, 2025 15:49

jonathan-eq marked this pull request as ready for review February 25, 2025 15:55

jonathan-eq force-pushed the fix-tests branch 2 times, most recently from 792e6b2 to 71393a7 Compare February 26, 2025 07:47

JHolba requested changes Feb 26, 2025

View reviewed changes

tests/ert/unit_tests/scheduler/test_slurm_driver.py Outdated Show resolved Hide resolved

jonathan-eq force-pushed the fix-tests branch from 71393a7 to a04bc0b Compare February 26, 2025 12:10

JHolba approved these changes Feb 26, 2025

View reviewed changes

berland reviewed Feb 27, 2025

View reviewed changes

jonathan-eq force-pushed the fix-tests branch from a04bc0b to 162e24e Compare February 27, 2025 08:36

berland reviewed Feb 27, 2025

View reviewed changes

jonathan-eq added 2 commits February 27, 2025 10:13

Refactor test mocked binaries for queue_system interaction

46e4d11

This commit refactors the mocked binaries to have each job use its own sub-directory instead of prefixing the files with job_id.

Add test for the propagation of queue_options from ertconfig to cluster

a4f8e8c

This commit adds tests that verify that the queue_options are all propagated correctly to the slurm, openpbs, and lsf queue system, using our mocked binaries.

jonathan-eq force-pushed the fix-tests branch from 162e24e to a4f8e8c Compare February 27, 2025 09:34

jonathan-eq added 2 commits February 27, 2025 11:52

fixup - have new test be usable against actual cluster

7f9f68b

add more tests

9a5b094

jonathan-eq commented Feb 27, 2025

View reviewed changes

jonathan-eq requested a review from berland February 27, 2025 12:37

jonathan-eq force-pushed the fix-tests branch 3 times, most recently from e0d02ea to cd141a3 Compare February 27, 2025 13:32

fixup tests

9a8ec0e

jonathan-eq force-pushed the fix-tests branch from cd141a3 to 9a8ec0e Compare February 27, 2025 13:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add test for the propagation of queue_options from ertconfig to cluster #10147

Add test for the propagation of queue_options from ertconfig to cluster #10147

jonathan-eq commented Feb 25, 2025 •

edited

Loading

codspeed-hq bot commented Feb 25, 2025 •

edited

Loading

JHolba left a comment

berland Feb 27, 2025

jonathan-eq Feb 27, 2025

jonathan-eq Feb 27, 2025

berland Feb 27, 2025 •

edited

Loading

berland Feb 27, 2025

berland Feb 27, 2025

berland Feb 27, 2025

berland Feb 27, 2025

jonathan-eq Feb 27, 2025

berland Feb 27, 2025

jonathan-eq Feb 27, 2025

jonathan-eq Feb 27, 2025

berland Feb 27, 2025

jonathan-eq Feb 27, 2025

jonathan-eq Feb 27, 2025

xjules commented Feb 27, 2025

Add test for the propagation of queue_options from ertconfig to cluster #10147

Are you sure you want to change the base?

Add test for the propagation of queue_options from ertconfig to cluster #10147

Conversation

jonathan-eq commented Feb 25, 2025 • edited Loading

When applicable

codspeed-hq bot commented Feb 25, 2025 • edited Loading

Merging #10147 will not alter performance

Summary

JHolba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

berland Feb 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xjules commented Feb 27, 2025

jonathan-eq commented Feb 25, 2025 •

edited

Loading

codspeed-hq bot commented Feb 25, 2025 •

edited

Loading

berland Feb 27, 2025 •

edited

Loading