container universe: fix container_image selector truncation (holds every ILE job on OSPool)

MonteCarloMarginalizeCode/Code/RIFT/misc/container_manifest.py

@@ -105,6 +105,14 @@ def _image_runtime_path(image):
     return image
 
 
+def _image_basename(image):
+    """The bare file name an image has once transferred into the job scratch dir.
+
+    Used by the container-universe selector, which MUST NOT contain a ``/``.
+    """
+    return image.rstrip("/").split("/")[-1]
+
+
 def _fmt_cap(value):
     """Format a capability number for a ClassAd expression (e.g. 7.0 -> '7.0')."""
     return repr(float(value))
@@ -311,17 +319,36 @@ def build_container_image_select(manifest, request_gpu=True):
     GPU jobs (``request_gpu=True``, the default) get a per-machine selection: an
     unquoted ``$$([ ... ])`` token.  ``$$()`` is HTCondor's *match-time machine-ad
     substitution* -- the schedd evaluates the bracketed expression against the
-    matched machine ad and substitutes a literal image string into
-    ``container_image`` before the job reaches the execution point.  Unlike
-    :func:`build_singularity_image_expr` (an execute-side ClassAd expression that
-    OSPool glidein pilots read as a literal string and hold on), the pilot only
-    ever sees a literal URL.  ``$$`` in ``container_image`` is HTCondor's
-    documented mechanism for per-GPU-capability image selection, and it works on
-    both the CIT-local pool and OSPool glideins.  The branch value is the manifest
-    image *verbatim* (an ``osdf://`` URL the container-universe file-transfer
-    plugin fetches, or a CVMFS/local path used in place) -- NOT a ``./basename``
-    rewrite.  ``container_image`` is a single submit command (not a comma list),
-    so the comma-bearing ``ifThenElse`` form is fine.
+    matched machine ad and substitutes a literal string before the job reaches the
+    execution point.  Unlike :func:`build_singularity_image_expr` (an execute-side
+    ClassAd expression that OSPool glidein pilots read as a literal string and hold
+    on), the pilot only ever sees a literal image name.  ``container_image`` is a
+    single submit command (not a comma list), so the comma-bearing ``ifThenElse``
+    form is fine here.
+
+    **The branch values are BASENAMES, not full URLs.**  ``condor_submit`` parses
+    ``container_image`` *before* any ``$$`` expansion and derives the job ad's
+    ``ContainerImage`` -- the name the image will have in the job scratch dir -- as
+    the text after the **last** ``/``.  A selector containing full paths therefore
+    gets cut in half, and what survives is not even a valid image name.  This is not
+    theoretical: submitting the full-URL form to the IGWN pool holds the job at the
+    execute point with::
+
+        PREPARE_JOB (prepare-hook) failed (reported status 001):
+        Unable to download or build singularity image cutest_busybox_...sif") ])
+
+    With no ``/`` in the value, that derivation is a no-op, the whole ``$$`` token
+    survives into ``ContainerImage``, and the schedd expands it at match time
+    (``MATCH_EXP_ContainerImage = "rift_container_modern.sif"``) -- verified end to
+    end on an OSPool glidein.
+
+    Because the selector now names only basenames, the caller MUST also deliver the
+    matched image itself: add :func:`build_transfer_input_expr` (the comma-free
+    ternary over the full URLs) to ``transfer_input_files`` **and** emit it as
+    ``MY.TransferInput`` so it overrides the entry ``condor_submit`` would otherwise
+    derive from ``container_image``.  All images in the family must therefore be
+    transferable URLs; a family that references an image in place (CVMFS/local path)
+    cannot be selected this way and raises :class:`ContainerManifestError`.
 
     **Non-GPU jobs (``request_gpu=False``) collapse to a SINGLE fixed container**:
     the plain ``fallback`` image (a literal ``container_image``, no ``$$()``).
@@ -339,9 +366,20 @@ def build_container_image_select(manifest, request_gpu=True):
     by_label = {c["label"]: c for c in manifest["containers"]}
     fb_image = by_label[manifest["fallback"]]["image"]
     if not request_gpu:
-        # Single fixed container: no capability, no $$() -- a plain literal.
+        # Single fixed container: no capability, no $$() -- a plain literal.  This is
+        # the ordinary single-image path condor_submit handles correctly (it derives
+        # ContainerImage as the basename, which is exactly right).
         return fb_image
