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,7 +800,12 @@ 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(
@@ -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

@@ -287,7 +287,7 @@ def expandFileList(
   
   NOTE: because of the internal implementation, a `fileListPaths` 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.
   """
 
@@ -308,7 +308,7 @@ def expandFileList(
   # Just don't call your file lists "a", "b" etc. Nor your files.
   # Nevertheless, some special cases are singled out and specifically addressed.
 
-  isClearlyList = isinstance(fileListPaths, (ROOT.std.vector[ROOT.std.string], list, set))
+  isClearlyList = isinstance(fileListPaths, (ROOT.std.vector[ROOT.std.string], list, tuple, set))
   isClearlyPath = isinstance(fileListPaths, (ROOT.std.string, str))
   # we expand the argument to support single-pass generators; what happens?
   #  str              -> list[str] (one character each element)

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)