From 49e376591926b46a8f89a3888551e0827504803b Mon Sep 17 00:00:00 2001 From: oshaughnessy-junior <274802661+oshaughnessy-junior@users.noreply.github.com> Date: Sat, 26 Sep 2026 14:17:30 -0400 Subject: [PATCH 1/2] Fix O4d CPU container fallback transfers and ILE transfer separators --- .../Code/RIFT/misc/dag_utils_generic.py | 16 ++--- .../Code/test/test_container_manifest.py | 63 +++++++++++++++++++ 2 files changed, 72 insertions(+), 7 deletions(-) diff --git a/MonteCarloMarginalizeCode/Code/RIFT/misc/dag_utils_generic.py b/MonteCarloMarginalizeCode/Code/RIFT/misc/dag_utils_generic.py index 832167dcb..da674b520 100644 --- a/MonteCarloMarginalizeCode/Code/RIFT/misc/dag_utils_generic.py +++ b/MonteCarloMarginalizeCode/Code/RIFT/misc/dag_utils_generic.py @@ -2530,8 +2530,10 @@ def write_ILE_sub_simple(tag='integrate', exe=None, log_dir=None, use_eos=False, # names BASENAMES (it may not contain a '/', or condor_submit truncates # it -- see build_container_image_select), so the image itself must be # delivered by file transfer. Runtime-select mode self-fetches inside - # the wrapper, so it is the only mode that skips the token. - if singularity_transfer_expr and not singularity_runtime_select: + # the wrapper. CPU container-universe jobs use a literal fallback image, + # which condor_submit transfers without a GPU-capability expression. + if (singularity_transfer_expr and not singularity_runtime_select + and (request_gpu or not singularity_container_universe)): extra_files += [singularity_transfer_expr] elif singularity_image: if 'osdf:' in singularity_image: @@ -2871,13 +2873,13 @@ def write_ILE_sub_simple(tag='integrate', exe=None, log_dir=None, use_eos=False, if not transfer_files is None: if not isinstance(transfer_files, list): - fname_str=transfer_files + ' '.join(extra_files) + fname_str = ','.join(part for part in [transfer_files] + extra_files if part) else: fname_str = ','.join(transfer_files+extra_files) fname_str=fname_str.strip() ile_job.add_condor_cmd('transfer_input_files', fname_str) ile_job.add_condor_cmd('should_transfer_files','YES') - if singularity_container_universe: + if singularity_container_universe and request_gpu: # condor_submit APPENDS the container_image value to the derived # TransferInput. Our selector names basenames (it may not contain a # '/'), so that appended entry would ask the execute point to fetch a @@ -3118,14 +3120,14 @@ def write_calpilot_sub(tag='calpilot', exe=None, log_dir=None, universe="vanilla singularity_require_gpus_floor = build_require_gpus_floor(_manifest) singularity_container_universe = bool(use_singularity and os.environ.get('RIFT_CONTAINER_UNIVERSE')) if singularity_container_universe: - singularity_container_image_select = build_container_image_select(_manifest) + singularity_container_image_select = build_container_image_select(_manifest, request_gpu=request_gpu) # Selective ($$()) transfer of only the matched osdf image (comma-free so it # survives transfer_input_files comma-splitting). Container universe needs it # too: its container_image selector names BASENAMES (it may not contain a '/', # or condor_submit truncates it), so the image arrives by file transfer. # (container universe requires use_singularity, which already implies on_osg) _transfer_expr = build_transfer_input_expr(_manifest) - if on_osg and _transfer_expr: + if on_osg and _transfer_expr and (request_gpu or not singularity_container_universe): transfer_files += [_transfer_expr] if use_singularity: @@ -3236,7 +3238,7 @@ def write_calpilot_sub(tag='calpilot', exe=None, log_dir=None, universe="vanilla _tif_str = ','.join(transfer_files) job.add_condor_cmd('transfer_input_files', _tif_str) job.add_condor_cmd('should_transfer_files', 'YES') - if singularity_container_universe: + if singularity_container_universe and request_gpu: # condor_submit APPENDS the container_image value to the derived # TransferInput; our selector names basenames, so that entry would ask # the execute point to fetch a bare file name and fail. Pin the list. diff --git a/MonteCarloMarginalizeCode/Code/test/test_container_manifest.py b/MonteCarloMarginalizeCode/Code/test/test_container_manifest.py index e1b495bbb..858b87832 100644 --- a/MonteCarloMarginalizeCode/Code/test/test_container_manifest.py +++ b/MonteCarloMarginalizeCode/Code/test/test_container_manifest.py @@ -411,6 +411,69 @@ def test_integration_cip_legacy_single_image(tmp_path, monkeypatch): assert "require_gpus" not in cmds +@pytest.mark.parametrize("role", ["ILE", "ILE_STRING", "CALPILOT"]) +@pytest.mark.parametrize("request_gpu", [True, False]) +def test_container_universe_effective_job_ad(tmp_path, monkeypatch, role, request_gpu): + """Use condor_submit itself: object-level checks miss selector truncation.""" + import json + import shutil + import subprocess + + submit = shutil.which("condor_submit") + if not submit: + pytest.skip("HTCondor required for effective job-ad validation") + from RIFT.misc import dag_utils_generic as dag + + monkeypatch.setenv("RIFT_CONTAINER_UNIVERSE", "1") + monkeypatch.chdir(tmp_path) + manifest = _write(tmp_path, ALL_OSDF_MANIFEST) + (tmp_path / "all.net").write_text("fixture\n") + (tmp_path / "args_ile.txt").write_text("--n-max 1\n") + (tmp_path / "consolidated_0.composite").write_text("fixture\n") + common = dict(tag=role, exe="/usr/bin/true", log_dir=str(tmp_path) + "/", + use_singularity=True, singularity_image=manifest, + request_gpu=request_gpu, transfer_files=[str(tmp_path / "all.net")]) + if role.startswith("ILE"): + if role == "ILE_STRING": + common["transfer_files"] = str(tmp_path / "all.net") + job, sub = dag.write_ILE_sub_simple(cache_file="local.cache", arg_str="--gpu --force-xpy --vectorized", **common) + else: + job, sub = dag.write_calpilot_sub(working_directory=str(tmp_path), + ile_args_file=str(tmp_path / "args_ile.txt"), **common) + cmds = dict(job.condor_cmds) + if request_gpu: + assert "/" not in cmds["container_image"] + assert str(tmp_path / "all.net") + "," in cmds["transfer_input_files"] + assert "osdf:///" in cmds["transfer_input_files"] + assert "MY.TransferInput" in cmds + else: + assert cmds["container_image"] == "osdf:///igwn/sw/rift_ancient_cuda11.sif" + assert "$$(" not in cmds["transfer_input_files"] + assert "MY.TransferInput" not in cmds + job.add_condor_cmd("macroevent", "0") + job.add_condor_cmd("macroiteration", "0") + job.add_condor_cmd("macroiterationprev", "0") + job.write_sub_file() + ad_path = tmp_path / "effective.ad" + result = subprocess.run([submit, "-disable", "-dry-run", str(ad_path), str(sub)], + capture_output=True, text=True, timeout=30) + assert result.returncode == 0, result.stdout + result.stderr + ad = {} + for line in ad_path.read_text().splitlines(): + key, sep, value = line.partition("=") + key = key.strip() + value = value.strip() + if sep and key in ("ContainerImage", "TransferInput"): + ad[key] = json.loads(value) + if request_gpu: + assert ad["ContainerImage"] == cmds["container_image"] + assert ad["TransferInput"] == cmds["transfer_input_files"].replace("$(macroiteration)", "0") + assert "ifThenElse" not in ad["TransferInput"] # no spurious basename input + else: + assert ad["ContainerImage"] == "rift_ancient_cuda11.sif" + assert "$$(" not in ad["TransferInput"] + + def _make_calibration_job(tmp_path, monkeypatch, manifest, container_universe): if container_universe: monkeypatch.setenv("RIFT_CONTAINER_UNIVERSE", "1") From ccc45e42a63476a21f50bec3c615c73e424dd7f9 Mon Sep 17 00:00:00 2001 From: Session Router Gate Date: Wed, 30 Sep 2026 09:30:54 +0000 Subject: [PATCH 2/2] Address automated review findings for PR #374 --- .../Code/test/test_container_manifest.py | 89 ++++++++++++++----- 1 file changed, 66 insertions(+), 23 deletions(-) diff --git a/MonteCarloMarginalizeCode/Code/test/test_container_manifest.py b/MonteCarloMarginalizeCode/Code/test/test_container_manifest.py index 858b87832..09a4d3e8c 100644 --- a/MonteCarloMarginalizeCode/Code/test/test_container_manifest.py +++ b/MonteCarloMarginalizeCode/Code/test/test_container_manifest.py @@ -3,8 +3,11 @@ SingularityImage / selective-transfer / require_gpus wiring. These run without a real HTCondor pool: the parser + expression builders are -pure, and the integration test inspects the generated ``condor_cmds`` on the -job object returned by ``write_ILE_sub_simple`` (no .sub file or condor needed). +pure, and the integration tests inspect the generated ``condor_cmds`` on the +job object returned by ``write_ILE_sub_simple``, plus the submit file the +backend renders in pure python. Nothing here skips when HTCondor is absent; +``test_container_universe_effective_job_ad`` additionally hands that submit file +to ``condor_submit -dry-run`` wherever that executable exists. Run directly: python test/test_container_manifest.py Or via pytest: pytest test/test_container_manifest.py @@ -414,14 +417,26 @@ def test_integration_cip_legacy_single_image(tmp_path, monkeypatch): @pytest.mark.parametrize("role", ["ILE", "ILE_STRING", "CALPILOT"]) @pytest.mark.parametrize("request_gpu", [True, False]) def test_container_universe_effective_job_ad(tmp_path, monkeypatch, role, request_gpu): - """Use condor_submit itself: object-level checks miss selector truncation.""" + """Check the SUBMITTED text, not just the job object: object-level checks + miss selector truncation. + + Two legs, neither of which skips. The submit-file leg runs EVERYWHERE: the + HTCondor backend has a pure-python submit-file renderer, so the selector can + always be read back as written, and a value that acquires a newline (which + silently truncates the selector into a different, still-plausible submit + description) fails here with no HTCondor present. The condor_submit leg -- + the only one that also proves condor's own parser resolves the expression -- + additionally runs wherever condor_submit is on PATH, i.e. a submit host or a + Condor-enabled gate. + + Deliberately NOT a pytest.skip when condor_submit is absent. This file is a + member of .travis/test-core-units.sh, whose runner has no HTCondor, so six + parametrized skips would spend that gate's skip budget on cases that can + never run there -- and the gate's outcome accounting, which exists to stop a + skip absorbing a lost check, would fail on a check that is not lost. + """ import json - import shutil - import subprocess - submit = shutil.which("condor_submit") - if not submit: - pytest.skip("HTCondor required for effective job-ad validation") from RIFT.misc import dag_utils_generic as dag monkeypatch.setenv("RIFT_CONTAINER_UNIVERSE", "1") @@ -454,24 +469,52 @@ def test_container_universe_effective_job_ad(tmp_path, monkeypatch, role, reques job.add_condor_cmd("macroiteration", "0") job.add_condor_cmd("macroiterationprev", "0") job.write_sub_file() - ad_path = tmp_path / "effective.ad" - result = subprocess.run([submit, "-disable", "-dry-run", str(ad_path), str(sub)], - capture_output=True, text=True, timeout=30) - assert result.returncode == 0, result.stdout + result.stderr - ad = {} - for line in ad_path.read_text().splitlines(): + + # Leg 1, everywhere: the values as WRITTEN. Keys are matched case-folded + # because the renderer used depends on whether the htcondor bindings are + # importable, and only the VALUES are the contract here. + written = {} + with open(str(sub)) as fh: + sub_text = fh.read() + for line in sub_text.splitlines(): key, sep, value = line.partition("=") - key = key.strip() - value = value.strip() - if sep and key in ("ContainerImage", "TransferInput"): - ad[key] = json.loads(value) + if not sep: + continue # 'queue N', and any truncated remnant + key = key.strip().lower() + if key.startswith("+"): + key = "my." + key[1:] # the two spellings of a custom attribute + written.setdefault(key, value.strip()) + assert written["container_image"] == cmds["container_image"] if request_gpu: - assert ad["ContainerImage"] == cmds["container_image"] - assert ad["TransferInput"] == cmds["transfer_input_files"].replace("$(macroiteration)", "0") - assert "ifThenElse" not in ad["TransferInput"] # no spurious basename input + # A selector truncated on write reaches this comparison short, not wrong. + assert written["transfer_input_files"] == cmds["transfer_input_files"] + assert written["my.transferinput"] == cmds["MY.TransferInput"] else: - assert ad["ContainerImage"] == "rift_ancient_cuda11.sif" - assert "$$(" not in ad["TransferInput"] + assert "$$(" not in written["transfer_input_files"] + assert "my.transferinput" not in written + + # Leg 2, Condor-enabled environments only: condor's own parser resolves the + # expression and reports what the job would actually run with. + submit = shutil.which("condor_submit") + if submit: + ad_path = tmp_path / "effective.ad" + result = subprocess.run([submit, "-disable", "-dry-run", str(ad_path), str(sub)], + capture_output=True, text=True, timeout=30) + assert result.returncode == 0, result.stdout + result.stderr + ad = {} + for line in ad_path.read_text().splitlines(): + key, sep, value = line.partition("=") + key = key.strip() + value = value.strip() + if sep and key in ("ContainerImage", "TransferInput"): + ad[key] = json.loads(value) + if request_gpu: + assert ad["ContainerImage"] == cmds["container_image"] + assert ad["TransferInput"] == cmds["transfer_input_files"].replace("$(macroiteration)", "0") + assert "ifThenElse" not in ad["TransferInput"] # no spurious basename input + else: + assert ad["ContainerImage"] == "rift_ancient_cuda11.sif" + assert "$$(" not in ad["TransferInput"] def _make_calibration_job(tmp_path, monkeypatch, manifest, container_universe):