-    selector = _build_selector(manifest, lambda c: '"{}"'.format(c["image"]))
+    in_place = [c["label"] for c in manifest["containers"] if not _image_needs_transfer(c["image"])]
+    if in_place:
+        raise ContainerManifestError(
+            "container universe per-machine selection requires every image in the family "
+            "to be a transferable URL (e.g. osdf://), because the selector may not contain "
+            "a '/' -- condor_submit would truncate it.  In-place image(s): {}.  Either "
+            "stage those images at a URL, or use RIFT_CONTAINER_RUNTIME_SELECT=1 instead "
+            "of RIFT_CONTAINER_UNIVERSE=1.".format(", ".join(sorted(in_place)))
+        )
+    selector = _build_selector(manifest, lambda c: '"{}"'.format(_image_basename(c["image"])))
     return "$$([ {} ])".format(selector)
 
 

MonteCarloMarginalizeCode/Code/RIFT/misc/dag_utils_generic.py

@@ -2524,11 +2524,14 @@ def write_ILE_sub_simple(tag='integrate', exe=None, log_dir=None, use_eos=False,
         # Selective transfer: only the matched osdf image is fetched (via the
         # $$() token, which is comma-free so it survives transfer_input_files
         # comma-splitting).  CVMFS/local images are referenced in place and
-        # never transferred, so the whole family is never pulled.  In container-
-        # universe mode the image is delivered via container_image itself; in
-        # runtime-select mode the wrapper self-fetches.  In both cases do NOT add
-        # the match-time transfer token.
-        if singularity_transfer_expr and not singularity_container_universe and not singularity_runtime_select:
+        # never transferred, so the whole family is never pulled.
+        #
+        # Container universe needs this token too: its container_image selector
+        # 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:
             extra_files += [singularity_transfer_expr]
     elif singularity_image:
         if 'osdf:' in singularity_image:
@@ -2874,6 +2877,15 @@ def write_ILE_sub_simple(tag='integrate', exe=None, log_dir=None, use_eos=False,
         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:
+            # 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
+            # bare file name from the access point and fail.  Set TransferInput
+            # directly -- emitted after transfer_input_files, it wins -- so the
+            # list is exactly ours, with the matched image supplied by the
+            # comma-free $$() ternary already in extra_files.
+            ile_job.add_condor_cmd('MY.TransferInput', '"' + fname_str.replace('"', '\\"') + '"')
 
     if not transfer_output_files is None:
         if not isinstance(transfer_output_files, list):
@@ -3107,13 +3119,14 @@ def write_calpilot_sub(tag='calpilot', exe=None, log_dir=None, universe="vanilla
         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)
-        else:
-            # Selective ($$()) transfer of only the matched osdf image (comma-free so
-            # it survives transfer_input_files comma-splitting).  In container-universe
-            # mode the image is delivered via container_image itself, so skip this.
-            _transfer_expr = build_transfer_input_expr(_manifest)
-            if on_osg and _transfer_expr:
-                transfer_files += [_transfer_expr]
+        # 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:
+            transfer_files += [_transfer_expr]
 
     if use_singularity:
         base = os.environ.get('SINGULARITY_BASE_EXE_DIR', '/usr/bin/')
@@ -3220,8 +3233,14 @@ def write_calpilot_sub(tag='calpilot', exe=None, log_dir=None, universe="vanilla
         # absolute paths -> condor transfers each to the worker scratch dir by basename,
         # which is what the stage args (basenames) reference.
         transfer_files += [wd + "/consolidated_$(macroiteration).composite", ile_args_file]
-        job.add_condor_cmd('transfer_input_files', ','.join(transfer_files))
+        _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:
+            # 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.
+            job.add_condor_cmd('MY.TransferInput', '"' + _tif_str.replace('"', '\\"') + '"')
         job.add_condor_cmd('when_to_transfer_output', 'ON_EXIT')
         job.add_condor_cmd('transfer_output_files', 'cal_consolidated_$(macroiteration).npz')
     # Container-family GPU jobs (CALPILOT runs ILE on a GPU): exclude slots that

MonteCarloMarginalizeCode/Code/test/test_container_manifest.py

@@ -58,6 +58,22 @@
 )
 
 
+ALL_OSDF_MANIFEST = textwrap.dedent(
+    """
+    version: 1
+    fallback: ancient
+    containers:
+      - label: ancient
+        image: osdf:///igwn/sw/rift_ancient_cuda11.sif
+        cuda_capability_min: 3.0
+        cuda_capability_max: 7.0
+      - label: modern
+        image: osdf:///igwn/sw/rift_modern_cuda12.sif
+        cuda_capability_min: 7.0
+    """
+)
+
+
 def _write(tmp_path, text, name="fam.yaml"):
     p = tmp_path / name
     p.write_text(text)
@@ -147,7 +163,8 @@ def test_selectors_are_not_undefined_guarded(tmp_path):
     m = cm.load_container_manifest(_write(tmp_path, MIXED_MANIFEST))
     assert "=?= undefined" not in cm.build_singularity_image_expr(m)
     assert "=?= undefined" not in cm.build_transfer_input_expr(m)
-    assert "=?= undefined" not in cm.build_container_image_select(m)
+    assert "=?= undefined" not in cm.build_container_image_select(
+        cm.load_container_manifest(_write(tmp_path, ALL_OSDF_MANIFEST, "osdf.yaml")))
 
 
 def test_capability_defined_requirement(tmp_path):
@@ -257,17 +274,33 @@ def test_backward_compat_single_sif(tmp_path, monkeypatch):
 # ---------------------------------------------------------------------------
 
 def test_container_image_select_expression(tmp_path):
-    m = cm.load_container_manifest(_write(tmp_path, MIXED_MANIFEST))
+    m = cm.load_container_manifest(_write(tmp_path, ALL_OSDF_MANIFEST))
     expr = cm.build_container_image_select(m)
-    # a $$() match-time substitution token with VERBATIM image values (osdf URL
-    # fetched by container universe; cvmfs path used in place) -- NOT a ./basename
-    # rewrite, and NOT undefined-guarded (Requirements exclusion is used instead)
+    # A $$() match-time substitution token over BASENAMES.  condor_submit derives the
+    # job ad's ContainerImage as the text after the LAST '/', *before* any $$
+    # expansion, so a selector containing a path is truncated and the job holds at the
+    # execute point ("Unable to download or build singularity image ...sif\") ])",
+    # observed live on an OSPool glidein).  With no '/' the token survives intact and
+    # the schedd expands it at match time.
     assert expr.startswith("$$([ ") and expr.endswith(" ])")
+    assert "/" not in expr                                        # THE invariant
     assert "=?= undefined" not in expr                            # not a guess-guard
     assert "ifThenElse(TARGET.GPUs_Capability >= 7.0," in expr
-    assert '"osdf:///igwn/rift_modern_cuda12.sif"' in expr         # raw osdf URL
-    assert '"/cvmfs/sw/rift_ancient_cuda11.sif"' in expr           # fallback verbatim
-    assert "./rift_modern_cuda12.sif" not in expr                  # no basename rewrite
+    assert '"rift_modern_cuda12.sif"' in expr                     # basename branch
+    assert '"rift_ancient_cuda11.sif"' in expr                    # fallback basename
+
+
+def test_container_image_select_rejects_in_place_images(tmp_path):
+    # An in-place (CVMFS/local) image can only be named by its full path, which would
+    # reintroduce the '/' truncation.  Refuse loudly rather than emit a submit file
+    # that holds every job.
+    m = cm.load_container_manifest(_write(tmp_path, MIXED_MANIFEST))
+    with pytest.raises(cm.ContainerManifestError) as exc:
+        cm.build_container_image_select(m)
+    assert "ancient" in str(exc.value)
+    # ... but the CPU-only single-image path is unaffected: it is a plain literal that
+    # condor_submit handles correctly.
+    assert cm.build_container_image_select(m, request_gpu=False) == "/cvmfs/sw/rift_ancient_cuda11.sif"
 
 
 def test_integration_container_universe(tmp_path, monkeypatch):
@@ -285,7 +318,7 @@ def test_integration_container_universe(tmp_path, monkeypatch):
         arg_str="--foo bar",
         transfer_files=["../all.net"],
         use_singularity=True,
-        singularity_image=_write(tmp_path, MIXED_MANIFEST),
+        singularity_image=_write(tmp_path, ALL_OSDF_MANIFEST),
         request_gpu=True,
         cache_file="local.cache",
     )
@@ -294,9 +327,19 @@ def test_integration_container_universe(tmp_path, monkeypatch):
     ci = cmds["container_image"]
     assert ci.startswith("$$([")               # match-time substitution, unquoted
     assert not ci.startswith('"')
+    assert "/" not in ci                       # else condor_submit truncates it
     assert "MY.SingularityImage" not in cmds    # the OSG-breaking attr is gone
     assert "MY.SingularityBindCVMFS" not in cmds
-    assert "$$([" not in cmds.get("transfer_input_files", "")  # image via container_image, not transfer
+
+    # container_image names only a basename, so the image must arrive by transfer:
+    # exactly one comma-free $$() token carrying the full URLs.
+    tif = cmds["transfer_input_files"]
+    assert tif.count("$$([") == 1
+    assert "osdf:///igwn/sw/rift_modern_cuda12.sif" in tif
+    # ... and TransferInput is pinned, so condor_submit does not append the basename
+    # selector to it as a bogus extra input file.
+    assert cmds["MY.TransferInput"] == '"' + tif.replace('"', '\\"') + '"'
+
     assert "Capability >= 3.0" in cmds["require_gpus"]         # floor still steers GPUs
     # GPU family job: still excludes slots that don't advertise the capability attr
     assert "TARGET.GPUs_Capability =!= undefined" in cmds["requirements"]

containers/README.md

@@ -121,9 +121,9 @@ example. Schema:
 For the ILE (and CIP) Condor submit, a manifest produces:
 
 - **`MY.SingularityImage`** — an *unquoted* `ifThenElse(...)` expression that
-  selects the highest-capability image the matched machine can run, defaulting to
-  the `fallback` image (also used when the capability attribute is `undefined`,
-  e.g. on a CPU-only CIP slot — hence the fallback must be CPU-safe):
+  selects the highest-capability image the matched machine can run, with the
+  `fallback` image as the innermost `else` (used when the machine's capability is
+  below every threshold):
 
   ```
   ifThenElse(TARGET.GPUs_Capability >= 8.0, "./rift_container_modern.sif", "/cvmfs/.../rift_container_default.sif")
@@ -145,6 +145,59 @@ For the ILE (and CIP) Condor submit, a manifest produces:
   composed (`&&`) with any user-supplied `RIFT_REQUIRE_GPUS` (which today you use
   to block incompatible hosts by `DeviceName`). Both apply; neither is dropped.
 
+### OSG: pick a delivery mode
+
+The expression-valued `MY.SingularityImage` is evaluated *execute-side*. OSPool
+glidein pilots read `SingularityImage` as a **literal string**, so an
+`ifThenElse` lands verbatim and the job holds. Two opt-in modes fix this,
+selected by an environment variable at DAG-build time:
+
+| env var | behaviour |
+|---|---|
+| *(unset)* | legacy `universe = vanilla` + expression-valued `MY.SingularityImage`. Correct on a local/CIT pool; **not OSG-safe**. |
+| `RIFT_CONTAINER_UNIVERSE=1` | **recommended for OSG.** `universe = container` + `container_image = $$([ ifThenElse(...) ])` over image BASENAMES. `$$()` is HTCondor's match-time (schedd-side) machine-ad substitution, so the pilot only ever sees a literal image name. No `MY.SingularityImage`, no `MY.SingularityBindCVMFS`; the matched image arrives via the `$$()` transfer token with `MY.TransferInput` pinned (see below). Requires every family image to be a transferable URL. GPU access is automatic under `request_gpus`. Works on CIT-local too. |
+| `RIFT_CONTAINER_RUNTIME_SELECT=1` | older ILE-only fallback: Condor runs a generated `rift_container_select.sh` on the bare node, which reads the real capability from `nvidia-smi`, fetches only the matching image (`stashcp`/`pelican`) and re-execs under `apptainer exec --nv`. |
+
+Under asimov set it from the blueprint, not the shell:
+
+```yaml
+scheduler:
+  singularity image: /path/to/rift_container_family.yaml
+  singularity base exe directory: /usr/local/bin/
+  environment variables:
+    RIFT_CONTAINER_UNIVERSE: 1
+```
+
+With `osdf://` images inside a manifest, the pipeline also enables the matching
+transfer credential automatically (`use_oauth_services = scitokens`, or `igwn`
+for `igwn+osdf:`) by inspecting the manifest's image URLs — the single-image path
+keys off the `SINGULARITY_RIFT_IMAGE` string, which for a family is only a
+`.yaml` path.
+
+> **Why the container-universe selector names basenames, not URLs.**
+> `condor_submit` parses `container_image` *before* any `$$` expansion and derives
+> the job ad's `ContainerImage` -- the name the image gets in the job scratch dir --
+> as the text after the **last** `/`. A selector containing full paths is cut in
+> half, and the fragment that survives is not a valid image name. Submitting that
+> form to the IGWN pool holds the job at the execute point:
+> `PREPARE_JOB (prepare-hook) failed: Unable to download or build singularity image
+> cutest_busybox_...sif") ])`.
+>
+> So the selector emits **basenames only** (no `/`); the whole `$$` token survives
+> into `ContainerImage` and the schedd expands it at match time
+> (`MATCH_EXP_ContainerImage = "rift_container_modern.sif"`). The image itself
+> arrives via the comma-free `$$()` transfer token, and `MY.TransferInput` is pinned
+> so `condor_submit` does not append the basename selector to `TransferInput` as a
+> bogus extra input file. Verified end to end on an OSPool glidein against
+> `$CondorVersion: 25.11.1`.
+>
+> Consequence: **every image in a family used with container universe must be a
+> transferable URL.** An in-place (CVMFS/local) image can only be named by its full
+> path, which reintroduces the truncation, so `build_container_image_select()`
+> raises `ContainerManifestError` for such a family. Stage those images at a URL, or
+> use `RIFT_CONTAINER_RUNTIME_SELECT=1`.
+
+
 ### HTCondor GPU attribute names — important
 
 Two different namespaces are in play and are kept separate: