From ab191b46e005b212cb02c93ba71da519a6805058 Mon Sep 17 00:00:00 2001 From: Michelle Gower Date: Mon, 23 Mar 2026 21:29:03 -0500 Subject: [PATCH] Fix bug that converts s3:// to s3:/. --- doc/changes/DM-54466.bugfix.rst | 1 + python/lsst/ctrl/bps/bps_config.py | 18 +++++-- python/lsst/ctrl/bps/transform.py | 3 -- tests/test_bpsconfig.py | 81 ++++++++++++++++++++++++++++++ tests/test_transform.py | 10 ++++ 5 files changed, 106 insertions(+), 7 deletions(-) create mode 100644 doc/changes/DM-54466.bugfix.rst diff --git a/doc/changes/DM-54466.bugfix.rst b/doc/changes/DM-54466.bugfix.rst new file mode 100644 index 00000000..b3aa5fe5 --- /dev/null +++ b/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. diff --git a/python/lsst/ctrl/bps/bps_config.py b/python/lsst/ctrl/bps/bps_config.py index 30036b64..c2c561f0 100644 --- a/python/lsst/ctrl/bps/bps_config.py +++ b/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"]+)>", 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: diff --git a/python/lsst/ctrl/bps/transform.py b/python/lsst/ctrl/bps/transform.py index a04080e2..ea4e075e 100644 --- a/python/lsst/ctrl/bps/transform.py +++ b/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"", 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.) diff --git a/tests/test_bpsconfig.py b/tests/test_bpsconfig.py index 69baa83b..7eb69ac6 100644 --- a/tests/test_bpsconfig.py +++ b/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.""" diff --git a/tests/test_transform.py b/tests/test_transform.py index f62e5903..5d7d5cd9 100644 --- a/tests/test_transform.py +++ b/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()