LArSoft/Python library support for ChannelStatusService [1/2]#7
LArSoft/Python library support for ChannelStatusService [1/2]#7PetrilloAtWork wants to merge 7 commits intoSBNSoftware:developfrom
Conversation
Improvements in cppyy allow to use Python list[str] for C++ vector<str> parameters.
Also moved from print() to logging module.
It now supports as argument a list or a single file. Also moved from print() to logging module.
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
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.
PetrilloAtWork
added
bug
PetrilloAtWork
changed the title
PetrilloAtWork
changed the title
There was a problem hiding this comment.
Pull request overview
Expands the SBNSoftware Python/Gallery helper layer to better support loading/configuring LArSoft services (e.g. lariov::ChannelStatusService), while migrating console output to module-local logging.
Changes:
- Add more flexible service configuration key resolution and service loader configuration options in
LArSoftUtils.py. - Introduce/extend helper utilities (
SourceCentral.loadMany(),expandFileList()multi-path support) to simplify loading headers/libs and file lists. - Replace
print()-based output with module-local loggers across the Python helpers.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
sbnalg/gallery/python/galleryUtils.py |
Switches to logging and updates file-list construction to use the enhanced expandFileList(). |
sbnalg/gallery/python/cppUtils.py |
Adds SourceCentral.loadMany() and migrates warnings to logging. |
sbnalg/gallery/python/ROOTutils.py |
Adds module logger and extends expandFileList() to handle multiple paths and file suffix rules. |
sbnalg/gallery/python/LArSoftUtils.py |
Adds service config key guessing/lookup helpers and expands configuration management for service loading. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try: serviceClassName = serviceClassName[:serviceClassName.index('::')] | ||
| except ValueError: pass # no namespace | ||
|
|
| 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}'") |
There was a problem hiding this comment.
Partially accepted. The error message will contain the key after splitting, but the loop will use its own "local" variable instead of reusing configKey.
| for pathSpec in pathSpecs: | ||
| try: | ||
| res.append(self.load(**pathSpec)) | ||
| except: |
There was a problem hiding this comment.
That is a sensible suggestion. Unfortunately I can't predict which type of exception cppyy might be emitting on ROOT.gSystem.Load()/ROOT.gSystem.ProcessLine() calls (which are at the back end of this).
sbnalg/gallery/python/cppUtils.py
Outdated
| 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 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, | ||
| ) |
When renaming is an afterthought. Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Expands the SBN Python/Gallery helpers to better support loading LArSoft services (e.g., lariov::ChannelStatusService) by improving service configuration key discovery and allowing custom configuration injection, while also migrating user-facing output to logging.
Changes:
- Add smarter service configuration lookup helpers and enhance
SimpleServiceLoader/ServiceManagerInstanceconfiguration handling. - Introduce
SourceCentral.loadMany()and refactor some loaders to use it. - Replace
print()calls with module-levelloggingand expand file-list handling utilities.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
sbnalg/gallery/python/galleryUtils.py |
Switches user output to logging; refactors file list creation and event loop logging/flow control. |
sbnalg/gallery/python/cppUtils.py |
Adds loadMany() helper and migrates include-path warnings to logging. |
sbnalg/gallery/python/ROOTutils.py |
Adds module-level logger and expands expandFileList() to accept multiple inputs and suffix rules. |
sbnalg/gallery/python/LArSoftUtils.py |
Adds service config-key resolution utilities; enhances simple service loader and configuration composition support; migrates output to logging. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try: serviceClassName = serviceClassName[:serviceClassName.rindex('::')] | ||
| except ValueError: pass # no namespace | ||
|
|
There was a problem hiding this comment.
Right again.
Will force-push and try to forget this one.
| return expandFileList(filePaths, | ||
| fileListSuffixes=[ '.list', '.filelist' ], fileSuffixes=[ '.root' ]) |
There was a problem hiding this comment.
The cppyy library became able to convert Python list[str] to std::vector<std::string> when performing C++ function calls, so at Python side both approaches are now supported.
The code did internally still take an extra step when eventLoop() argument was not std::vector<std::string>, which is now superfluous and clearly confuses readers.
I will remove that one.
| if self.configuration is None else self.configuration | ||
| configurationInfo = self.configuration if self.configuration \ | ||
| else self.defaultConfiguration() | ||
|
|
There was a problem hiding this comment.
I have modified the protocol to require that defaultConfiguration() returns a ConfigurationInfo object (even if invalid). In this way configurationInfo variable is guaranteed, apart from implementation errors, to be a ConfigurationInfo object.
PetrilloAtWork
force-pushed
the
feature/gp_pyChanStatus_develop
branch
from
2311c90 to
03ad3bb
Compare
There was a problem hiding this comment.
Pull request overview
Expands the Python gallery/LArSoft helper layer to better support loading/configuring LArSoft services (e.g. lariov::ChannelStatusService), while standardizing user-facing output through logging instead of print().
Changes:
- Add smarter service configuration key resolution and configuration-table composition support in
LArSoftUtils. - Improve/extend file list expansion utilities and switch status/output to module-level loggers.
- Add multi-load helper for headers/libraries in
cppUtilsand refactor call sites to use it.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| sbnalg/gallery/python/galleryUtils.py | Refactors file list creation and event iteration; migrates prints to Logger. |
| sbnalg/gallery/python/cppUtils.py | Adds SourceCentral.loadMany() and replaces stderr prints with Logger. |
| sbnalg/gallery/python/ROOTutils.py | Enhances expandFileList() to support multiple inputs and adds logger usage. |
| sbnalg/gallery/python/LArSoftUtils.py | Adds service config key guessing + custom config composition; migrates logging and refactors service loading. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
sbnalg/gallery/python/ROOTutils.py
Outdated
| 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. |
| inputFiles = makeFileList(*inputFiles) | ||
| # if | ||
| # create a gallery event with an expanded (flattened) input list | ||
| inputFiles = makeFileList(*inputFiles) |
There was a problem hiding this comment.
So Copilot forces me to fix that known issue... fine, fine.
Since now makeFileList() can accept lists of lists, I will just pass the whole list of inputs.
| 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 = readServiceConfig( | ||
| 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))}'" | ||
|
|
There was a problem hiding this comment.
Right. Obviously this path was not exercised during testing, or it would have met the assertion on the next line.
| def defaultConfiguration(self): | ||
| """Returns the default configuration. | ||
|
|
||
| def setConfiguration(self, configFile, serviceTable = None): | ||
| """Sets which configuration to use for setup. | ||
| 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() | ||
|
|
||
| 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`). | ||
| def setConfiguration(self, configFile, serviceTable = None, extra = ""): | ||
| """Sets which configuration to use for setup. | ||
|
|
||
| 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() | ||
|
|
There was a problem hiding this comment.
Truly, the range of possibility has outgrown the single mute assert.
| # if we don't need a special configuration, | ||
| # we let loadSimpleService() find it |
PetrilloAtWork
force-pushed
the
feature/gp_pyChanStatus_develop
branch
from
03ad3bb to
2aa2144
Compare
There was a problem hiding this comment.
Pull request overview
Expands the Python↔LArSoft helper layer to better support loading/configuring additional services (e.g. lariov::ChannelStatusService), while also standardizing runtime output via logging across the helper modules.
Changes:
- Added more flexible service configuration key resolution and loader options (incl. custom/extra FHiCL config support).
- Refactored file-list expansion utilities and integrated them into gallery event setup.
- Replaced
print()-based output with module-localloggingloggers.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
sbnalg/gallery/python/galleryUtils.py |
Updates event/file-list handling and switches output to logging. |
sbnalg/gallery/python/cppUtils.py |
Adds SourceCentral.loadMany() and switches warnings to logging. |
sbnalg/gallery/python/ROOTutils.py |
Introduces logger and expands expandFileList() functionality/heuristics. |
sbnalg/gallery/python/LArSoftUtils.py |
Adds service config-key discovery utilities and enhances service manager configuration/loading. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
sbnalg/gallery/python/ROOTutils.py
Outdated
| 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 |
sbnalg/gallery/python/cppUtils.py
Outdated
| `{ 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 }`. |
| for entry in entries: files.push_back(entry) | ||
| # for | ||
| return files | ||
| return expandFileList(filePaths, |
There was a problem hiding this comment.
I have in vain trying to reproduce the issue described here after the fix at the first comment (which explicitly recognises tuples as sure lists).
I assume that this point is therefore addressed.
PetrilloAtWork
force-pushed
the
feature/gp_pyChanStatus_develop
branch
2 times, most recently
from
b98d027 to
e9ea7ff
Compare
There was a problem hiding this comment.
Pull request overview
Expands the Python gallery/LArSoft helper utilities to better support loading/configuring LArSoft services (notably lariov::ChannelStatusService) and standardizes runtime output via the Python logging module.
Changes:
- Added flexible service-configuration key discovery (
serviceClassToConfigKeys(),readServiceConfig()) and improved service loader configuration handling. - Enhanced file-list expansion to support multiple inputs and additional suffix rules; simplified gallery event input handling.
- Replaced runtime
print()usage with module-level loggers and improved diagnostics around ROOT/message-facility setup.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| sbnalg/gallery/python/galleryUtils.py | Updates file list construction and event iteration behavior; switches runtime output to logging and improves MF startup error reporting. |
| sbnalg/gallery/python/cppUtils.py | Introduces bulk loader (loadMany) and switches include-path warnings to logging. |
| sbnalg/gallery/python/ROOTutils.py | Adds module logger and expands expandFileList() to accept multiple inputs and handle suffix-based expansion rules. |
| sbnalg/gallery/python/LArSoftUtils.py | Adds service config-key discovery + improved configuration assembly (incl. custom config), and switches service-loading messages to logging. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
sbnalg/gallery/python/ROOTutils.py
Outdated
| Only absolute paths are supported in file lists. The current implementation | ||
| will assume all files (and file lists) are relative to the current directory, | ||
| but this behaviour may change in the future (ideally, it will). |
There was a problem hiding this comment.
All statements are true, but I tried to clarify.
So yes, the function does accept relative paths in the sense that it does not reject them and does something with them, but that something may end up in a mistake and may be changed in the future.
| if configurationInfo.needsCustom(): | ||
| Logger.debug( | ||
| "Using service table '%s' from configuration file '%s' plus %d custom lines", | ||
| configurationInfo.serviceTable, configurationInfo.configPath, |
There was a problem hiding this comment.
Aware sloppiness. Fixed.
PetrilloAtWork
force-pushed
the
feature/gp_pyChanStatus_develop
branch
from
e9ea7ff to
ce916cc
Compare
There was a problem hiding this comment.
Pull request overview
Expands the Python gallery/LArSoft helper layer to better support loading/configuring LArSoft services (including lariov::ChannelStatusService), while standardizing user-facing output via Python logging instead of print().
Changes:
- Added more flexible service-configuration key discovery and loader configuration options in
LArSoftUtils.py. - Enhanced file list expansion logic to support multiple input path types and file/list suffix handling.
- Introduced/expanded logging usage and added a multi-load helper for headers/libraries.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| sbnalg/gallery/python/galleryUtils.py | Switches runtime messages to logging, refactors file list construction, and hardens event skipping behavior. |
| sbnalg/gallery/python/cppUtils.py | Adds logging and introduces SourceCentral.loadMany() for batch loading headers/libraries. |
| sbnalg/gallery/python/ROOTutils.py | Expands expandFileList() to handle multiple input forms and configurable suffix rules; moves to module logger. |
| sbnalg/gallery/python/LArSoftUtils.py | Implements flexible service config-key resolution, loader enhancements, and configurable “extra” FHiCL injection; moves to module logger. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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) |
sbnalg/gallery/python/ROOTutils.py
Outdated
| fileListSuffixes: "suffix of entries to recursively add file lists" = [], | ||
| fileSuffixes: "suffix of entries never to be treated as file lists" = [ '.root' ], |
There was a problem hiding this comment.
The possibility of those being changed is very remote. But I agree the proposed change is solid,
| Logger.debug( | ||
| "Using service table '%s' from configuration file '%s' plus %d custom lines", | ||
| configurationInfo.serviceTableName(), configurationInfo.configPath, | ||
| sum(c == '\n' for c in configurationInfo.extraConfig), |
There was a problem hiding this comment.
The count is off by 1. The count is in the content of a print in the form \n{string}, and I count the lines as starting with \n. I have fixed the count (because it's cheap), but I am not going to use .splitlines() which is a waste of resources for an informational message.
PetrilloAtWork
force-pushed
the
feature/gp_pyChanStatus_develop
branch
from
ce916cc to
2309ac4
Compare
There was a problem hiding this comment.
Pull request overview
This PR expands the SBN gallery/LArSoft Python helper modules to better support loading/configuring LArSoft services (notably patterns needed for lariov::ChannelStatusService), while also standardizing runtime output through Python logging.
Changes:
- Added flexible service configuration-key resolution and service-loader configuration options (including custom extra FHiCL snippets).
- Introduced a bulk-load helper (
SourceCode.loadMany) to streamline header/library loading. - Replaced remaining
print()-based output with per-moduleLoggerusage and added a few defensive error-handling/logging tweaks.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| sbnalg/gallery/python/galleryUtils.py | Switches to logging, refactors file list expansion, and adds more robust event skipping/message facility startup handling. |
| sbnalg/gallery/python/cppUtils.py | Adds loadMany() helper and converts warnings to Logger output. |
| sbnalg/gallery/python/ROOTutils.py | Adds module logger and enhances expandFileList() to support multiple input path forms and additional suffix logic. |
| sbnalg/gallery/python/LArSoftUtils.py | Implements service config key candidate logic, extends service loader configuration behavior, and supports injecting extra FHiCL configuration. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Logger.debug( | ||
| "Using service table '%s' from configuration file '%s' plus %d custom lines", | ||
| configurationInfo.serviceTableName(), configurationInfo.configPath, | ||
| sum(c == '\n' for c in configurationInfo.extraConfig) + 1, |
The Python libraries for interface to LArSoft has been expanded to support services like
lariov::ChannelStatusService.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, some maintenance and bug fixing was sprinkled around, and the whole library has been moved to use
logging(and a local logger) for output to screen instead ofprint().Reviewers:
This PR was "built" against
v10_06_00. Since it's Python, there is no actual build though.This PR is a dependency of SBNSoftware/icarusalg#98 (which also implements an example of how to use the new features for
ChannelStatusService).