Python interface: various PR bugs caught by Copilot

Author: PetrilloAtWork

sbnalg/gallery/python/LArSoftUtils.py

@@ -226,7 +226,7 @@ def serviceClassToConfigKeys(serviceClassName: str) -> list[str]:
   candidates = []
 
   # remove namespaces (namespaces are not part of any candidate)
-  try: serviceClassName = serviceClassName[:serviceClassName.index('::')]
+  try: serviceClassName = serviceClassName[serviceClassName.rindex('::')+2:]
   except ValueError: pass # no namespace
 
   if not serviceClassName: return [] # ?!
@@ -284,10 +284,10 @@ def readServiceConfig(getConfig, configKey, returnConfigKey = True):
     = tuple(base + suffix for base in serviceClassToConfigKeys(configKey))
 
   Logger.debug("Configuration from candidates: '%s'", "', '".join(configKeys))
-  for configKey in configKeys:
-    try: config = getConfig(configKey)
+  for candidateKey in configKeys:
+    try: config = getConfig(candidateKey)
     except Exception: continue
-    return (config, configKey) if returnConfigKey else config
+    return (config, candidateKey) if returnConfigKey else config
   raise RuntimeError(f"No configuration for service key '{configKey}'")
 # readServiceConfig()
 
@@ -319,6 +319,7 @@ def loadSimpleService \
       config = readServiceConfig(
         getConfig=(config.service if config else registry.config),
         configKey=serviceName,
+        returnConfigKey=False,
         )
     except RuntimeError:
       Logger.debug("Full configuration:\n%s",
@@ -400,7 +401,10 @@ def __init__(self,
   def _needsSpecialConfig(self): return self.addConfig or self.purgeConfig
 
   def _makeConfig(self, registry):
-    """See the constructor for details on the configuration key algorithm."""
+    """Fetches, finalizes and returns the FHiCL configuration for the service.
+    
+    See the constructor for details on the configuration key algorithm.
+    """
 
     configKey = self.configKey
     if not configKey or configKey.startswith('.'):
@@ -471,16 +475,15 @@ def _loadService(self, manager, dependencies):
     The dependencies are expected to be a dictionary: provider name → provider object
     (`None` if not available).
     """
-    
-    # if we don't need a special configuration,
-    # we let loadSimpleService() find it
+   
+    # put together the configuration of the service being loaded
     registry = manager.registry()
     config = self._makeConfig(registry)
     Logger.debug("Service configuration:\n%s", config.to_indented_string())
 
     self._loadCode()
 
-    # loads the actual classes from ROOT
+    # load the actual classes from ROOT
     self.expandClass('serviceClass')
     if self.interfaceClass is not None: self.expandClass('interfaceClass')
 
@@ -701,6 +704,8 @@ def fullConfig(self): return self.serviceTable is None
     def isValid(self): return self.configPath is not None
     def hasExtraConfig(self): return bool(self.extraConfig)
     def needsCustom(self): return not self.fullConfig() or self.hasExtraConfig()
+    def serviceTableName(self):
+      return 'services' if self.fullConfig() else self.serviceTable
 
     def addExtraConfig(self, extra): self.extraConfig += "\n" + extra
 
@@ -743,7 +748,15 @@ def get(self, serviceKey, interfaceClass = None):
   # get()
 
 
-  def defaultConfiguration(self): return None
+  def defaultConfiguration(self):
+    """Returns the default configuration.
+
+    The configuration is delivered as a ServiceManagerInstance.ConfigurationInfo object.
+    This configuration is used when the service manager is not explicitly
+    configured with `setConfiguration()`.
+    """
+    return ServiceManagerInstance.ConfigurationInfo()
+  # defaultConfiguration()
 
   def setConfiguration(self, configFile, serviceTable = None, extra = ""):
     """Sets which configuration to use for setup.
@@ -787,13 +800,18 @@ def loadConfiguration(self):
       else self.defaultConfiguration()
 
     # if assertion fails, then `setConfiguration()` was not correctly called.
-    assert configurationInfo.isValid()
+    if not configurationInfo or not configurationInfo.isValid():
+      raise RuntimeError(
+        "No configuration in place at setup time. Call 'setConfiguration()'"
+        " or override 'defaultConfiguration()' to provide a configuration file path."
+        )
+    # if
 
     if configurationInfo.needsCustom():
       Logger.debug(
         "Using service table '%s' from configuration file '%s' plus %d custom lines",
-        configurationInfo.serviceTable, configurationInfo.configPath,
-        sum(c == '\n' for c in configurationInfo.extraConfig),
+        configurationInfo.serviceTableName(), configurationInfo.configPath,
+        sum(c == '\n' for c in configurationInfo.extraConfig) + 1,
         )
       config = galleryUtils.ConfigurationString(
           '#include "{configPath}"'
@@ -811,7 +829,7 @@ def loadConfiguration(self):
         '\n# ==============================='
         .format(
           configPath=configurationInfo.configPath,
-          serviceTable=configurationInfo.serviceTable,
+          serviceTable=configurationInfo.serviceTableName(),
           extraConfig=configurationInfo.extraConfig,
           )
         )

sbnalg/gallery/python/ROOTutils.py

@@ -262,14 +262,14 @@ def getROOTclass(classPath):
 ################################################################################
 # this is not really specific to ROOT, but we often have ROOT file lists
 def expandFileList(
- fileListPaths: "path (or paths) of the file list (or single file)",
+ dataPaths: "path (or paths) of the file list (or single file)",
  comment: "(default: '#') character used to introduce a comment" = '#',
- fileListSuffixes: "suffix of entries to recursively add file lists" = [],
- fileSuffixes: "suffix of entries never to be treated as file lists" = [ '.root' ],
+ fileListSuffixes: "suffix of entries to recursively add file lists" = (),
+ fileSuffixes: "suffix of entries never to be treated as file lists" = ( '.root', ),
  ) -> "a list of file names":
   """Returns a list of file names as found in the specified file lists.
   
-  The `fileListPaths` paths are read as text files; in them, each line
+  The `dataPaths` paths are read as text files; in them, each line
   represents a full file path.
   Empty lines and lines starting with a comment character are ignored.
   Also if blanks and a comment character are found, the content of the line
@@ -281,13 +281,16 @@ def expandFileList(
   If file suffixes are specified, a line ending with any of those suffixes
   will be considered a file, and not expanded. That takes priority over the file
   list suffix.
+  Only absolute paths are supported in file lists. The current behaviour,
+  which does not reject relative paths and assumes them relative to the current
+  directory, is not guaranteed and may change in the future (ideally, it will).
   
-  If any of the file lists in `fileListPaths` can't be read, an exception is
+  If any of the file lists in `dataPaths` can't be read, an exception is
   raised.
   
-  NOTE: because of the internal implementation, a `fileListPaths` that is a
+  NOTE: because of the internal implementation, a `dataPaths` that is a
     collection of file names that are all one character long may be mistaken
-    for a string. The general solution is: do not name your file not file lists
+    for a string. The general solution is: do not name your file nor file lists
     with a single-character name. It's most often a bad idea anyway.
   """
 
@@ -297,48 +300,65 @@ def expandFileList(
     )
 
   # ----------------------------------------------------------------------------
-  # Path or list?
+  # Path or collection of paths?
   #
-  # This looks very silly, but distinguishing a string and a list is not trivial
-  # (even less so in Python); and we need to support both std::vector and list,
-  # both str and std::string.
+  # This looks very silly, but distinguishing a string and a collection of
+  # strings is not trivial (even less so in Python); and we need to support both
+  # std::vector and list, both str and std::string.
   # The elements of all these types are of Python type str.
   # So it is hereby decided that if they all are of length 1, then it's a string
-  # otherwise it is a list. Yes, this fails in an obvious corner case.
-  # Just don't call your file lists "a", "b" etc. Nor your files.
+  # otherwise it is a collection. Yes, this fails in an obvious corner case.
+  # Just don't call your files or file lists "a", "b" etc.
   # Nevertheless, some special cases are singled out and specifically addressed.
 
-  isClearlyList = isinstance(fileListPaths, (ROOT.std.vector[ROOT.std.string], list, set))
-  isClearlyPath = isinstance(fileListPaths, (ROOT.std.string, str))
+  isClearlyColl = isinstance(dataPaths, (ROOT.std.vector[ROOT.std.string], list, tuple, set))
+  isClearlyPath = isinstance(dataPaths, (ROOT.std.string, str))
   # we expand the argument to support single-pass generators; what happens?
   #  str              -> list[str] (one character each element)
   #  std::string      -> [str ->] list[str] (one character each element)
   #  Python iterable  -> list (usually?)
   #  Python generator -> list
-  fileListList = list(fileListPaths)
-  if isClearlyList or (any(len(p) > 1 for p in fileListList) and not isClearlyPath):
+  dataPathList = list(dataPaths)
+  if isClearlyColl or (any(len(p) > 1 for p in dataPathList) and not isClearlyPath):
     Logger.debug("Input is a list of %d paths and will be expanded as such.",
-      len(fileListList))
-    return sum([ expandAgain(path) for path in fileListList ], [])
+      len(dataPathList))
+    return sum([ expandAgain(path) for path in dataPathList ], [])
   # if the argument is a list
 
-  # at this point we believe the original argument was a string (not a list,
-  # not a generator) and we broke it into pieces into `fileListList`.
-  # But `fileListPath` is still whole, so we will use it.
+  # at this point we believe the original argument was a string (not a
+  # collection nor generator) and we broke it into pieces into `dataPaths`.
+  # But `dataPaths` is still whole, so we will use it.
 
   # ----------------------------------------------------------------------------
   # Expand, at last
   #
-  # from here on, it's only one file or file list
-  fileListPath = fileListPaths
+  # from here on, it's only a single path (file or file list)
+  # 
+  # NOTE on relative paths: in principle we can expand relative paths prepending
+  #   the base path of the parent file list (if any; otherwise we can either
+  #   append the current directory, or leave them relative).
+  #   One complication is that paths may not be easy to identify as relative;
+  #   for example, a XRootD URL is considered by `os.path.isabs()` as relative.
+  #   Here `pathlib` module may help. Another complication is if a base path
+  #   includes symbolic links. Imagine an `output` directory structure with
+  #   `output/data/` and `output/lists/`, where the items of the file lists in
+  #   the latter all include a `../data` path. While that list works well when
+  #   expanded using its real path (`output/lists/../data/...`), when such list
+  #   is linked somewhere else, the simple expansion of those files to
+  #   `<current dir>/../data/...` will be broken. In addition, the use of
+  #   file-system-accessing functions as `os.path.realpath()` may still break
+  #   since those functions won't work paths like XRootD URL.
+  #   All of this can be worked around, with enough motivation.
+  # 
+  dataPath = dataPaths
   hasSuffix = lambda s, suffixes: any(s.endswith(sx) for sx in suffixes)
 
-  if hasSuffix(fileListPath, fileSuffixes):
-    Logger.debug("'%s' was for sure a file, not a file list.", fileListPath)
-    return [ fileListPath ]
+  if hasSuffix(dataPath, fileSuffixes):
+    Logger.debug("'%s' was for sure a file, not a file list.", dataPath)
+    return [ dataPath ]
 
-  Logger.debug("Processing file list '%s'", fileListPath)
-  with open(fileListPath, 'r') as fileList:
+  Logger.debug("Processing file list '%s'", dataPath)
+  with open(dataPath, 'r') as fileList:
 
     l = []
     for iLine, line in enumerate(fileList):

sbnalg/gallery/python/cppUtils.py

@@ -145,12 +145,13 @@ def loadMany(self, *pathSpecs, extraPaths = [], force = False, loadAll = False):
     """Calls `load()` for each of the path specifications.
     
     A path specification is a dictionary of `load()` parameters, e.g.
-    `{ relPath: 'larcorealg_Geometry', extraPaths: [] }`.
+    `dict(relPath='larcorealg_Geometry', extraPaths=[])`.
     If a path specification is not a dictionary, it is assumed to be a `relPath`
-    and it is equivalent to having `{ relPath: pathSpec }`.
-    If an argument `extraPath` or `force` is specified in the path
-    specification, a value is used from the `extraPath` and `force` arguments
-    of this function.
+    and it is equivalent to having `dict(relPath=pathSpec)`.
+    If `extraPaths` or `force` are not specified in a path specification,
+    they default to the `extraPaths` and `force` arguments of this function;
+    if they are specified in the path specification, their values override
+    the defaults from this function.
     
     If `loadAll` is set, all loads are attempted; the return value is a pair:
     whether an exception was thrown, and a list of all return values or

sbnalg/gallery/python/galleryUtils.py

@@ -289,11 +289,8 @@ def eventLoop(inputFiles,
   nSkip = options.get('nSkip', 0)
   nEvents = options.get('nEvents', None)
 
-  # make sure the input file list is in the right format
-  if not isinstance(inputFiles, ROOT.vector(ROOT.string)):
-    if isinstance(inputFiles, str): inputFiles = [ inputFiles, ]
-    inputFiles = makeFileList(*inputFiles)
-  # if
+  # create a gallery event with an expanded (flattened) input list
+  inputFiles = makeFileList(inputFiles)
 
   event = ROOT.gallery.Event(inputFiles)
 
@@ -533,7 +530,7 @@ def init(self, config, applName):
     if not applName: applName = os.path.basename(sys.argv[0])
     if not applName: applName = "this application"
     if isinstance(config, ConfigurationClass): config = config.service("message")
-    Logger.debug("Starting message facility for {%s}...", applName)
+    Logger.debug("Starting message facility for %s...", applName)
     try:
       ROOT.mf.StartMessageFacility(config, applName)
     except ROOT.std.exception: