DM-54466: Fix bug that converts s3:// to s3:/.

doc/changes/DM-54466.bugfix.rst

@@ -0,0 +1 @@
+Fixed bug that converts s3:// to s3:/.  Changed code to only remove double slashes with subDirTemplate since we are guaranteed it is used as part of a path and doesn't include a scheme.

python/lsst/ctrl/bps/bps_config.py

@@ -37,7 +37,7 @@
 import os
 import re
 import string
-from os.path import expandvars
+from os.path import expandvars, normpath
 from typing import Any
 
 from lsst.daf.butler import Config
@@ -366,6 +366,10 @@ def search(self, key, opt=None):
 
         _LOG.debug("opt=%s %s", opt, type(opt))
         if found and isinstance(value, str):
+            if key == "subDirTemplate":
+                # Save if template ends with slash
+                template_endswith_slash = value.endswith("/")
+
             if opt.get("expandEnvVars", True):
                 _LOG.debug("before format=%s", value)
                 value = re.sub(r"<ENV:([^>]+)>", r"$\1", value)
@@ -378,6 +382,15 @@ def search(self, key, opt=None):
 
             if opt.get("replaceVars", True):
                 value = self.replace_vars(value, opt)
+                if key == "subDirTemplate":
+                    # Make yaml-specified subdirs easier to read
+                    # by removing empty subdirs (//).  normpath
+                    # removes any trailing slash.
+                    value = normpath(value)
+                    # Check if subDirTemplate pattern actually ends in slash
+                    # If so, the value returned should.
+                    if template_endswith_slash:
+                        value += "/"
 
             _LOG.debug("after format=%s", value)
 
@@ -431,9 +444,6 @@ def replace_vars(self, value: str, opt: dict[str, Any]) -> str:
         if "bpsEval" in value:
             raise ValueError(f"Unparsable bpsEval in '{value}'")
 
-        # Make yaml-specified paths easier to read, remove empty subdirs (//)
-        value = re.sub(r"//+", "/", value)
-
         return value
 
     def generate_config(self) -> None:

python/lsst/ctrl/bps/transform.py

@@ -265,9 +265,6 @@ def _enhance_command(config, generic_workflow, gwjob, cached_job_values):
         r"{wms([^}]+)}", lambda x: f"<WMS:{x[1][0].lower() + x[1][1:]}>", gwjob.arguments
     )
 
-    # To make yaml-specified paths easier to read, remove empty subdirs (//)
-    gwjob.arguments = re.sub(r"//+", "/", gwjob.arguments)
-
     # Save dict of other values needed to complete command line.
     # (Be careful to not replace env variables as they may
     # be different in compute job.)

tests/test_bpsconfig.py

@@ -296,6 +296,87 @@ def testRequired(self):
         with self.assertRaises(KeyError):
             self.config.search("fred", opt={"required": True})
 
+    def testS3Path(self):
+        """Test that double slashes aren't replaced with single slash."""
+        config = BpsConfig(self.config)
+        s3 = "s3://user1@rubin-place-users/butler-pipeline1-processing.yaml"
+        config["butlerConfig"] = s3
+        test_opt = {"expandEnvVars": True, "replaceEnvVars": True, "replaceVars": True}
+        found, value = config.search("butlerConfig", opt=test_opt)
+        self.assertEqual(found, True)
+        self.assertEqual(value, s3)
+
+    def testSubDirTemplate(self):
+        """Test that double slashes are replaced with single slash
+        in subDirTemplate.
+        """
+        config = BpsConfig(self.config)
+
+        # template doesn't end in slash, post-slashes value didn't end in slash
+        config["subDirTemplate"] = "{label}/{val1}/{val2}/{val3}/{val4}/{val5}"
+        test_opt = {
+            "expandEnvVars": True,
+            "replaceEnvVars": True,
+            "replaceVars": True,
+            "curvals": {"label": "label1", "val5": 12345},
+        }
+        found, value = config.search("subDirTemplate", opt=test_opt)
+        self.assertEqual(found, True)
+        self.assertEqual(value, "label1/12345")
+
+        # template doesn't end in slash, post-slashes value ended in slash
+        config["subDirTemplate"] = "{label}/{val1}/{val2}/{val3}/{val4}/{val5}"
+        test_opt = {
+            "expandEnvVars": True,
+            "replaceEnvVars": True,
+            "replaceVars": True,
+            "curvals": {"label": "label2", "val4": 12345},
+        }
+        found, value = config.search("subDirTemplate", opt=test_opt)
+        self.assertEqual(found, True)
+        self.assertEqual(value, "label2/12345")
+
+        # template ends in slash, post-slashes value didn't end in slash
+        config["subDirTemplate"] = "{label}/{val1}/{val2}/{val3}/{val4}/{val5}/"
+        test_opt = {
+            "expandEnvVars": True,
+            "replaceEnvVars": True,
+            "replaceVars": True,
+            "curvals": {"label": "label3", "val4": 12345},
+        }
+        found, value = config.search("subDirTemplate", opt=test_opt)
+        self.assertEqual(found, True)
+        self.assertEqual(value, "label3/12345/")
+
+        # template ends in slash, post-slashes value ended in slash
+        config["subDirTemplate"] = "{label}/{val1}/{val2}/{val3}/{val4}/{val5}/"
+        test_opt = {
+            "expandEnvVars": True,
+            "replaceEnvVars": True,
+            "replaceVars": True,
+            "curvals": {"label": "label4", "val2": 12345},
+        }
+        found, value = config.search("subDirTemplate", opt=test_opt)
+        self.assertEqual(found, True)
+        self.assertEqual(value, "label4/12345/")
+
+    def testOtherTemplate(self):
+        """Test that other subDirTemplate-like value doesn't have
+        double slashes replaced.
+        """
+        config = BpsConfig(self.config)
+
+        config["otherTemplate"] = "{label}/{val1}/{val2}/{val3}/{val4}/{val5}"
+        test_opt = {
+            "expandEnvVars": True,
+            "replaceEnvVars": True,
+            "replaceVars": True,
+            "curvals": {"label": "label1", "val4": 12345, "val5": ""},
+        }
+        found, value = config.search("otherTemplate", opt=test_opt)
+        self.assertEqual(found, True)
+        self.assertEqual(value, "label1////12345/")
+
 
 class TestBpsConfigGenerateConfig(unittest.TestCase):
     """Test BpsConfig.generate_config and bpsEval methods."""

tests/test_transform.py

@@ -395,6 +395,16 @@ def testKeyCachedCmdVal(self):
         _enhance_command(self.config, gw, gwjob, self.cached_vals)
         self.assertEqual(gwjob.cmdvals["key1"], "val1")
 
+    def testS3Argument(self):
+        """Make sure s3 double slashes are not getting removed."""
+        gwjob = GenericWorkflowJob("job1", "label1", executable=self.gw_exec)
+        gw = GenericWorkflow("test1")
+        gw.add_job(gwjob)
+        s3 = "s3://user1@rubin-place-users/butler-pipeline1-processing.yaml"
+        gwjob.arguments = s3
+        _enhance_command(self.config, gw, gwjob, {})
+        self.assertEqual(gwjob.arguments, s3)
+
 
 if __name__ == "__main__":
     unittest.main()