From 4e77a5422907d5b5e10576d8ddc26cb7e683a4ea Mon Sep 17 00:00:00 2001 From: Gianluca Petrillo Date: Wed, 17 Dec 2025 01:51:26 -0600 Subject: [PATCH 1/7] Simplified galleryUtils.makeFileList() Improvements in cppyy allow to use Python list[str] for C++ vector parameters. --- sbnalg/gallery/python/galleryUtils.py | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/sbnalg/gallery/python/galleryUtils.py b/sbnalg/gallery/python/galleryUtils.py index 0ecf482..d6f0461 100644 --- a/sbnalg/gallery/python/galleryUtils.py +++ b/sbnalg/gallery/python/galleryUtils.py @@ -151,7 +151,7 @@ def __call__(self, def makeFileList( *filePaths: "a list of input files" - ) -> "a list of files suitable to construct a `gallery.Event object": + ) -> "a list of files suitable to construct a `gallery.Event` object": """Creates a file list suitable for `gallery::Event`. If a file ends with `.root`, it is added directly to the list. @@ -159,12 +159,7 @@ def makeFileList( (see `ROOTutils.expandFileList()`). File list recursion is disabled. """ - files = ROOT.vector(ROOT.string)() - for path in filePaths: - entries = [ path ] if path.endswith('.root') else expandFileList(path) - for entry in entries: files.push_back(entry) - # for - return files + return sum((( [ path ] if path.endswith('.root') else expandFileList(path) ) for path in filePaths), []) # makeFileList() From c7031fa3093e51e595e893f68dde59c2d844e7d5 Mon Sep 17 00:00:00 2001 From: Gianluca Petrillo Date: Fri, 6 Mar 2026 18:07:46 -0600 Subject: [PATCH 2/7] cppUtils.py: SourceCode.loadMany() to load headers and libraries at once Also moved from print() to logging module. --- sbnalg/gallery/python/cppUtils.py | 64 +++++++++++++++++++++++++------ 1 file changed, 53 insertions(+), 11 deletions(-) diff --git a/sbnalg/gallery/python/cppUtils.py b/sbnalg/gallery/python/cppUtils.py index 7ffc0fc..f605a92 100644 --- a/sbnalg/gallery/python/cppUtils.py +++ b/sbnalg/gallery/python/cppUtils.py @@ -1,7 +1,5 @@ #!/usr/bin/env python -from __future__ import print_function - __doc__ = """ Collection of utilities to interface C++ code with Python via PyROOT. @@ -13,7 +11,7 @@ 'SourceCode', ] -import sys, os +import sys, os, logging from ROOTutils import ROOT # Make sure is not included in the range v3 library @@ -56,12 +54,9 @@ def __init__(self, *includePaths): def addIncPath(self, path, force=False): expPath = os.path.expandvars(path) if not os.path.isdir(expPath): - print( - "Warning: include path '%s'" % path, - (" ( => '%s')" % expPath if path != expPath else ""), - " does not exist.", - sep='', - file=sys.stderr + Logger.warning( + "Include path '%s'%s does not exist.", + path, (" ( => '%s')" % expPath if path != expPath else ""), ) if force or expPath not in self.includePaths: self.includePaths.append(expPath) @@ -108,7 +103,8 @@ def loadHeader(self, headerRelPath, extraPaths = [], force = False): try: return self.headers[headerRelPath] except KeyError: pass headerPath = self.findHeader(headerRelPath, extraPaths=extraPaths) - if not headerPath: raise RuntimeError("Can't locate header file '%s'" % headerRelPath) + if not headerPath: + raise RuntimeError(f"Can't locate header file '{headerRelPath}'") readHeader(headerPath) self.headers[headerRelPath] = headerPath return headerPath @@ -145,6 +141,50 @@ def load(self, relPath, extraPaths = [], force = False): return (self.loadLibrary if self.isLibrary(relPath) else self.loadHeaderFromUPS)(relPath, extraPaths=extraPaths, force=force) # load() + 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: [] }`. + 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. + + 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 + exceptions, one per path specification. When an exception is thrown, a + triplet (like `sys.exc_info()`) is representing it in the return value. + + Otherwise, the loading is interrupted at the first exception, and that + exception is passed through. + """ + + # convert all specifications into dictionaries + pathSpecs = [ + dict(relPath=spec) if isinstance(spec, str) else spec + for spec in pathSpecs + ] + # set the default values + for pathSpec in pathSpecs: + pathSpec.setdefault('extraPaths', extraPaths) + pathSpec.setdefault('force', force) + # + hasExceptions = False + res = [] + for pathSpec in pathSpecs: + try: + res.append(self.load(**pathSpec)) + except: + if not loadAll: raise + res.append(sys.exc_info()) + hasExceptions = True + # for + return hasExceptions, res + # loadMany() + + def isLibrary(self, path): return os.path.splitext(path)[-1] in [ self.PlatformInfo['LibSuffix'], '' ] @@ -157,7 +197,7 @@ def expandLibraryName(self, name): @staticmethod def packageNameFromHeaderPath(headerPath): - return os.path.split(os.path.dirname(headerPath))[0] + return headerPath.split(os.sep)[0] @staticmethod def packageVarNameFromHeaderPath(varSuffix, headerPath): @@ -176,6 +216,8 @@ def LibraryPaths(): # global instance of source tracking class SourceCode = SourceCentral() +Logger = logging.getLogger(__name__) + # type for decorations class UnusedAttr: pass From 863b2110edb9d4d4552d8c1d7cf21290f26c93a0 Mon Sep 17 00:00:00 2001 From: Gianluca Petrillo Date: Fri, 6 Mar 2026 18:10:58 -0600 Subject: [PATCH 3/7] ROOTutils.py: revamped expandFileList() It now supports as argument a list or a single file. Also moved from print() to logging module. --- sbnalg/gallery/python/ROOTutils.py | 104 +++++++++++++++++++++++------ 1 file changed, 82 insertions(+), 22 deletions(-) diff --git a/sbnalg/gallery/python/ROOTutils.py b/sbnalg/gallery/python/ROOTutils.py index afea0ec..989cb48 100644 --- a/sbnalg/gallery/python/ROOTutils.py +++ b/sbnalg/gallery/python/ROOTutils.py @@ -13,6 +13,10 @@ "ROOT", ] +import logging + +Logger = logging.getLogger(__name__) + ################################################################################ ### ### Try to save the command line arguments from unconsiderate ROOT behaviour @@ -49,11 +53,11 @@ def ROOTloader(): or equivalent. """ - import sys, logging + import sys try: alreadyLoaded = 'gInterpreter' in dir(ROOT) except NameError: alreadyLoaded = False if alreadyLoaded: - logging.warning( + Logger.warning( "ROOT module was loaded before ROOTutils.py: command line arguments may be garbled" ) return sys.modules['ROOT'] @@ -62,14 +66,14 @@ def ROOTloader(): class EmptyArgs: def __enter__(self): self.args = sys.argv - logging.debug("Saving command line: %s", self.args) + Logger.debug("Saving command line: %s", self.args) sys.argv = sys.argv[0:1] - logging.debug( + Logger.debug( "Replaced command line %s with %s before loading ROOT module", self.args, sys.argv) def __exit__(self, exc_type, exc_value, traceback): sys.argv = self.args - logging.debug("Restored command line %s", sys.argv) + Logger.debug("Restored command line %s", sys.argv) # class EmptyArgs with EmptyArgs(): @@ -258,14 +262,15 @@ def getROOTclass(classPath): ################################################################################ # this is not really specific to ROOT, but we often have ROOT file lists def expandFileList( - fileListPath: "path of the file list", + fileListPaths: "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' ], ) -> "a list of file names": - """Returns a list of file names as found in the specified file list. + """Returns a list of file names as found in the specified file lists. - The `fileListPath` path is read as a text file; each line represents a full - file path. + The `fileListPaths` 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 from the first of those blank characters on is ignored as part of a comment. @@ -273,15 +278,69 @@ def expandFileList( If file list suffixes are specified, a line ending with any of those suffixes will be considered a file list itself, and recursively expanded. + 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. - If `fileListPath` can't be read, an exception is raised. - """ + If any of the file lists in `fileListPaths` can't be read, an exception is + raised. - import logging + 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 + with a single-character name. It's most often a bad idea anyway. + """ - l = [] + expandAgain = lambda path: expandFileList( + path, comment=comment, + fileListSuffixes=fileListSuffixes, fileSuffixes=fileSuffixes, + ) + + # ---------------------------------------------------------------------------- + # Path or list? + # + # 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. + # 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. + # 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)) + # 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): + 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 ], []) + # 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. + + # ---------------------------------------------------------------------------- + # Expand, at last + # + # from here on, it's only one file or file list + fileListPath = fileListPaths + 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 ] + + Logger.debug("Processing file list '%s'", fileListPath) with open(fileListPath, 'r') as fileList: + l = [] for iLine, line in enumerate(fileList): line = line.strip() if not line: continue @@ -292,23 +351,24 @@ def expandFileList( line = words[0] for left, right in zip(words[:-1], words[1:]): if left and left[-1].isspace(): - logging.debug("Comment starting at line %d between '%s' and '%s'", iLine, left, right) + Logger.debug("Comment starting at line %d between '%s' and '%s'", iLine, left, right) break line += comment + right # for line = line.rstrip() # if comment - for suffix in fileListSuffixes: - if not line.endswith(suffix): continue - logging.debug("Adding content of file list from line %d ('%s')", iLine, line) - extra = expandFileList(line, comment=comment, fileListSuffixes=fileListSuffixes) - logging.debug("%d entries collected under file list '%s'", len(extra), line) - l.extend(extra) - break + if hasSuffix(line, fileListSuffixes): + # NOTE in case of conflicting suffixes, a line that matches both + # list and file suffixes is first treated as a list here, and recursive + # expansion is attempted; but the recursive expansion will consider it + # a file and then return it unexpanded. + Logger.debug("Adding content of file list from line %d ('%s')", iLine, line) + l.extend((extra:= expandAgain(line))) + Logger.debug("%d entries collected under file list '%s'", len(extra), line) else: - logging.debug("Line %d added to the list", iLine) l.append(line) + Logger.debug("Line %d added to the list", iLine) # for suffix... else # for line in list From 772c0da8949d1e13392a1c39c7207a270609c25c Mon Sep 17 00:00:00 2001 From: Gianluca Petrillo Date: Fri, 6 Mar 2026 18:16:16 -0600 Subject: [PATCH 4/7] galleryUtils.py: workaround for skipping events in event loop I have observed an exception raised when trying to skip events. This commit implements a workaround catching that exception and using a different, likely slower method to skip. In addition: * makeFileList() now relies fully on ROOTutils.expandFileList() * also moved from print() to logging module --- sbnalg/gallery/python/galleryUtils.py | 41 +++++++++++++++++++-------- 1 file changed, 29 insertions(+), 12 deletions(-) diff --git a/sbnalg/gallery/python/galleryUtils.py b/sbnalg/gallery/python/galleryUtils.py index d6f0461..33d12a0 100644 --- a/sbnalg/gallery/python/galleryUtils.py +++ b/sbnalg/gallery/python/galleryUtils.py @@ -1,7 +1,5 @@ #!/usr/bin/env python -from __future__ import print_function - __doc__ = """ Collection of utilities to interface gallery with python. @@ -43,11 +41,13 @@ 'ConfigurationHelper', ] -import sys, os +import sys, os, logging from ROOTutils import ROOT, expandFileList import cppUtils import warnings +Logger = logging.getLogger(__name__) + ################################################################################ ### Pass-through @@ -157,9 +157,9 @@ def makeFileList( If a file ends with `.root`, it is added directly to the list. Otherwise, it is interpreted as a file list and treated as such (see `ROOTutils.expandFileList()`). - File list recursion is disabled. """ - return sum((( [ path ] if path.endswith('.root') else expandFileList(path) ) for path in filePaths), []) + return expandFileList(filePaths, + fileListSuffixes=[ '.list', '.filelist' ], fileSuffixes=[ '.root' ]) # makeFileList() @@ -210,10 +210,19 @@ def forEach( being skipped). """ if fromStart: - if skipEvents > 0: event.goToEntry(skipEvents) - else: event.toBegin() - elif skipEvents > 0: + if skipEvents > 0: + try: + event.goToEntry(skipEvents) + except ROOT.std.exception: + # encountered in v10_06_00_01p05 for no good reason; + # it will take care of this later + Logger.debug("event.goToEntry(%d) encountered a possible bug...", skipEvents) + else: skipEvents = 0 # no event left to be skipped + else: event.toBegin() + if skipEvents > 0: # may be from failure above too... + Logger.debug("Skipping %d events one by one...", skipEvents) for _ in range(skipEvents): event.next() # probably slower than goToEntry() + Logger.debug("Skipping finished") nEvents = 0 while (not event.atEnd() and ((maxEvents is None) or (nEvents < maxEvents))): yield event @@ -299,7 +308,7 @@ def eventLoop(inputFiles, if iFile != event.fileEntry(): iFile = event.fileEntry() - print("Opening: '%s'" % inputFiles[iFile]) + Logger.info("Opening: '%s'", inputFiles[iFile]) # if new file # event flow control @@ -319,7 +328,7 @@ def eventLoop(inputFiles, # for if nErrors > 0: - print("Encountered %d/%d errors." % (nErrors, nProcessedEvents),file=sys.stderr) + Logger.error("Encountered %d/%d errors.", nErrors, nProcessedEvents) return nErrors # eventLoop() @@ -524,8 +533,16 @@ 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") - print(f"Starting message facility for {applName}...") - ROOT.mf.StartMessageFacility(config, applName) + Logger.debug("Starting message facility for {%s}...", applName) + try: + ROOT.mf.StartMessageFacility(config, applName) + except ROOT.std.exception: + Logger.critical( + "The (possibly mute) exception below was raised" + " when setting up message facility service." + " A possible cause is a wrong service configuration." + ) + raise startMessageFacility.Init = True # init() # class startMessageFacility From 6bea1427a3a37395aaaf476a2249d02af564d487 Mon Sep 17 00:00:00 2001 From: Gianluca Petrillo Date: Fri, 6 Mar 2026 18:23:19 -0600 Subject: [PATCH 5/7] LArSoftUtils.py: support for ChannelStatusService Infrastructure to support services like ChannelStatusService has been added. That includes more flexible logic to guess the configuration key for a service and the option to tell the loader that the service provider is configured in a sub-table of the service configuration. In addition, error reporting should be improved a bit. Also moved from print() to logging module. --- sbnalg/gallery/python/LArSoftUtils.py | 305 ++++++++++++++++++++------ 1 file changed, 240 insertions(+), 65 deletions(-) diff --git a/sbnalg/gallery/python/LArSoftUtils.py b/sbnalg/gallery/python/LArSoftUtils.py index 6e64aad..de9c667 100644 --- a/sbnalg/gallery/python/LArSoftUtils.py +++ b/sbnalg/gallery/python/LArSoftUtils.py @@ -1,7 +1,5 @@ #!/usr/bin/env python -from __future__ import print_function - __doc__ = """ Collection of utilities to interface LArSoft with python and gallery. @@ -26,7 +24,7 @@ 'loadSimpleService', ] -import sys, os +import sys, os, logging import ROOTutils from ROOTutils import ROOT import galleryUtils @@ -37,6 +35,7 @@ ) from cppUtils import UnusedAttr +Logger = logging.getLogger(__name__) ################################################################################ ### Pass-through @@ -211,6 +210,88 @@ def loadAuxDetGeometry(config=None, registry=None, sorterClass=None, auxDetReado # loadAuxDetGeometry() +################################################################################ +def serviceClassToConfigKeys(serviceClassName: str) -> list[str]: + """Returns the configuration key bound to the specified service provider name. + + A list of candidates is returned, put together with some known patterns. + The candidates are in the order of decreasing likelihood. + + General rules: + * all namespaces are removed + * suffixes "Provider" and "Service" are attempted, after the version + with all suffixes removed + """ + + candidates = [] + + # remove namespaces (namespaces are not part of any candidate) + try: serviceClassName = serviceClassName[:serviceClassName.index('::')] + except ValueError: pass # no namespace + + if not serviceClassName: return [] # ?! + + # the unadulterated class name + candidates.append(serviceClassName) + + # remove all suffixes, recursively + baseName = serviceClassName + while True: + for suffix in ( 'Provider', 'Service' ): + if not baseName.endswith(suffix): continue + baseName = baseName[:-len(suffix)] + break # restart to remove more suffixes + else: break # no suffix left + # while + + # add only one suffix at a time + for suffix in ( '', 'Provider', 'Service' ): + candidates.append(baseName + suffix) + + return candidates + +# serviceClassToConfigKeys() + + +def readServiceConfig(getConfig, configKey, returnConfigKey = True): + """Returns the FHiCL parameter set for the specified configuration key. + + The callable `getConfig` is expected to take a configuration key path (string) + and to return a `fhicl.ParameterSet` object, or to raise an exception if the + key was not found. The configuration key is expected to be specified relative + to the service configuration block (typically the content of the `services` + table). + + The parameter `configKey` is a base pattern of where to find the + configuration. The first part of the path (before the first dot) is treated + as a class name and processed with `serviceClassToConfigKeys()`. + All the base configuration keys returned by that function are attempted, + each with the rest of the path (after the first dot) appended. + + The first configuration found is returned, and a `RuntimeError` exception is + raised if none is found. + + If `returnConfigKey` is set, a pair is returned with the configuration object + (`fhicl.ParameterSet`) and the key that was used to retrieve it. If it is + not set instead, only the configuration object is returned. + """ + try: + configKey, suffix = configKey.split('.', 1) + suffix = '.' + suffix + except ValueError: suffix = "" # no dot, no suffix + + configKeys \ + = tuple(base + suffix for base in serviceClassToConfigKeys(configKey)) + + Logger.debug("Configuration from candidates: '%s'", "', '".join(configKeys)) + for configKey in configKeys: + try: config = getConfig(configKey) + except Exception: continue + return (config, configKey) if returnConfigKey else config + raise RuntimeError(f"No configuration for service key '{configKey}'") +# readServiceConfig() + + ################################################################################ def loadSimpleService \ (serviceClass, config=None, registry=None, interfaceClass=None, args = []): @@ -232,19 +313,29 @@ def loadSimpleService \ """ serviceName = (interfaceClass if interfaceClass else serviceClass).__name__ - configKey = serviceName + "Service" if not isinstance(config, ROOT.fhicl.ParameterSet): - config = config.service(configKey) if config else registry.config(configKey) - if config is None: - raise RuntimeError("Failed to retrieve the configuration for %s service" % serviceName) - - TestSetupClass = ROOT.testing.ProviderSetupClass(serviceClass) - if TestSetupClass: + try: + config = getServiceConfig( + getConfig=(config.service if config else registry.config), + configKey=serviceName, + ) + except RuntimeError: + Logger.debug("Full configuration:\n%s", + (config if config else registry.fullConfig).config.to_indented_string()) + raise + # if config not ParameterSet + assert isinstance(config, ROOT.fhicl.ParameterSet), f"Config is '{str(type(config))}'" + + SourceCode.load("larcorealg/TestUtils/ProviderTestHelpers.h") + TestSetupClass = ROOT.testing.ProviderSetupClass[serviceClass] + if TestSetupClass: # really not sure how this check could fail... + Logger.debug("Using '%s' to set up '%s'", TestSetupClass, serviceName) + Logger.debug(" config: '%s'", config) serviceSetup = TestSetupClass.setup try: providers = [ serviceClass.providers_type(*args), ] except: - # we received arguments, called expected them to be used, we did not: + # we received arguments, caller expected them to be used, we did not: if args: raise # there must have been some problem providers = [] service = serviceSetup(config, *providers) @@ -270,10 +361,23 @@ class SimpleServiceLoader: So far a "simple" service is one that can be loaded with `loadSimpleService()` allowing service provider dependencies and simple configuration adaptions. + + Some parameters: + * `configKey`: it is the FHiCL path within the service table where the + configuration to be passed to the service class is stored. + If it's not empty and it does not start with a dot ("."), it's used + verbatim. + If empty, it is based on the `interfaceClass` name or, if none, from the + `serviceClass` name. A few versions are considered: verbatim, then with + 'Service' and 'Provider' suffixes removed, then that with appended + one of the suffixes 'Service' and 'Provider'. + If `configKey` starts with a dot (".") instead of being empty, it is + appended to all those base candidates. """ def __init__(self, serviceClass, interfaceClass = None, + configKey: str = "", headers = [], libraries = [], dependencies = [], @@ -283,26 +387,33 @@ def __init__(self, assert serviceClass is not None self.serviceClass = serviceClass self.interfaceClass = interfaceClass + self.serviceProviderClass = None + self.configKey = configKey self.headers = makeStringList(headers) self.libraries = makeStringList(libraries) self.serviceDependencies = makeStringList(dependencies) self.purgeConfig = makeStringList(purgeConfig) self.addConfig = addConfig + self.usedConfigKey = None # the key where the actual configuration is found # __init__() def _needsSpecialConfig(self): return self.addConfig or self.purgeConfig def _makeConfig(self, registry): - for configKey in ( self.serviceKey(), self.serviceKey() + "Service", ): - try: - config = registry.config(configKey) - break - except Exception: pass - else: config = None - if not config: - raise RuntimeError \ - ("No configuration for service '{}'".format(self.serviceKey())) - # if + """See the constructor for details on the configuration key algorithm.""" + + configKey = self.configKey + if not configKey or configKey.startswith('.'): + configKey = self.serviceKey() + configKey + try: + config, configKey \ + = readServiceConfig(getConfig=registry.config, configKey=configKey) + except RuntimeError: + Logger.debug("Full configuration when failed to get config for '%s':\n%s", + self.configKey, registry.fullConfig.config.to_indented_string()) + raise + self.usedConfigKey = configKey + for key in self.purgeConfig: config.erase(key) for key, value in self.addConfig.items(): config.put(key, str(value)) return config @@ -313,6 +424,22 @@ def serviceKey(self): if not isinstance(source, str): source = source.__name__ return source.replace('::', '.').split('.')[-1] # serviceKey() + + def readServiceProviderName(self, registry): + """Reads the `service_provider` key from the main configuration table + of this service. + + The complete service configuration table (with all services) is specified in + `config`. + + The function returns `None` if the service configuration hasn't been read + yet, and an empty string if there is no such key. + """ + if self.usedConfigKey is None: return None + assert registry + serviceKey = (self.usedConfigKey + ".").split('.', 1)[0] + return registry.config(serviceKey).get[str]('service_provider', "") + # readServiceProviderName() def load(self, manager): return self._loadService(manager, dependencies=self._prepareDependencies(manager)) @@ -333,10 +460,7 @@ def _prepareDependencies(self, manager): def _loadCode(self): # load the required headers and libraries - for header in self.headers: - galleryUtils.SourceCode.loadHeaderFromUPS(header) - for library in self.libraries: - galleryUtils.SourceCode.loadLibrary(library) + galleryUtils.SourceCode.loadMany(*self.headers, *self.libraries) # _loadCode() def _loadService(self, manager, dependencies): @@ -348,17 +472,26 @@ def _loadService(self, manager, dependencies): (`None` if not available). """ + # if we don't need a special configuration, + # we let loadSimpleService() find it + 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 self.expandClass('serviceClass') if self.interfaceClass is not None: self.expandClass('interfaceClass') - - # if we don't need a special configuration, - # we let loadSimpleService() find it - registry = manager.registry() - config = self._makeConfig(registry) if self._needsSpecialConfig() else None - + + # For a moment, here there has been a failed attempt to load a service + # provider, is specified. The problem is always the same: the "provider" + # specified in the configuration is also an _art_ service, which we don't + # load here. We would need to know which its service provider (in the + # LArSoft meaning) is, and only the service class can tell us. + # So the solution is to ask the user to specify the concrete provider class + # in the configuration of the service loader. + return loadSimpleService( self.serviceClass, config=config, registry=registry, interfaceClass=self.interfaceClass, @@ -508,7 +641,7 @@ def get(self, serviceKey, interfaceClass = None): except KeyError: loader = SimpleServiceLoader(serviceKey, interfaceClass=interfaceClass) - print("Loading service provider: '{}'".format(serviceKey)) + Logger.info("Loading service provider: '%s'", serviceKey) return loader(self) # get() @@ -558,16 +691,22 @@ class ServiceManagerInstance(ServiceManagerInterface): class ConfigurationInfo: - __slots__ = [ 'configPath', 'serviceTable' ] - def __init__(self, configPath = None, serviceTable = None): + """Record of the source of the configuration.""" + __slots__ = [ 'configPath', 'serviceTable', 'extraConfig' ] + def __init__(self, configPath = None, serviceTable = None, extra = ""): self.configPath = configPath self.serviceTable = serviceTable + self.extraConfig = extra 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 addExtraConfig(self, extra): self.extraConfig += "\n" + extra def __str__(self): if self.fullConfig(): return self.configPath - else: return "{{services={}}}@{}".format(self.serviceTable, self.configPath) + else: return f"{{services={self.serviceTable}}}@{self.configPath}" # __str__() # class ConfigurationInfo @@ -600,70 +739,106 @@ def get(self, serviceKey, interfaceClass = None): will be raised. """ if not self.manager: self.setup() - return self.manager(serviceKey) + return self.manager(serviceKey, interfaceClass=interfaceClass) # get() def defaultConfiguration(self): return None - def setConfiguration(self, configFile, serviceTable = None): + def setConfiguration(self, configFile, serviceTable = None, extra = ""): """Sets which configuration to use for setup. - If `serviceTable` is not `None`, a new configuration is created with the - service table as `serviceTable`, and `configPath` is included in that - configuration (presumably to define `serviceTable`). - - If `serviceTable` is `None` instead, the configuration file in - `configPath` is included directly, and it is assumed that it already - properly defines a `services` table. + If `serviceTable` is `None`, the configuration file at `configFile` will be + used directly, and it is assumed that it already properly defines a + `services` table. + + If `serviceTable` is not `None`, instead, a new configuration will be + created with a service table ("services:") as `serviceTable`, and + `configPath` is included in that configuration (presumably to define + `serviceTable`). + + This configuration unrolling will happen when `setup()` is called; this + function only records the configuration parameters needed for that. """ assert configFile is not None self.configuration \ - = ServiceManagerInstance.ConfigurationInfo(configFile, serviceTable) + = ServiceManagerInstance.ConfigurationInfo(configFile, serviceTable, extra=extra) # setConfiguration() - def setup(self): - """Prepares for service provider access in python/Gallery.""" - - if self.manager is not None: return self.manager + def addCustomConfiguration(self, extra): + """Adds extra FHiCL configuration. + + `setConfiguration()` must have been already called. + The configuration in `extra` string is written in FHiCL language and it will + be appended at the end of the configuration. + + The configuration unrolling will happen when `setup()` is called; this + function only records the configuration parameters needed for that. + """ + assert self.configuration, "setConfiguration() needs to be called first." + self.configuration.addExtraConfig(extra) + # addCustomConfiguration() + def loadConfiguration(self): # # configuration "file" # - configurationInfo = self.defaultConfiguration() \ - if self.configuration is None else self.configuration + configurationInfo = self.configuration if self.configuration \ + else self.defaultConfiguration() # if assertion fails, then `setConfiguration()` was not correctly called. - assert self.configuration.isValid() + assert configurationInfo.isValid() - if configurationInfo.fullConfig(): - config = configurationInfo.configPath - else: + 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), + ) config = galleryUtils.ConfigurationString( '#include "{configPath}"' '\n' '\nservices: @local::{serviceTable}' '\n' + '\n# ===============================' + '\n# custom configuration follows ' + '\n# -------------------------------' + '\n' + '\n{extraConfig}' + '\n' + '\n# -------------------------------' + '\n# custom configuration ended ' + '\n# ===============================' .format( configPath=configurationInfo.configPath, - serviceTable=configurationInfo.serviceTable + serviceTable=configurationInfo.serviceTable, + extraConfig=configurationInfo.extraConfig, ) ) + else: + config = configurationInfo.configPath + Logger.debug("Using configuration file: '%s'", config) # if ... else + + return config + # loadConfiguration() + + def serviceLoaderTable(self): + """Returns the table with the services with known loaders.""" + return self.StandardLoadingTable + + def setup(self): + """Prepares for service provider access in python/Gallery.""" + + if self.manager is not None: return self.manager + + config = self.loadConfiguration() # # prepare the service registry and manager # self.manager = ServiceManagerClass \ - (config, loadingTable=ServiceManagerInstance.StandardLoadingTable) - - # - # register the services we know about; - # some are already known - # (`LArSoftUtils.ServiceManagerClass.StandardLoadingTable`), including - # 'Geometry', 'LArProperties', 'DetectorClocks' and 'DetectorProperties', - # but se override the former with our flavor of it - # + (config, loadingTable=self.serviceLoaderTable()) return self.manager From 8fa1a7993bea5131ed45ca97ffd54e4cbcd82076 Mon Sep 17 00:00:00 2001 From: Gianluca Petrillo Date: Fri, 13 Mar 2026 10:47:06 -0700 Subject: [PATCH 6/7] LArSoftUtils.py: fixed name call When renaming is an afterthought. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- sbnalg/gallery/python/LArSoftUtils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sbnalg/gallery/python/LArSoftUtils.py b/sbnalg/gallery/python/LArSoftUtils.py index de9c667..c208a3f 100644 --- a/sbnalg/gallery/python/LArSoftUtils.py +++ b/sbnalg/gallery/python/LArSoftUtils.py @@ -316,7 +316,7 @@ def loadSimpleService \ if not isinstance(config, ROOT.fhicl.ParameterSet): try: - config = getServiceConfig( + config = readServiceConfig( getConfig=(config.service if config else registry.config), configKey=serviceName, ) From 2309ac49238d2c23eebce6142bb74e2074d3eb16 Mon Sep 17 00:00:00 2001 From: Gianluca Petrillo Date: Fri, 13 Mar 2026 13:19:19 -0500 Subject: [PATCH 7/7] Python interface: various PR bugs caught by Copilot --- sbnalg/gallery/python/LArSoftUtils.py | 46 +++++++++++----- sbnalg/gallery/python/ROOTutils.py | 78 +++++++++++++++++---------- sbnalg/gallery/python/cppUtils.py | 11 ++-- sbnalg/gallery/python/galleryUtils.py | 9 ++-- 4 files changed, 90 insertions(+), 54 deletions(-) diff --git a/sbnalg/gallery/python/LArSoftUtils.py b/sbnalg/gallery/python/LArSoftUtils.py index c208a3f..49d4c49 100644 --- a/sbnalg/gallery/python/LArSoftUtils.py +++ b/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, ) ) diff --git a/sbnalg/gallery/python/ROOTutils.py b/sbnalg/gallery/python/ROOTutils.py index 989cb48..a488ed5 100644 --- a/sbnalg/gallery/python/ROOTutils.py +++ b/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 + # `/../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): diff --git a/sbnalg/gallery/python/cppUtils.py b/sbnalg/gallery/python/cppUtils.py index f605a92..9fd8887 100644 --- a/sbnalg/gallery/python/cppUtils.py +++ b/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 diff --git a/sbnalg/gallery/python/galleryUtils.py b/sbnalg/gallery/python/galleryUtils.py index 33d12a0..941a144 100644 --- a/sbnalg/gallery/python/galleryUtils.py +++ b/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: