From d8b9e433bf8b34896ceaa405cc7386fa75056d18 Mon Sep 17 00:00:00 2001 From: graepaul Date: Tue, 27 Oct 2026 10:08:56 -0700 Subject: [PATCH 1/6] Add optional lazy plugin loading and share one PluginRegistry PluginRegistry imported every plugin module at startup (~260 plugin modules, ~2.4s) even when a run uses one or two plugins. Add an opt-in lazy mode (--lazy-load, NODESCRAPER_LAZY_LOAD=1, or PluginRegistry(lazy_load=True)) that indexes plugin names from source without importing and resolves a plugin class on first access, i.e. when PluginExecutor runs it. Eager loading stays the default. - LazyPluginMapping: dict-like name->class mapping; broken plugins are logged and treated as missing instead of aborting the registry. - ConfigRegistry can defer entry-point recipes (lazy_entry_point_configs) so AllIbPlugins no longer resolves every plugin at startup. - In lazy mode the CLI builds per-plugin subparsers only for plugins named on the command line. - Centralize registry creation: cli.main() builds the single registry and installs it with set_default_registry(); PluginDiscovery and the PluginExecutor fallback use default_registry() instead of building their own. - Add scripts/memray to compare memory use of the eager and lazy paths. Co-Authored-By: Claude --- nodescraper/cli/cli.py | 92 +++++++-- nodescraper/cli/helper.py | 4 +- nodescraper/configbuilder.py | 7 +- nodescraper/configregistry.py | 149 ++++++++++---- nodescraper/pluginexecutor.py | 10 +- nodescraper/pluginrecipe/discovery.py | 35 ++-- nodescraper/pluginregistry.py | 275 ++++++++++++++++++++++++-- scripts/memray/registry_target.py | 38 ++++ scripts/memray/run_memray.sh | 66 +++++++ 9 files changed, 584 insertions(+), 92 deletions(-) create mode 100644 scripts/memray/registry_target.py create mode 100755 scripts/memray/run_memray.sh diff --git a/nodescraper/cli/cli.py b/nodescraper/cli/cli.py index 1e1b9783..7240c413 100644 --- a/nodescraper/cli/cli.py +++ b/nodescraper/cli/cli.py @@ -32,7 +32,7 @@ import platform import sys import uuid -from collections.abc import Callable, Sequence +from collections.abc import Callable, Collection, Sequence from typing import Optional import nodescraper @@ -72,7 +72,12 @@ from nodescraper.models import SystemInfo from nodescraper.models.pluginresult import PluginResult from nodescraper.pluginexecutor import PluginExecutor -from nodescraper.pluginregistry import PluginRegistry +from nodescraper.pluginrecipe.discovery import PluginDiscovery +from nodescraper.pluginregistry import ( + LAZY_LOAD_ENV_VAR, + PluginRegistry, + set_default_registry, +) def _parse_plugin_configs_csv(value: str) -> list[str]: @@ -80,21 +85,21 @@ def _parse_plugin_configs_csv(value: str) -> list[str]: return [p.strip() for p in value.split(",") if p.strip()] -def _default_config_registry(_plugin_reg: PluginRegistry) -> ConfigRegistry: +def _default_config_registry(lazy: bool = False) -> ConfigRegistry: """Build the config registry from bundled JSON and plugin-config entry points. Args: - _plugin_reg (PluginRegistry): Unused; retained for call-site compatibility. + lazy (bool): Evaluate entry-point configs (recipes) only when requested by name, so that + recipes which inspect every plugin do not defeat lazy plugin loading. Returns: ConfigRegistry: Registry containing bundled and entry-point plugin configs. """ - return ConfigRegistry() + return ConfigRegistry(lazy_entry_point_configs=lazy) def _add_cli_root_globals( parser: argparse.ArgumentParser, - plugin_reg: PluginRegistry, config_reg: ConfigRegistry, ) -> None: """Register top-level flags before ``subcmd`` subparsers (shared with :func:`build_global_argument_parser`).""" @@ -204,28 +209,40 @@ def _add_cli_root_globals( help="Skip plugins that require sudo permissions", ) + parser.add_argument( + "--lazy-load", + dest="lazy_load", + action="store_true", + help=f"Import plugin modules only when they are needed instead of at startup " + f"(can also be enabled with the {LAZY_LOAD_ENV_VAR}=1 environment variable)", + ) + def build_global_argument_parser(*, add_help: bool = True) -> argparse.ArgumentParser: """Globals only (no subcommands), for host CLIs.""" - plugin_reg = PluginRegistry() - config_reg = _default_config_registry(plugin_reg) + config_reg = _default_config_registry(lazy=True) parser = argparse.ArgumentParser( description="node scraper CLI (global options only)", formatter_class=argparse.ArgumentDefaultsHelpFormatter, add_help=add_help, ) - _add_cli_root_globals(parser, plugin_reg, config_reg) + _add_cli_root_globals(parser, config_reg) return parser def build_parser( plugin_reg: PluginRegistry, config_reg: ConfigRegistry, + only_plugins: Optional[Collection[str]] = None, ) -> tuple[argparse.ArgumentParser, dict[str, tuple[argparse.ArgumentParser, dict]]]: """Build an argument parser Args: plugin_reg (PluginRegistry): registry of plugins + config_reg (ConfigRegistry): registry of configs + only_plugins (Optional[Collection[str]]): if given, per-plugin ``run-plugins`` subparsers are + built (which requires importing the plugin class) only for these names. ``None`` builds + a subparser for every registered plugin. Returns: tuple[argparse.ArgumentParser, dict[str, tuple[argparse.ArgumentParser, dict]]]: tuple containing main @@ -236,7 +253,7 @@ def build_parser( formatter_class=argparse.ArgumentDefaultsHelpFormatter, ) - _add_cli_root_globals(parser, plugin_reg, config_reg) + _add_cli_root_globals(parser, config_reg) subparsers = parser.add_subparsers(dest="subcmd", help="Subcommands") subparsers.default = "run-plugins" @@ -370,7 +387,15 @@ def build_parser( ) plugin_subparser_map = {} - for plugin_name, plugin_class in plugin_reg.plugins.items(): + if only_plugins is None: + plugin_items = list(plugin_reg.plugins.items()) + else: + plugin_items = [ + (name, plugin_class) + for name, plugin_class in ((n, plugin_reg.plugins.get(n)) for n in only_plugins) + if plugin_class is not None + ] + for plugin_name, plugin_class in plugin_items: plugin_subparser = plugin_subparsers.add_parser( plugin_name, help=f"Run {plugin_name} plugin", @@ -410,9 +435,9 @@ def get_cli_top_level_subcommands() -> tuple[str, ...]: Returns: Tuple of ``subcmd`` subparser names; call ``cache_clear()`` if registries change in-process. """ - plugin_reg = PluginRegistry() - config_reg = _default_config_registry(plugin_reg) - parser, _plugin_subparser_map = build_parser(plugin_reg, config_reg) + plugin_reg = PluginRegistry(lazy_load=True) + config_reg = _default_config_registry(lazy=True) + parser, _plugin_subparser_map = build_parser(plugin_reg, config_reg, only_plugins=()) return _top_level_subcommand_names(parser) @@ -472,6 +497,34 @@ def _handle_keyboard_interrupt(logger: Optional[logging.Logger] = None) -> None: sys.exit(KEYBOARD_INTERRUPT_EXIT_CODE) +def _lazy_load_flag_present(arg_input: Sequence[str]) -> bool: + """True if ``--lazy-load`` appears among the top-level args (before ``run-plugins``). + + The registry must exist before the real parser is built, so this checks for the flag + on its own. ``NODESCRAPER_LAZY_LOAD`` is handled by the registry itself. + """ + top_level = list(arg_input) + if "run-plugins" in top_level: + top_level = top_level[: top_level.index("run-plugins")] + pre_parser = argparse.ArgumentParser(add_help=False) + pre_parser.add_argument("--lazy-load", dest="lazy_load", action="store_true") + known, _ = pre_parser.parse_known_args(top_level) + return bool(known.lazy_load) + + +def _plugins_needing_parsers( + plugin_reg: PluginRegistry, arg_input: Sequence[str] +) -> Optional[list[str]]: + """Plugin names whose ``run-plugins`` subparser must be built in lazy mode. + + Returns ``None`` (build everything) when help was requested so help output stays complete. + """ + if any(arg in ("-h", "--help") for arg in arg_input): + return None + _, plugin_arg_map, _ = process_args(list(arg_input), list(plugin_reg.plugins.keys())) + return list(plugin_arg_map) + + def main( arg_input: Optional[list[str]] = None, *, @@ -492,9 +545,14 @@ def main( if arg_input is None: arg_input = sys.argv[1:] - plugin_reg = PluginRegistry() - config_reg = _default_config_registry(plugin_reg) - parser, plugin_subparser_map = build_parser(plugin_reg, config_reg) + # The one registry for this invocation: shared with recipes/discovery and the executor. + plugin_reg = PluginRegistry(lazy_load=True if _lazy_load_flag_present(arg_input) else None) + set_default_registry(plugin_reg) + PluginDiscovery.clear_cache() + lazy = bool(getattr(plugin_reg, "lazy", False)) + config_reg = _default_config_registry(lazy=lazy) + only_plugins = _plugins_needing_parsers(plugin_reg, arg_input) if lazy else None + parser, plugin_subparser_map = build_parser(plugin_reg, config_reg, only_plugins) try: top_level_args, plugin_arg_map, invalid_plugins = process_args( diff --git a/nodescraper/cli/helper.py b/nodescraper/cli/helper.py index 5385de76..60ab780c 100644 --- a/nodescraper/cli/helper.py +++ b/nodescraper/cli/helper.py @@ -240,10 +240,10 @@ def parse_describe( log_cli_text_block(logger, out) elif parsed_args.type == "plugin": - if parsed_args.name not in plugin_reg.plugins: + plugin_class = plugin_reg.plugins.get(parsed_args.name) + if plugin_class is None: logger.error("No plugin found for name: %s", parsed_args.name) sys.exit(1) - plugin_class = plugin_reg.plugins[parsed_args.name] out = [ f"Plugin Name: {parsed_args.name}", f"Description: {getattr(plugin_class, '__doc__', '')}", diff --git a/nodescraper/configbuilder.py b/nodescraper/configbuilder.py index bc8f1b8a..03b4807d 100644 --- a/nodescraper/configbuilder.py +++ b/nodescraper/configbuilder.py @@ -54,10 +54,9 @@ def gen_config(self, plugin_names: list[str]) -> PluginConfig: """ config = PluginConfig() for plugin in plugin_names: - if plugin in self.plugin_registry.plugins: - config.plugins[plugin] = self._build_plugin_config( - self.plugin_registry.plugins[plugin] - ) + plugin_class = self.plugin_registry.plugins.get(plugin) + if plugin_class is not None: + config.plugins[plugin] = self._build_plugin_config(plugin_class) else: self.logger.warning("No plugin found with name: %s", plugin) return config diff --git a/nodescraper/configregistry.py b/nodescraper/configregistry.py index c56cf922..fd60169a 100644 --- a/nodescraper/configregistry.py +++ b/nodescraper/configregistry.py @@ -29,6 +29,7 @@ import inspect import json import os +from collections.abc import Callable, Iterator, MutableMapping from pathlib import Path from typing import Any, Optional @@ -43,6 +44,50 @@ class PluginConfigEntryPointError(RuntimeError): """Raised when a ``nodescraper.plugin_configs`` entry point cannot be loaded.""" +class LazyConfigMapping(MutableMapping): + """Config name to :class:`PluginConfig` mapping that builds each config on first access. + + Listing names never runs a loader, so recipes that inspect every plugin are not + evaluated until their config is actually requested. + """ + + def __init__(self) -> None: + self._values: dict[str, PluginConfig] = {} + self._loaders: dict[str, Callable[[], PluginConfig]] = {} + + def add_loader(self, name: str, loader: Callable[[], PluginConfig]) -> None: + self._values.pop(name, None) + self._loaders[name] = loader + + def __getitem__(self, name: str) -> PluginConfig: + if name in self._values: + return self._values[name] + loader = self._loaders.get(name) + if loader is None: + raise KeyError(name) + self._values[name] = loader() + return self._values[name] + + def __setitem__(self, name: str, value: PluginConfig) -> None: + self._loaders.pop(name, None) + self._values[name] = value + + def __delitem__(self, name: str) -> None: + if name not in self._values and name not in self._loaders: + raise KeyError(name) + self._values.pop(name, None) + self._loaders.pop(name, None) + + def __contains__(self, name: object) -> bool: + return name in self._values or name in self._loaders + + def __iter__(self) -> Iterator[str]: + return iter([*self._values, *(n for n in self._loaders if n not in self._values)]) + + def __len__(self) -> int: + return len(self._values) + sum(1 for n in self._loaders if n not in self._values) + + class ConfigRegistry: """Class to load json plugin configs into models""" @@ -52,6 +97,7 @@ def __init__( self, config_path: Optional[str] = None, load_entry_point_configs: bool = True, + lazy_entry_point_configs: bool = False, ) -> None: """Initialize the config registry. @@ -60,11 +106,28 @@ def __init__( Defaults to None. load_entry_point_configs (bool, optional): Whether to load ``nodescraper.plugin_configs`` entry points. Defaults to True. + lazy_entry_point_configs (bool, optional): If True, entry-point configs are loaded (and + their recipes evaluated) only when first accessed by name. Load errors then surface + at access time instead of construction. Defaults to False. """ - self.configs: dict[str, PluginConfig] = {} + self.configs: MutableMapping[str, PluginConfig] + if lazy_entry_point_configs: + self.configs = LazyConfigMapping() + else: + self.configs = {} self.load_configs(config_path) if load_entry_point_configs: - self.configs.update(self.load_plugin_configs_from_entry_points()) + if lazy_entry_point_configs: + assert isinstance(self.configs, LazyConfigMapping) + for entry_point in self._entry_points_for_group(PLUGIN_CONFIG_ENTRY_POINT_GROUP): + name = getattr(entry_point, "name", None) + if name: + self.configs.add_loader( + name, + lambda ep=entry_point: self._load_entry_point_config(ep)[1], + ) + else: + self.configs.update(self.load_plugin_configs_from_entry_points()) def load_configs(self, config_path: Optional[str] = None): """Load plugin config JSON files into pydantic models. @@ -136,6 +199,50 @@ def _resolve_entry_point_config( return config_data return None + @classmethod + def _load_entry_point_config(cls, entry_point: Any) -> tuple[Optional[str], PluginConfig]: + """Load one ``nodescraper.plugin_configs`` entry point. + + Returns: + tuple[Optional[str], PluginConfig]: Config key (entry-point name, else config name) and model. + + Raises: + PluginConfigEntryPointError: If the entry point target is missing, invalid, or unsupported. + """ + entry_point_name = getattr(entry_point, "name", None) + entry_point_label = entry_point_name or "" + try: + loaded = entry_point.load() + config_data = cls._resolve_entry_point_config(loaded) + if config_data is None: + raise PluginConfigEntryPointError( + f"Failed to load plugin config entry point {entry_point_label!r}: " + f"unsupported target {loaded!r}" + ) + + config_model = ( + config_data + if isinstance(config_data, PluginConfig) + else PluginConfig(**config_data) + ) + return entry_point_name or config_model.name, config_model + except ModuleNotFoundError as exc: + raise PluginConfigEntryPointError( + f"Failed to load plugin config entry point {entry_point_label!r}: " + f"module not found ({exc}). Check the entry point target in pyproject.toml." + ) from exc + except ValidationError as exc: + raise PluginConfigEntryPointError( + f"Failed to load plugin config entry point {entry_point_label!r}: " + "invalid plugin config" + ) from exc + except PluginConfigEntryPointError: + raise + except Exception as exc: + raise PluginConfigEntryPointError( + f"Failed to load plugin config entry point {entry_point_label!r}: {exc}" + ) from exc + @classmethod def load_plugin_configs_from_entry_points(cls) -> dict[str, PluginConfig]: """Load plugin configs registered under ``nodescraper.plugin_configs`` entry points. @@ -149,40 +256,8 @@ def load_plugin_configs_from_entry_points(cls) -> dict[str, PluginConfig]: configs: dict[str, PluginConfig] = {} for entry_point in cls._entry_points_for_group(PLUGIN_CONFIG_ENTRY_POINT_GROUP): - entry_point_name = getattr(entry_point, "name", None) - entry_point_label = entry_point_name or "" - try: - loaded = entry_point.load() # type: ignore[attr-defined] - config_data = cls._resolve_entry_point_config(loaded) - if config_data is None: - raise PluginConfigEntryPointError( - f"Failed to load plugin config entry point {entry_point_label!r}: " - f"unsupported target {loaded!r}" - ) - - config_model = ( - config_data - if isinstance(config_data, PluginConfig) - else PluginConfig(**config_data) - ) - config_key = entry_point_name or config_model.name - if config_key: - configs[config_key] = config_model - except ModuleNotFoundError as exc: - raise PluginConfigEntryPointError( - f"Failed to load plugin config entry point {entry_point_label!r}: " - f"module not found ({exc}). Check the entry point target in pyproject.toml." - ) from exc - except ValidationError as exc: - raise PluginConfigEntryPointError( - f"Failed to load plugin config entry point {entry_point_label!r}: " - "invalid plugin config" - ) from exc - except PluginConfigEntryPointError: - raise - except Exception as exc: - raise PluginConfigEntryPointError( - f"Failed to load plugin config entry point {entry_point_label!r}: {exc}" - ) from exc + config_key, config_model = cls._load_entry_point_config(entry_point) + if config_key: + configs[config_key] = config_model return configs diff --git a/nodescraper/pluginexecutor.py b/nodescraper/pluginexecutor.py index 4571ab03..0c20bb06 100644 --- a/nodescraper/pluginexecutor.py +++ b/nodescraper/pluginexecutor.py @@ -48,7 +48,7 @@ from nodescraper.interfaces.taskresulthook import TaskResultHook from nodescraper.models import PluginConfig, SystemInfo from nodescraper.models.pluginresult import PluginResult -from nodescraper.pluginregistry import PluginRegistry +from nodescraper.pluginregistry import PluginRegistry, default_registry from nodescraper.taskresulthooks import FileSystemLogHook from nodescraper.typeutils import TypeUtils @@ -100,7 +100,7 @@ def __init__( logger = logging.getLogger(DEFAULT_LOGGER) self.logger = logger - self.plugin_registry = plugin_registry or PluginRegistry() + self.plugin_registry = plugin_registry or default_registry() if system_info is None: system_info = SystemInfo() @@ -336,12 +336,12 @@ def _run_single_plugin( logged), ``False`` if it was skipped due to a registry miss, missing connection manager, or invalid global args. """ - if plugin_name not in self.plugin_registry.plugins: + # With a lazy registry this is where the plugin module is imported. + plugin_class = self.plugin_registry.plugins.get(plugin_name) + if plugin_class is None: self.logger.error("Unable to find registered plugin for name %s", plugin_name) return False - plugin_class = self.plugin_registry.plugins[plugin_name] - init_payload = { "system_info": self.system_info, "logger": self.logger, diff --git a/nodescraper/pluginrecipe/discovery.py b/nodescraper/pluginrecipe/discovery.py index 65d0186e..3e1ee0e4 100644 --- a/nodescraper/pluginrecipe/discovery.py +++ b/nodescraper/pluginrecipe/discovery.py @@ -27,9 +27,9 @@ from __future__ import annotations import threading -from typing import TYPE_CHECKING, Iterable +from typing import TYPE_CHECKING, Iterable, Mapping -from nodescraper.pluginregistry import PluginRegistry +from nodescraper.pluginregistry import default_registry if TYPE_CHECKING: from nodescraper.interfaces import PluginInterface @@ -43,7 +43,8 @@ class PluginDiscovery: If use_cache is False, it will query the PluginRegistry each time. """ - _plugin_cache: dict[str, type[PluginInterface]] | None = None + _plugin_cache: Mapping[str, type[PluginInterface]] | None = None + _cache_registry: object | None = None _cache_lock = threading.Lock() COLLECTOR_ATTRIBUTE = "COLLECTOR" ANALYZER_ATTRIBUTE = "ANALYZER" @@ -56,14 +57,23 @@ def __init__(self, use_cache: bool = True) -> None: """ self._use_cache = use_cache - def load_plugin_class(self, plugin_name: str) -> type | None: + def _plugins(self) -> Mapping[str, type[PluginInterface]]: + """Plugin mapping from the shared registry (kept as-is, never copied, so lazy loading is preserved).""" if not self._use_cache: - return PluginRegistry().plugins.get(plugin_name) + return default_registry().plugins + registry = default_registry() with PluginDiscovery._cache_lock: - if PluginDiscovery._plugin_cache is None: - PluginDiscovery._plugin_cache = PluginRegistry().plugins.copy() - return PluginDiscovery._plugin_cache.get(plugin_name) + if ( + PluginDiscovery._plugin_cache is None + or PluginDiscovery._cache_registry is not registry + ): + PluginDiscovery._plugin_cache = registry.plugins + PluginDiscovery._cache_registry = registry + return PluginDiscovery._plugin_cache + + def load_plugin_class(self, plugin_name: str) -> type | None: + return self._plugins().get(plugin_name) def plugin_has_collector(self, plugin_name: str) -> bool: """Check if a plugin has a COLLECTOR attribute. @@ -125,6 +135,7 @@ def clear_cache() -> None: """ with PluginDiscovery._cache_lock: PluginDiscovery._plugin_cache = None + PluginDiscovery._cache_registry = None def registered_plugin_names(self) -> tuple[str, ...]: """Return all plugin names known to :class:`~nodescraper.pluginregistry.PluginRegistry`. @@ -132,13 +143,7 @@ def registered_plugin_names(self) -> tuple[str, ...]: Returns: tuple[str, ...]: Sorted registered plugin names. """ - if not self._use_cache: - return tuple(sorted(PluginRegistry().plugins.keys())) - - with PluginDiscovery._cache_lock: - if PluginDiscovery._plugin_cache is None: - PluginDiscovery._plugin_cache = PluginRegistry().plugins - return tuple(sorted(PluginDiscovery._plugin_cache.keys())) + return tuple(sorted(self._plugins().keys())) def plugin_names_matching(self, names: Iterable[str]) -> tuple[str, ...]: """Return plugin names from ``names`` that are registered at runtime. diff --git a/nodescraper/pluginregistry.py b/nodescraper/pluginregistry.py index ff6a7478..9d587e00 100644 --- a/nodescraper/pluginregistry.py +++ b/nodescraper/pluginregistry.py @@ -23,13 +23,18 @@ # SOFTWARE. # ############################################################################### +import ast +import functools import importlib import importlib.metadata import inspect +import logging +import os import pkgutil import threading import types -from typing import Iterable, Optional +from collections.abc import MutableMapping +from typing import Callable, Iterable, Iterator, Optional # Python 3.9 compatibility: EntryPoints type was added in 3.10 try: @@ -50,6 +55,109 @@ ENTRY_POINT_PLUGINS = "nodescraper.plugins" ENTRY_POINT_CONNECTION_MANAGERS = "nodescraper.connection_managers" +# Environment variable enabling lazy plugin loading when ``lazy_load`` is not given explicitly +LAZY_LOAD_ENV_VAR = "NODESCRAPER_LAZY_LOAD" + +_logger = logging.getLogger(__name__) + + +def lazy_load_from_env() -> bool: + """Return True if ``NODESCRAPER_LAZY_LOAD`` is set to a truthy value (1/true/yes/on).""" + return os.environ.get(LAZY_LOAD_ENV_VAR, "").strip().lower() in ("1", "true", "yes", "on") + + +class LazyPluginMapping(MutableMapping): + """Mapping of plugin name to plugin class that imports each class on first access. + + Listing names (``keys()``, ``in``, ``len``, iteration) never imports plugin modules. + ``mapping[name]`` / ``get(name)`` import just that plugin. A plugin that fails to + import or is not valid is logged and treated as missing; other plugins are unaffected. + ``items()`` / ``values()`` / ``copy()`` resolve every plugin. + """ + + def __init__(self, base_class: type) -> None: + self._base_class = base_class + self._loaders: dict[str, Callable[[], Optional[type]]] = {} + self._resolved: dict[str, type] = {} + self._failed: set[str] = set() + self._lock = threading.RLock() + + def add_loader(self, name: str, loader: Callable[[], Optional[type]]) -> None: + """Register a callable that returns the class for ``name`` when first needed.""" + with self._lock: + self._loaders[name] = loader + self._resolved.pop(name, None) + self._failed.discard(name) + + def is_resolved(self, name: str) -> bool: + return name in self._resolved + + def __getitem__(self, name: str) -> type: + resolved = self._resolved.get(name) + if resolved is not None: + return resolved + with self._lock: + if name in self._resolved: + return self._resolved[name] + loader = self._loaders.get(name) + if loader is None or name in self._failed: + raise KeyError(name) + try: + cls = loader() + except Exception as exc: # a broken plugin must not break the others + _logger.warning("Unable to load plugin %s: %s", name, exc) + self._failed.add(name) + raise KeyError(name) from exc + if cls is None or not PluginRegistry._valid_sub_class_check(cls, self._base_class): + self._failed.add(name) + raise KeyError(name) + if hasattr(cls, "is_valid") and not cls.is_valid(): + self._failed.add(name) + raise KeyError(name) + self._resolved[name] = cls + return cls + + def __setitem__(self, name: str, value: type) -> None: + with self._lock: + self._resolved[name] = value + self._loaders[name] = lambda: value + self._failed.discard(name) + + def __delitem__(self, name: str) -> None: + with self._lock: + if name not in self._loaders: + raise KeyError(name) + del self._loaders[name] + self._resolved.pop(name, None) + self._failed.discard(name) + + def __contains__(self, name: object) -> bool: + return name in self._loaders and name not in self._failed + + def __iter__(self) -> Iterator[str]: + return iter([n for n in list(self._loaders) if n not in self._failed]) + + def __len__(self) -> int: + return len(self._loaders) - len(self._failed) + + def items(self): # type: ignore[override] + """Resolve all plugins and return ``(name, class)`` pairs, skipping unloadable ones.""" + pairs = [(n, self._try_get(n)) for n in list(self)] + return [(n, cls) for n, cls in pairs if cls is not None] + + def values(self): # type: ignore[override] + return [cls for _, cls in self.items()] + + def copy(self) -> dict[str, type]: + """Return a plain dict of all loadable plugins (resolves every plugin).""" + return dict(self.items()) + + def _try_get(self, name: str) -> Optional[type]: + try: + return self[name] + except KeyError: + return None + class PluginRegistry: """This class dynamically loads plugins. Internal plugins are loaded by default using @@ -66,6 +174,8 @@ class PluginRegistry: _module_cache: dict[str, types.ModuleType] = {} # Cache for entry points by group name _entry_points_cache: dict[str, EntryPoints] = {} + # Cache for the lazy-mode plugin name index (name -> (module, class)) + _plugin_index_cache: Optional[dict[str, tuple[str, str]]] = None # Global cache control switch _use_cache: bool = True @@ -79,6 +189,7 @@ def __init__( load_internal_plugins: bool = True, load_entry_point_plugins: bool = True, load_entry_point_connection_managers: bool = True, + lazy_load: Optional[bool] = None, ) -> None: """Initialize the PluginRegistry with optional plugin packages. @@ -88,7 +199,12 @@ def __init__( load_entry_point_plugins (bool, optional): Whether to load plugins from entry points. Defaults to True. load_entry_point_connection_managers (bool, optional): Whether to load connection managers from the ``nodescraper.connection_managers`` entry-point group. Defaults to True. + lazy_load (Optional[bool], optional): If True, ``plugins`` is a :class:`LazyPluginMapping` and plugin + modules are imported only when a plugin is first accessed. If None, the + ``NODESCRAPER_LAZY_LOAD`` environment variable decides (default: eager). """ + self.lazy: bool = lazy_load_from_env() if lazy_load is None else lazy_load + if load_internal_plugins: self.plugin_pkg = [ internal_plugins, @@ -101,15 +217,17 @@ def __init__( if plugin_pkg: self.plugin_pkg += plugin_pkg - self.plugins: dict[str, type[PluginInterface]] = PluginRegistry.load_plugins( - PluginInterface, self.plugin_pkg - ) - self.connection_managers: dict[str, type[ConnectionManager]] = PluginRegistry.load_plugins( - ConnectionManager, self.plugin_pkg - ) - self.result_collators: dict[str, type[PluginResultCollator]] = PluginRegistry.load_plugins( - PluginResultCollator, self.plugin_pkg - ) + self.plugins: MutableMapping[str, type[PluginInterface]] + if self.lazy: + self._init_lazy(plugin_pkg or [], load_internal_plugins) + else: + self.plugins = PluginRegistry.load_plugins(PluginInterface, self.plugin_pkg) + self.connection_managers: dict[str, type[ConnectionManager]] = ( + PluginRegistry.load_plugins(ConnectionManager, self.plugin_pkg) + ) + self.result_collators: dict[str, type[PluginResultCollator]] = ( + PluginRegistry.load_plugins(PluginResultCollator, self.plugin_pkg) + ) if load_entry_point_connection_managers: for ( @@ -119,8 +237,110 @@ def __init__( self.connection_managers[name] = mgr_cls if load_entry_point_plugins: - entry_point_plugins = self.load_plugins_from_entry_points() - self.plugins.update(entry_point_plugins) + if self.lazy: + self._add_lazy_entry_point_plugins() + else: + self.plugins.update(self.load_plugins_from_entry_points()) + + def _init_lazy(self, extra_pkgs: list[types.ModuleType], load_internal: bool) -> None: + """Lazy setup: index internal plugin names without importing them. + + Connection managers and result collators are few, so they are still loaded eagerly, + but only from their own packages (not by walking every plugin module). + """ + lazy_plugins = LazyPluginMapping(PluginInterface) + if load_internal: + index = PluginRegistry._internal_plugin_index() + for name, (module_name, class_name) in index.items(): + lazy_plugins.add_loader( + name, + functools.partial(PluginRegistry._import_class, module_name, class_name), + ) + if extra_pkgs: + # Caller-supplied packages have no naming convention to index; load them directly + for name, cls in PluginRegistry.load_plugins(PluginInterface, extra_pkgs).items(): + lazy_plugins[name] = cls + self.plugins = lazy_plugins + + non_plugin_pkgs = ([internal_connections] if load_internal else []) + extra_pkgs + self.connection_managers = PluginRegistry.load_plugins(ConnectionManager, non_plugin_pkgs) + collator_pkgs = ([internal_collators] if load_internal else []) + extra_pkgs + self.result_collators = PluginRegistry.load_plugins(PluginResultCollator, collator_pkgs) + + def _add_lazy_entry_point_plugins(self) -> None: + """Register entry-point plugins by entry-point name; ``load()`` runs on first access.""" + assert isinstance(self.plugins, LazyPluginMapping) + for entry_point in PluginRegistry.load_entry_points(ENTRY_POINT_PLUGINS): + # Eager mode keys by class __name__, which is the attribute in ``module:Class``. + value = getattr(entry_point, "value", "") or "" + attr = value.partition(":")[2].strip().split(".")[-1].split("[")[0].strip() + self.plugins.add_loader( + attr or entry_point.name, + entry_point.load, # type: ignore[attr-defined] + ) + + @staticmethod + def _import_class(module_name: str, class_name: str) -> type: + """Import ``module_name`` (through the shared module cache) and return ``class_name``.""" + with PluginRegistry._cache_lock: + module = PluginRegistry._module_cache.get(module_name) + if module is None: + module = importlib.import_module(module_name) + if PluginRegistry._use_cache: + with PluginRegistry._cache_lock: + PluginRegistry._module_cache[module_name] = module + return getattr(module, class_name) + + @staticmethod + def _base_name(node: ast.expr) -> str: + if isinstance(node, ast.Subscript): + node = node.value + if isinstance(node, ast.Attribute): + return node.attr + if isinstance(node, ast.Name): + return node.id + return "" + + @staticmethod + def _scan_plugin_index(pkg: types.ModuleType) -> dict[str, tuple[str, str]]: + """Map plugin class name to ``(module, class)`` by parsing ``*plugin*.py`` sources. + + Nothing is imported. A class is a candidate if a base class name contains ``Plugin``; + validity is checked when the class is resolved. + """ + index: dict[str, tuple[str, str]] = {} + for root in list(getattr(pkg, "__path__", [])): + for dirpath, dirnames, filenames in os.walk(root): + dirnames[:] = sorted(d for d in dirnames if d != "__pycache__") + for filename in sorted(filenames): + if "plugin" not in filename or not filename.endswith(".py"): + continue + path = os.path.join(dirpath, filename) + rel = os.path.relpath(path, root)[: -len(".py")] + module_name = ".".join([pkg.__name__, *rel.split(os.sep)]) + try: + with open(path, "r", encoding="utf-8") as src: + tree = ast.parse(src.read(), filename=path) + except (OSError, SyntaxError, ValueError) as exc: + _logger.warning("Unable to index plugin module %s: %s", path, exc) + continue + for node in tree.body: + if not isinstance(node, ast.ClassDef): + continue + bases = [PluginRegistry._base_name(b) for b in node.bases] + if any("Plugin" in b for b in bases): + index[node.name] = (module_name, node.name) + return index + + @staticmethod + def _internal_plugin_index() -> dict[str, tuple[str, str]]: + """Cached name index for ``nodescraper.plugins``.""" + with PluginRegistry._cache_lock: + if PluginRegistry._plugin_index_cache is None or not PluginRegistry._use_cache: + PluginRegistry._plugin_index_cache = PluginRegistry._scan_plugin_index( + internal_plugins + ) + return PluginRegistry._plugin_index_cache @staticmethod def load_plugins( @@ -318,8 +538,39 @@ def clear_caches(cls) -> None: Thread-safe: Acquires all locks to ensure no other thread is accessing caches. """ + global _default_registry with cls._cache_lock: cls._entry_point_plugins_cache = None cls._entry_point_connection_managers_cache = None + cls._plugin_index_cache = None cls._module_cache.clear() cls._entry_points_cache.clear() + _default_registry = None + + +_default_registry: Optional[PluginRegistry] = None + + +def default_registry() -> PluginRegistry: + """Return the process-wide shared registry, creating it on first use. + + The CLI installs its own registry with :func:`set_default_registry`; embedders and + library code that are not handed a registry share this one instead of building new ones. + """ + global _default_registry + with PluginRegistry._cache_lock: + existing = _default_registry + if existing is not None: + return existing + created = PluginRegistry() + with PluginRegistry._cache_lock: + if _default_registry is None: + _default_registry = created + return _default_registry + + +def set_default_registry(registry: Optional[PluginRegistry]) -> None: + """Install ``registry`` as the shared default (``None`` resets it).""" + global _default_registry + with PluginRegistry._cache_lock: + _default_registry = registry diff --git a/scripts/memray/registry_target.py b/scripts/memray/registry_target.py new file mode 100644 index 00000000..c4408b6f --- /dev/null +++ b/scripts/memray/registry_target.py @@ -0,0 +1,38 @@ +"""Target for memray: build a PluginRegistry in eager or lazy mode and optionally resolve plugins. + +Run through ``run_memray.sh`` (or directly: ``memray run -o out.bin scripts/memray/registry_target.py --mode lazy``). +""" + +import argparse +import sys + +from nodescraper.pluginregistry import PluginRegistry + + +def main() -> None: + parser = argparse.ArgumentParser() + parser.add_argument("--mode", choices=["eager", "lazy"], required=True) + parser.add_argument( + "--plugin", + action="append", + default=[], + help="Plugin name to resolve after building the registry (repeatable). " + "Simulates a run that only uses these plugins.", + ) + args = parser.parse_args() + + registry = PluginRegistry(lazy_load=args.mode == "lazy") + for name in args.plugin: + registry.plugins[name] + + plugin_modules = sum( + 1 for m in sys.modules if m.startswith("nodescraper.plugins.") and m.count(".") >= 3 + ) + sys.stdout.write( + f"mode={args.mode} plugins_listed={len(registry.plugins)} " + f"plugin_modules_imported={plugin_modules} total_modules={len(sys.modules)}\n" + ) + + +if __name__ == "__main__": + main() diff --git a/scripts/memray/run_memray.sh b/scripts/memray/run_memray.sh new file mode 100755 index 00000000..2226170f --- /dev/null +++ b/scripts/memray/run_memray.sh @@ -0,0 +1,66 @@ +#!/usr/bin/env bash +# Compare memory use of the old (eager) and new (--lazy-load) plugin loading with memray, +# running the real node-scraper CLI. +# +# Usage: scripts/memray/run_memray.sh +# +# Override the invocation with env vars (defaults match a typical quanta run): +# CONNECTION_CONFIG path to the connection config JSON +# SYS_NAME, SYS_LOCATION, SYS_SKU, SYS_PLATFORM +# PLUGINS plugin name(s) passed to run-plugins (default: OsPlugin) +# OUT_DIR output directory (default: ./memray_out) +# +# Registry-only comparison (no connection needed): +# memray run -o out.bin scripts/memray/registry_target.py --mode {eager|lazy} --plugin OsPlugin +# +# Requires: pip install memray +set -euo pipefail + +REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)" +OUT_DIR="${OUT_DIR:-$PWD/memray_out}" +CONNECTION_CONFIG="${CONNECTION_CONFIG:-/home/graepaul/dev_trees/ns/connection-config_quanta-ccs-aus-f15-40.json}" +SYS_NAME="${SYS_NAME:-quanta}" +SYS_LOCATION="${SYS_LOCATION:-REMOTE}" +SYS_SKU="${SYS_SKU:-MI325}" +SYS_PLATFORM="${SYS_PLATFORM:-quanta}" +PLUGINS="${PLUGINS:-OsPlugin}" + +mkdir -p "$OUT_DIR" +export PYTHONPATH="$REPO_ROOT${PYTHONPATH:+:$PYTHONPATH}" + +# node-scraper --sys-name quanta [--lazy-load] --sys-location REMOTE --sys-sku MI325 \ +# --sys-platform quanta --connection-config run-plugins OsPlugin +run_cli_args() { + local lazy_flag=("$@") + echo --sys-name "$SYS_NAME" "${lazy_flag[@]}" \ + --sys-location "$SYS_LOCATION" --sys-sku "$SYS_SKU" --sys-platform "$SYS_PLATFORM" \ + --connection-config "$CONNECTION_CONFIG" run-plugins $PLUGINS +} + +for mode in eager lazy; do + # Pin NODESCRAPER_LAZY_LOAD per mode so a value exported in the caller's shell cannot + # turn the eager (old) run lazy. + lazy_flag=() + lazy_env=0 + if [ "$mode" = lazy ]; then + lazy_flag=(--lazy-load) + lazy_env=1 + fi + bin="$OUT_DIR/cli-$mode.bin" + rm -f "$bin" + echo "=== $mode (NODESCRAPER_LAZY_LOAD=$lazy_env) ===" + # shellcheck disable=SC2046 + env NODESCRAPER_LAZY_LOAD="$lazy_env" \ + python3 -m memray run --force -o "$bin" -m nodescraper.cli.cli $(run_cli_args "${lazy_flag[@]}") + python3 -m memray flamegraph --force -o "$OUT_DIR/cli-$mode-flamegraph.html" "$bin" + python3 -m memray stats "$bin" > "$OUT_DIR/cli-$mode-stats.txt" +done + +echo +echo "Peak memory:" +for mode in eager lazy; do + printf ' %-6s ' "$mode" + grep -m1 -iE "peak memory" "$OUT_DIR/cli-$mode-stats.txt" || echo "n/a (see $OUT_DIR/cli-$mode-stats.txt)" +done +echo "Flamegraphs: $OUT_DIR/cli-{eager,lazy}-flamegraph.html" +echo "Side-by-side: python3 -m memray summary $OUT_DIR/cli-eager.bin; python3 -m memray summary $OUT_DIR/cli-lazy.bin" From 3eac402f6769d7b6e46577785d41771f3cb0d04e Mon Sep 17 00:00:00 2001 From: graepaul Date: Tue, 27 Oct 2026 10:40:29 -0700 Subject: [PATCH 2/6] Update README --- README.md | 23 ++++++++ nodescraper/pluginregistry.py | 107 ++++++++++++++++++++-------------- 2 files changed, 85 insertions(+), 45 deletions(-) diff --git a/README.md b/README.md index 1084308a..67dc6188 100644 --- a/README.md +++ b/README.md @@ -13,6 +13,7 @@ system debug. For details on what data is collected and analyzed, see the [plugi - [3. Install from Source (Required)](#3-install-from-source-required) - [4. Git Hooks (Optional)](#4-git-hooks-optional) - [CLI Usage](#cli-usage) + - [Lazy plugin loading](#lazy-plugin-loading) - [Execution Methods](#execution-methods) - [Example: Remote Execution](#example-remote-execution) - [Example: connection\_config.json](#example-connection_configjson) @@ -167,6 +168,28 @@ options: ``` +### Lazy plugin loading + +By default node-scraper imports every plugin at startup, which takes a couple of seconds and a +noticeable amount of memory even if you run a single plugin. Lazy loading imports a plugin only when +it is first needed, so runs that use a few plugins start faster and use less memory. + +```sh +# enable with a flag ... +node-scraper --lazy-load run-plugins OsPlugin +# ... or with an environment variable (the flag/variable is off by default) +NODESCRAPER_LAZY_LOAD=1 node-scraper run-plugins OsPlugin +``` + +What to expect in lazy mode: +- Plugin names are discovered from the plugin source files without importing them. Plugin files must have `plugin` in the file name and the plugin class must inherit from a base class with `Plugin` in its name (all built-in plugins do). +- Only the plugins you name on the command line (or in a config) are imported. `--help` and `describe plugin` still list every plugin. +- A plugin that fails to import is logged and skipped instead of stopping node-scraper. +- Named configs from `--plugin-configs` are evaluated only when you select them, so recipes such as `AllIbPlugins` (which inspects every plugin) do not import everything at startup. + +When embedding node-scraper, use `PluginRegistry(lazy_load=True)` and share one registry via `set_default_registry()` / `default_registry()` instead of constructing `PluginRegistry()` repeatedly. +`scripts/memray/run_memray.sh` compares memory use of the default and lazy modes. + ### Execution Methods Node Scraper can operate in two modes: LOCAL and REMOTE, determined by the `--sys-location` argument. diff --git a/nodescraper/pluginregistry.py b/nodescraper/pluginregistry.py index 9d587e00..32bbf277 100644 --- a/nodescraper/pluginregistry.py +++ b/nodescraper/pluginregistry.py @@ -66,13 +66,14 @@ def lazy_load_from_env() -> bool: return os.environ.get(LAZY_LOAD_ENV_VAR, "").strip().lower() in ("1", "true", "yes", "on") -class LazyPluginMapping(MutableMapping): +class LazyPluginMapping(MutableMapping[str, type]): """Mapping of plugin name to plugin class that imports each class on first access. - Listing names (``keys()``, ``in``, ``len``, iteration) never imports plugin modules. - ``mapping[name]`` / ``get(name)`` import just that plugin. A plugin that fails to - import or is not valid is logged and treated as missing; other plugins are unaffected. - ``items()`` / ``values()`` / ``copy()`` resolve every plugin. + - ``keys()``, iteration and ``len()`` list every indexed name without importing anything. + - ``mapping[name]``, ``get(name)`` and ``name in mapping`` import just that plugin, so ``in`` + and ``[]`` always agree. A plugin that fails to import, or is not a valid plugin, is logged + once and then behaves as missing; other plugins are unaffected. + - ``items()``, ``values()`` and ``copy()`` resolve every plugin. """ def __init__(self, base_class: type) -> None: @@ -80,7 +81,7 @@ def __init__(self, base_class: type) -> None: self._loaders: dict[str, Callable[[], Optional[type]]] = {} self._resolved: dict[str, type] = {} self._failed: set[str] = set() - self._lock = threading.RLock() + self._lock = threading.Lock() def add_loader(self, name: str, loader: Callable[[], Optional[type]]) -> None: """Register a callable that returns the class for ``name`` when first needed.""" @@ -90,32 +91,44 @@ def add_loader(self, name: str, loader: Callable[[], Optional[type]]) -> None: self._failed.discard(name) def is_resolved(self, name: str) -> bool: + """True if ``name`` has already been imported (does not trigger an import).""" return name in self._resolved def __getitem__(self, name: str) -> type: - resolved = self._resolved.get(name) - if resolved is not None: - return resolved - with self._lock: - if name in self._resolved: - return self._resolved[name] - loader = self._loaders.get(name) - if loader is None or name in self._failed: - raise KeyError(name) - try: - cls = loader() - except Exception as exc: # a broken plugin must not break the others - _logger.warning("Unable to load plugin %s: %s", name, exc) - self._failed.add(name) - raise KeyError(name) from exc - if cls is None or not PluginRegistry._valid_sub_class_check(cls, self._base_class): - self._failed.add(name) - raise KeyError(name) - if hasattr(cls, "is_valid") and not cls.is_valid(): - self._failed.add(name) - raise KeyError(name) - self._resolved[name] = cls + cls = self._resolved.get(name) + if cls is not None: return cls + loader = self._loaders.get(name) + if loader is None or name in self._failed: + raise KeyError(name) + # The import runs outside the lock; two threads may both import, which is harmless. + cls = self._load(name, loader) + with self._lock: + if self._loaders.get(name) is loader: + self._resolved[name] = cls + return cls + + def _load(self, name: str, loader: Callable[[], Optional[type]]) -> type: + """Run ``loader`` and validate the result, marking ``name`` failed if unusable.""" + try: + cls = loader() + except Exception as exc: # a broken plugin must not break the others + _logger.warning("Unable to load plugin %s: %s", name, exc) + self._mark_failed(name) + raise KeyError(name) from exc + if ( + cls is None + or not PluginRegistry._valid_sub_class_check(cls, self._base_class) + or (hasattr(cls, "is_valid") and not cls.is_valid()) + ): + _logger.debug("Plugin %s is not a valid %s", name, self._base_class.__name__) + self._mark_failed(name) + raise KeyError(name) + return cls + + def _mark_failed(self, name: str) -> None: + with self._lock: + self._failed.add(name) def __setitem__(self, name: str, value: type) -> None: with self._lock: @@ -132,7 +145,11 @@ def __delitem__(self, name: str) -> None: self._failed.discard(name) def __contains__(self, name: object) -> bool: - return name in self._loaders and name not in self._failed + try: + self[name] # type: ignore[index] + except KeyError: + return False + return True def __iter__(self) -> Iterator[str]: return iter([n for n in list(self._loaders) if n not in self._failed]) @@ -140,24 +157,19 @@ def __iter__(self) -> Iterator[str]: def __len__(self) -> int: return len(self._loaders) - len(self._failed) - def items(self): # type: ignore[override] - """Resolve all plugins and return ``(name, class)`` pairs, skipping unloadable ones.""" - pairs = [(n, self._try_get(n)) for n in list(self)] - return [(n, cls) for n, cls in pairs if cls is not None] + def items(self) -> list[tuple[str, type]]: # type: ignore[override] + """Resolve every plugin; names that cannot be loaded are skipped.""" + pairs = [(name, self.get(name)) for name in list(self)] + return [(name, cls) for name, cls in pairs if cls is not None] - def values(self): # type: ignore[override] + def values(self) -> list[type]: # type: ignore[override] + """Resolve every plugin; see :meth:`items`.""" return [cls for _, cls in self.items()] def copy(self) -> dict[str, type]: """Return a plain dict of all loadable plugins (resolves every plugin).""" return dict(self.items()) - def _try_get(self, name: str) -> Optional[type]: - try: - return self[name] - except KeyError: - return None - class PluginRegistry: """This class dynamically loads plugins. Internal plugins are loaded by default using @@ -251,6 +263,12 @@ def _init_lazy(self, extra_pkgs: list[types.ModuleType], load_internal: bool) -> lazy_plugins = LazyPluginMapping(PluginInterface) if load_internal: index = PluginRegistry._internal_plugin_index() + if not index: + # Sources are not on the filesystem (zipapp/frozen build): lazy indexing is impossible. + _logger.warning("No plugin sources found to index; falling back to eager loading") + lazy_plugins.update( + PluginRegistry.load_plugins(PluginInterface, [internal_plugins]) + ) for name, (module_name, class_name) in index.items(): lazy_plugins.add_loader( name, @@ -538,17 +556,16 @@ def clear_caches(cls) -> None: Thread-safe: Acquires all locks to ensure no other thread is accessing caches. """ - global _default_registry with cls._cache_lock: cls._entry_point_plugins_cache = None cls._entry_point_connection_managers_cache = None cls._plugin_index_cache = None cls._module_cache.clear() cls._entry_points_cache.clear() - _default_registry = None _default_registry: Optional[PluginRegistry] = None +_default_registry_lock = threading.Lock() def default_registry() -> PluginRegistry: @@ -558,12 +575,12 @@ def default_registry() -> PluginRegistry: library code that are not handed a registry share this one instead of building new ones. """ global _default_registry - with PluginRegistry._cache_lock: + with _default_registry_lock: existing = _default_registry if existing is not None: return existing created = PluginRegistry() - with PluginRegistry._cache_lock: + with _default_registry_lock: if _default_registry is None: _default_registry = created return _default_registry @@ -572,5 +589,5 @@ def default_registry() -> PluginRegistry: def set_default_registry(registry: Optional[PluginRegistry]) -> None: """Install ``registry`` as the shared default (``None`` resets it).""" global _default_registry - with PluginRegistry._cache_lock: + with _default_registry_lock: _default_registry = registry From 9957352205e58815d7a6076741ac3395e8bbf143 Mon Sep 17 00:00:00 2001 From: graepaul Date: Tue, 27 Oct 2026 10:41:16 -0700 Subject: [PATCH 3/6] Remove scripts --- scripts/memray/registry_target.py | 38 ------------------ scripts/memray/run_memray.sh | 66 ------------------------------- 2 files changed, 104 deletions(-) delete mode 100644 scripts/memray/registry_target.py delete mode 100755 scripts/memray/run_memray.sh diff --git a/scripts/memray/registry_target.py b/scripts/memray/registry_target.py deleted file mode 100644 index c4408b6f..00000000 --- a/scripts/memray/registry_target.py +++ /dev/null @@ -1,38 +0,0 @@ -"""Target for memray: build a PluginRegistry in eager or lazy mode and optionally resolve plugins. - -Run through ``run_memray.sh`` (or directly: ``memray run -o out.bin scripts/memray/registry_target.py --mode lazy``). -""" - -import argparse -import sys - -from nodescraper.pluginregistry import PluginRegistry - - -def main() -> None: - parser = argparse.ArgumentParser() - parser.add_argument("--mode", choices=["eager", "lazy"], required=True) - parser.add_argument( - "--plugin", - action="append", - default=[], - help="Plugin name to resolve after building the registry (repeatable). " - "Simulates a run that only uses these plugins.", - ) - args = parser.parse_args() - - registry = PluginRegistry(lazy_load=args.mode == "lazy") - for name in args.plugin: - registry.plugins[name] - - plugin_modules = sum( - 1 for m in sys.modules if m.startswith("nodescraper.plugins.") and m.count(".") >= 3 - ) - sys.stdout.write( - f"mode={args.mode} plugins_listed={len(registry.plugins)} " - f"plugin_modules_imported={plugin_modules} total_modules={len(sys.modules)}\n" - ) - - -if __name__ == "__main__": - main() diff --git a/scripts/memray/run_memray.sh b/scripts/memray/run_memray.sh deleted file mode 100755 index 2226170f..00000000 --- a/scripts/memray/run_memray.sh +++ /dev/null @@ -1,66 +0,0 @@ -#!/usr/bin/env bash -# Compare memory use of the old (eager) and new (--lazy-load) plugin loading with memray, -# running the real node-scraper CLI. -# -# Usage: scripts/memray/run_memray.sh -# -# Override the invocation with env vars (defaults match a typical quanta run): -# CONNECTION_CONFIG path to the connection config JSON -# SYS_NAME, SYS_LOCATION, SYS_SKU, SYS_PLATFORM -# PLUGINS plugin name(s) passed to run-plugins (default: OsPlugin) -# OUT_DIR output directory (default: ./memray_out) -# -# Registry-only comparison (no connection needed): -# memray run -o out.bin scripts/memray/registry_target.py --mode {eager|lazy} --plugin OsPlugin -# -# Requires: pip install memray -set -euo pipefail - -REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/../.." && pwd)" -OUT_DIR="${OUT_DIR:-$PWD/memray_out}" -CONNECTION_CONFIG="${CONNECTION_CONFIG:-/home/graepaul/dev_trees/ns/connection-config_quanta-ccs-aus-f15-40.json}" -SYS_NAME="${SYS_NAME:-quanta}" -SYS_LOCATION="${SYS_LOCATION:-REMOTE}" -SYS_SKU="${SYS_SKU:-MI325}" -SYS_PLATFORM="${SYS_PLATFORM:-quanta}" -PLUGINS="${PLUGINS:-OsPlugin}" - -mkdir -p "$OUT_DIR" -export PYTHONPATH="$REPO_ROOT${PYTHONPATH:+:$PYTHONPATH}" - -# node-scraper --sys-name quanta [--lazy-load] --sys-location REMOTE --sys-sku MI325 \ -# --sys-platform quanta --connection-config run-plugins OsPlugin -run_cli_args() { - local lazy_flag=("$@") - echo --sys-name "$SYS_NAME" "${lazy_flag[@]}" \ - --sys-location "$SYS_LOCATION" --sys-sku "$SYS_SKU" --sys-platform "$SYS_PLATFORM" \ - --connection-config "$CONNECTION_CONFIG" run-plugins $PLUGINS -} - -for mode in eager lazy; do - # Pin NODESCRAPER_LAZY_LOAD per mode so a value exported in the caller's shell cannot - # turn the eager (old) run lazy. - lazy_flag=() - lazy_env=0 - if [ "$mode" = lazy ]; then - lazy_flag=(--lazy-load) - lazy_env=1 - fi - bin="$OUT_DIR/cli-$mode.bin" - rm -f "$bin" - echo "=== $mode (NODESCRAPER_LAZY_LOAD=$lazy_env) ===" - # shellcheck disable=SC2046 - env NODESCRAPER_LAZY_LOAD="$lazy_env" \ - python3 -m memray run --force -o "$bin" -m nodescraper.cli.cli $(run_cli_args "${lazy_flag[@]}") - python3 -m memray flamegraph --force -o "$OUT_DIR/cli-$mode-flamegraph.html" "$bin" - python3 -m memray stats "$bin" > "$OUT_DIR/cli-$mode-stats.txt" -done - -echo -echo "Peak memory:" -for mode in eager lazy; do - printf ' %-6s ' "$mode" - grep -m1 -iE "peak memory" "$OUT_DIR/cli-$mode-stats.txt" || echo "n/a (see $OUT_DIR/cli-$mode-stats.txt)" -done -echo "Flamegraphs: $OUT_DIR/cli-{eager,lazy}-flamegraph.html" -echo "Side-by-side: python3 -m memray summary $OUT_DIR/cli-eager.bin; python3 -m memray summary $OUT_DIR/cli-lazy.bin" From d828aa8a2dc45c14c5ad77c6b323ce9029383027 Mon Sep 17 00:00:00 2001 From: graepaul Date: Tue, 27 Oct 2026 12:26:41 -0700 Subject: [PATCH 4/6] Refactor lazy plugin and then refrence pyproject toml --- README.md | 13 ++--- dev-setup.sh | 2 +- nodescraper/pluginregistry.py | 94 ++++------------------------------- pyproject.toml | 42 ++++++++++++++++ 4 files changed, 58 insertions(+), 93 deletions(-) diff --git a/README.md b/README.md index 67dc6188..d6f0106c 100644 --- a/README.md +++ b/README.md @@ -170,9 +170,8 @@ options: ### Lazy plugin loading -By default node-scraper imports every plugin at startup, which takes a couple of seconds and a -noticeable amount of memory even if you run a single plugin. Lazy loading imports a plugin only when -it is first needed, so runs that use a few plugins start faster and use less memory. +By default node-scraper imports every plugin at startup this can be high consumption to run a single plugin run. +Lazy loading imports a plugin only when it is first needed, so runs that use a few plugins start faster and use less memory. ```sh # enable with a flag ... @@ -182,14 +181,12 @@ NODESCRAPER_LAZY_LOAD=1 node-scraper run-plugins OsPlugin ``` What to expect in lazy mode: -- Plugin names are discovered from the plugin source files without importing them. Plugin files must have `plugin` in the file name and the plugin class must inherit from a base class with `Plugin` in its name (all built-in plugins do). -- Only the plugins you name on the command line (or in a config) are imported. `--help` and `describe plugin` still list every plugin. +- Plugin names are read from the `nodescraper.plugins` entry points in `pyproject.toml` without importing them. Built-in plugins are listed there, and a plugin must be registered there to be available in lazy mode (see [external plugins](docs/node-scraper-external/README.md) for the format). After adding or renaming a plugin, reinstall the package (`pip install -e .`) to refresh the entry-point metadata. +- Only the plugins you name on the command line (or in a config) are imported. +- `describe plugin` lists every plugin from the entry points without importing any; `describe plugin ` imports just that plugin to show its description. `--help` still imports every plugin because it documents each one's arguments. - A plugin that fails to import is logged and skipped instead of stopping node-scraper. - Named configs from `--plugin-configs` are evaluated only when you select them, so recipes such as `AllIbPlugins` (which inspects every plugin) do not import everything at startup. -When embedding node-scraper, use `PluginRegistry(lazy_load=True)` and share one registry via `set_default_registry()` / `default_registry()` instead of constructing `PluginRegistry()` repeatedly. -`scripts/memray/run_memray.sh` compares memory use of the default and lazy modes. - ### Execution Methods Node Scraper can operate in two modes: LOCAL and REMOTE, determined by the `--sys-location` argument. diff --git a/dev-setup.sh b/dev-setup.sh index e3416d54..26aa8fad 100755 --- a/dev-setup.sh +++ b/dev-setup.sh @@ -14,7 +14,7 @@ export UV_SYSTEM_CERTS=1 # Create venv if not already present or if Python version is below 3.9 if [ ! -d "venv" ] || ! venv/bin/python -c 'import sys; exit(0 if sys.version_info >= (3, 9) else 1)' 2>/dev/null; then - uv venv venv --python 3.9 --no-python-downloads + uv venv venv --python 3.9 --no-python-downloads --seed fi # Activate the desired venv diff --git a/nodescraper/pluginregistry.py b/nodescraper/pluginregistry.py index 32bbf277..7f982bb9 100644 --- a/nodescraper/pluginregistry.py +++ b/nodescraper/pluginregistry.py @@ -23,8 +23,6 @@ # SOFTWARE. # ############################################################################### -import ast -import functools import importlib import importlib.metadata import inspect @@ -186,8 +184,6 @@ class PluginRegistry: _module_cache: dict[str, types.ModuleType] = {} # Cache for entry points by group name _entry_points_cache: dict[str, EntryPoints] = {} - # Cache for the lazy-mode plugin name index (name -> (module, class)) - _plugin_index_cache: Optional[dict[str, tuple[str, str]]] = None # Global cache control switch _use_cache: bool = True @@ -255,111 +251,42 @@ def __init__( self.plugins.update(self.load_plugins_from_entry_points()) def _init_lazy(self, extra_pkgs: list[types.ModuleType], load_internal: bool) -> None: - """Lazy setup: index internal plugin names without importing them. + """Lazy setup: register internal plugins from entry points without importing them. Connection managers and result collators are few, so they are still loaded eagerly, but only from their own packages (not by walking every plugin module). """ lazy_plugins = LazyPluginMapping(PluginInterface) + self.plugins = lazy_plugins if load_internal: - index = PluginRegistry._internal_plugin_index() - if not index: - # Sources are not on the filesystem (zipapp/frozen build): lazy indexing is impossible. - _logger.warning("No plugin sources found to index; falling back to eager loading") - lazy_plugins.update( - PluginRegistry.load_plugins(PluginInterface, [internal_plugins]) - ) - for name, (module_name, class_name) in index.items(): - lazy_plugins.add_loader( - name, - functools.partial(PluginRegistry._import_class, module_name, class_name), - ) + self._add_lazy_entry_point_plugins(package_prefix=internal_plugins.__name__ + ".") if extra_pkgs: # Caller-supplied packages have no naming convention to index; load them directly for name, cls in PluginRegistry.load_plugins(PluginInterface, extra_pkgs).items(): lazy_plugins[name] = cls - self.plugins = lazy_plugins non_plugin_pkgs = ([internal_connections] if load_internal else []) + extra_pkgs self.connection_managers = PluginRegistry.load_plugins(ConnectionManager, non_plugin_pkgs) collator_pkgs = ([internal_collators] if load_internal else []) + extra_pkgs self.result_collators = PluginRegistry.load_plugins(PluginResultCollator, collator_pkgs) - def _add_lazy_entry_point_plugins(self) -> None: - """Register entry-point plugins by entry-point name; ``load()`` runs on first access.""" + def _add_lazy_entry_point_plugins(self, package_prefix: Optional[str] = None) -> None: + """Register entry-point plugins; ``load()`` runs on first access. + + If ``package_prefix`` is given, only entry points whose module starts with it are registered. + """ assert isinstance(self.plugins, LazyPluginMapping) for entry_point in PluginRegistry.load_entry_points(ENTRY_POINT_PLUGINS): # Eager mode keys by class __name__, which is the attribute in ``module:Class``. value = getattr(entry_point, "value", "") or "" + if package_prefix and not value.startswith(package_prefix): + continue attr = value.partition(":")[2].strip().split(".")[-1].split("[")[0].strip() self.plugins.add_loader( attr or entry_point.name, entry_point.load, # type: ignore[attr-defined] ) - @staticmethod - def _import_class(module_name: str, class_name: str) -> type: - """Import ``module_name`` (through the shared module cache) and return ``class_name``.""" - with PluginRegistry._cache_lock: - module = PluginRegistry._module_cache.get(module_name) - if module is None: - module = importlib.import_module(module_name) - if PluginRegistry._use_cache: - with PluginRegistry._cache_lock: - PluginRegistry._module_cache[module_name] = module - return getattr(module, class_name) - - @staticmethod - def _base_name(node: ast.expr) -> str: - if isinstance(node, ast.Subscript): - node = node.value - if isinstance(node, ast.Attribute): - return node.attr - if isinstance(node, ast.Name): - return node.id - return "" - - @staticmethod - def _scan_plugin_index(pkg: types.ModuleType) -> dict[str, tuple[str, str]]: - """Map plugin class name to ``(module, class)`` by parsing ``*plugin*.py`` sources. - - Nothing is imported. A class is a candidate if a base class name contains ``Plugin``; - validity is checked when the class is resolved. - """ - index: dict[str, tuple[str, str]] = {} - for root in list(getattr(pkg, "__path__", [])): - for dirpath, dirnames, filenames in os.walk(root): - dirnames[:] = sorted(d for d in dirnames if d != "__pycache__") - for filename in sorted(filenames): - if "plugin" not in filename or not filename.endswith(".py"): - continue - path = os.path.join(dirpath, filename) - rel = os.path.relpath(path, root)[: -len(".py")] - module_name = ".".join([pkg.__name__, *rel.split(os.sep)]) - try: - with open(path, "r", encoding="utf-8") as src: - tree = ast.parse(src.read(), filename=path) - except (OSError, SyntaxError, ValueError) as exc: - _logger.warning("Unable to index plugin module %s: %s", path, exc) - continue - for node in tree.body: - if not isinstance(node, ast.ClassDef): - continue - bases = [PluginRegistry._base_name(b) for b in node.bases] - if any("Plugin" in b for b in bases): - index[node.name] = (module_name, node.name) - return index - - @staticmethod - def _internal_plugin_index() -> dict[str, tuple[str, str]]: - """Cached name index for ``nodescraper.plugins``.""" - with PluginRegistry._cache_lock: - if PluginRegistry._plugin_index_cache is None or not PluginRegistry._use_cache: - PluginRegistry._plugin_index_cache = PluginRegistry._scan_plugin_index( - internal_plugins - ) - return PluginRegistry._plugin_index_cache - @staticmethod def load_plugins( base_class: type, @@ -559,7 +486,6 @@ def clear_caches(cls) -> None: with cls._cache_lock: cls._entry_point_plugins_cache = None cls._entry_point_connection_managers_cache = None - cls._plugin_index_cache = None cls._module_cache.clear() cls._entry_points_cache.clear() diff --git a/pyproject.toml b/pyproject.toml index e4fa338f..35c755b6 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -52,6 +52,48 @@ nodescraper = ["py.typed"] [project.scripts] node-scraper = "nodescraper.cli:cli_entry" +[project.entry-points."nodescraper.plugins"] +AmcRedfishDiagPlugin = "nodescraper.plugins.ooband.amc_redfish_diag.amc_diag_plugin:AmcRedfishDiagPlugin" +AmdSmiFlavorPlugin = "nodescraper.plugins.inband.amdsmi.amdsmi_flavor_plugin:AmdSmiFlavorPlugin" +AmdSmiPlugin = "nodescraper.plugins.inband.amdsmi.amdsmi_plugin:AmdSmiPlugin" +BiosPlugin = "nodescraper.plugins.inband.bios.bios_plugin:BiosPlugin" +CmdlinePlugin = "nodescraper.plugins.inband.cmdline.cmdline_plugin:CmdlinePlugin" +DeviceEnumerationPlugin = "nodescraper.plugins.inband.device_enumeration.device_enumeration_plugin:DeviceEnumerationPlugin" +DimmPlugin = "nodescraper.plugins.inband.dimm.dimm_plugin:DimmPlugin" +DkmsPlugin = "nodescraper.plugins.inband.dkms.dkms_plugin:DkmsPlugin" +DmesgPlugin = "nodescraper.plugins.inband.dmesg.dmesg_plugin:DmesgPlugin" +FabricsPlugin = "nodescraper.plugins.inband.fabrics.fabrics_plugin:FabricsPlugin" +GenericCollectionPlugin = "nodescraper.plugins.generic_collection.inband_plugin:GenericCollectionPlugin" +JournalPlugin = "nodescraper.plugins.inband.journal.journal_plugin:JournalPlugin" +KernelModulePlugin = "nodescraper.plugins.inband.kernel_module.kernel_module_plugin:KernelModulePlugin" +KernelPlugin = "nodescraper.plugins.inband.kernel.kernel_plugin:KernelPlugin" +MemoryPlugin = "nodescraper.plugins.inband.memory.memory_plugin:MemoryPlugin" +Mi4xxServiceabilityPlugin = "nodescraper.plugins.serviceability.mi4xx.serviceability_plugin_mi4xx:Mi4xxServiceabilityPlugin" +NetworkPlugin = "nodescraper.plugins.inband.network.network_plugin:NetworkPlugin" +NicPlugin = "nodescraper.plugins.inband.nic.nic_plugin:NicPlugin" +NvmePlugin = "nodescraper.plugins.inband.nvme.nvme_plugin:NvmePlugin" +OobBmcArchivePlugin = "nodescraper.plugins.ooband.bmc_archive.bmc_archive_plugin:OobBmcArchivePlugin" +OobGenericCollectionPlugin = "nodescraper.plugins.generic_collection.oob_plugin:OobGenericCollectionPlugin" +OsPlugin = "nodescraper.plugins.inband.os.os_plugin:OsPlugin" +PackagePlugin = "nodescraper.plugins.inband.package.package_plugin:PackagePlugin" +PciePlugin = "nodescraper.plugins.inband.pcie.pcie_plugin:PciePlugin" +ProcessPlugin = "nodescraper.plugins.inband.process.process_plugin:ProcessPlugin" +RdmaPlugin = "nodescraper.plugins.inband.rdma.rdma_plugin:RdmaPlugin" +RedfishEndpointPlugin = "nodescraper.plugins.ooband.redfish_endpoint.endpoint_plugin:RedfishEndpointPlugin" +RedfishOemDiagPlugin = "nodescraper.plugins.ooband.redfish_oem_diag.oem_diag_plugin:RedfishOemDiagPlugin" +RegexSearchPlugin = "nodescraper.plugins.inband.regex_search.regex_search_plugin:RegexSearchPlugin" +RocmPlugin = "nodescraper.plugins.inband.rocm.rocm_plugin:RocmPlugin" +ScaleOutAristaPlugin = "nodescraper.plugins.inband.switch.scale_out_arista.scale_out_arista_plugin:ScaleOutAristaPlugin" +ScaleOutDellPlugin = "nodescraper.plugins.inband.switch.scale_out_dell.scale_out_dell_plugin:ScaleOutDellPlugin" +ScaleUpSonicPlugin = "nodescraper.plugins.inband.switch.scale_up_sonic.scale_up_sonic_plugin:ScaleUpSonicPlugin" +ServiceabilityPluginBase = "nodescraper.plugins.serviceability.serviceability_plugin_base:ServiceabilityPluginBase" +ServiceabilityPluginMI3XX = "nodescraper.plugins.serviceability.mi3xx.serviceability_plugin_mi3xx:ServiceabilityPluginMI3XX" +StoragePlugin = "nodescraper.plugins.inband.storage.storage_plugin:StoragePlugin" +SysSettingsPlugin = "nodescraper.plugins.inband.sys_settings.sys_settings_plugin:SysSettingsPlugin" +SysctlPlugin = "nodescraper.plugins.inband.sysctl.sysctl_plugin:SysctlPlugin" +SyslogPlugin = "nodescraper.plugins.inband.syslog.syslog_plugin:SyslogPlugin" +UptimePlugin = "nodescraper.plugins.inband.uptime.uptime_plugin:UptimePlugin" + [project.entry-points."nodescraper.plugin_configs"] NodeStatus = "nodescraper.pluginrecipe.node_status:NodeStatus" AllIbPlugins = "nodescraper.pluginrecipe.all_ib_plugins:AllIbPlugins" From 3b61b70ea5a1845ea100a28a5e0d6eba4bdc414d Mon Sep 17 00:00:00 2001 From: graepaul Date: Wed, 28 Oct 2026 07:44:18 -0700 Subject: [PATCH 5/6] Adding lazy plugin reg test --- .../framework/test_plugin_registry_lazy.py | 161 ++++++++++++++++++ 1 file changed, 161 insertions(+) create mode 100644 test/unit/framework/test_plugin_registry_lazy.py diff --git a/test/unit/framework/test_plugin_registry_lazy.py b/test/unit/framework/test_plugin_registry_lazy.py new file mode 100644 index 00000000..969f313d --- /dev/null +++ b/test/unit/framework/test_plugin_registry_lazy.py @@ -0,0 +1,161 @@ +############################################################################### +# +# MIT License +# +# Copyright (C) 2026 Advanced Micro Devices, Inc. +# +# Permission is hereby granted, free of charge, to any person obtaining a copy +# of this software and associated documentation files (the "Software"), to deal +# in the Software without restriction, including without limitation the rights +# to use, copy, modify, merge, publish, distribute, sublicense, and/or sell +# copies of the Software, and to permit persons to whom the Software is +# furnished to do so, subject to the following conditions: +# +# The above copyright notice and this permission notice shall be included in all +# copies or substantial portions of the Software. +# +# THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR +# IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, +# FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE +# AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER +# LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, +# OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE +# SOFTWARE. +# +############################################################################### +from unittest.mock import MagicMock, patch + +import pytest + +import nodescraper.plugins as internal_plugins +from nodescraper.interfaces import PluginInterface +from nodescraper.pluginregistry import LazyPluginMapping, PluginRegistry +from nodescraper.plugins.inband.bios.bios_plugin import BiosPlugin +from nodescraper.plugins.inband.kernel.kernel_plugin import KernelPlugin + +BIOS_VALUE = "nodescraper.plugins.inband.bios.bios_plugin:BiosPlugin" +KERNEL_VALUE = "nodescraper.plugins.inband.kernel.kernel_plugin:KernelPlugin" + + +@pytest.fixture(autouse=True) +def clear_cache(): + PluginRegistry.clear_caches() + yield + PluginRegistry.clear_caches() + + +def _entry_point(name, value, loaded): + ep = MagicMock() + ep.name = name + ep.value = value + if isinstance(loaded, Exception): + ep.load.side_effect = loaded + else: + ep.load.return_value = loaded + return ep + + +def _patch_plugin_entry_points(eps): + def side_effect(*args, **kwargs): + return eps if kwargs.get("group") == "nodescraper.plugins" else [] + + return patch( + "nodescraper.pluginregistry.importlib.metadata.entry_points", side_effect=side_effect + ) + + +def test_lazy_registry_uses_lazy_mapping(): + reg = PluginRegistry(lazy_load=True) + assert reg.lazy + assert isinstance(reg.plugins, LazyPluginMapping) + + +def test_lazy_internal_plugins_match_eager_scan(): + """Every internal plugin must be declared in the ``nodescraper.plugins`` entry points.""" + lazy = PluginRegistry( + lazy_load=True, load_entry_point_plugins=False, load_entry_point_connection_managers=False + ) + eager = PluginRegistry.load_plugins(PluginInterface, [internal_plugins]) + assert sorted(lazy.plugins) == sorted(eager) + for name, cls in eager.items(): + assert lazy.plugins[name] is cls + + +def test_lazy_entry_points_not_loaded_until_accessed(): + bios_ep = _entry_point("BiosPlugin", BIOS_VALUE, BiosPlugin) + kernel_ep = _entry_point("KernelPlugin", KERNEL_VALUE, KernelPlugin) + with _patch_plugin_entry_points([bios_ep, kernel_ep]): + reg = PluginRegistry( + lazy_load=True, + load_entry_point_connection_managers=False, + ) + assert sorted(reg.plugins) == ["BiosPlugin", "KernelPlugin"] + bios_ep.load.assert_not_called() + kernel_ep.load.assert_not_called() + + assert reg.plugins["BiosPlugin"] is BiosPlugin + bios_ep.load.assert_called_once() + kernel_ep.load.assert_not_called() + assert reg.plugins.is_resolved("BiosPlugin") + assert not reg.plugins.is_resolved("KernelPlugin") + + assert reg.plugins["BiosPlugin"] is BiosPlugin + bios_ep.load.assert_called_once() + + +def test_lazy_internal_only_skips_external_entry_points(): + bios_ep = _entry_point("BiosPlugin", BIOS_VALUE, BiosPlugin) + external_ep = _entry_point("ExtPlugin", "ext_pkg.ext_plugin:ExtPlugin", KernelPlugin) + with _patch_plugin_entry_points([bios_ep, external_ep]): + reg = PluginRegistry( + lazy_load=True, + load_entry_point_plugins=False, + load_entry_point_connection_managers=False, + ) + assert list(reg.plugins) == ["BiosPlugin"] + external_ep.load.assert_not_called() + + +def test_lazy_external_entry_point_keyed_by_class_name(): + external_ep = _entry_point("alias", "ext_pkg.ext_plugin:KernelPlugin", KernelPlugin) + with _patch_plugin_entry_points([external_ep]): + reg = PluginRegistry( + lazy_load=True, + load_internal_plugins=False, + load_entry_point_connection_managers=False, + ) + assert list(reg.plugins) == ["KernelPlugin"] + assert reg.plugins["KernelPlugin"] is KernelPlugin + + +def test_lazy_broken_entry_point_does_not_affect_others(): + broken_ep = _entry_point( + "BrokenPlugin", "nodescraper.plugins.nope:BrokenPlugin", ImportError("boom") + ) + bios_ep = _entry_point("BiosPlugin", BIOS_VALUE, BiosPlugin) + with _patch_plugin_entry_points([broken_ep, bios_ep]): + reg = PluginRegistry(lazy_load=True, load_entry_point_connection_managers=False) + assert "BrokenPlugin" not in reg.plugins + assert "BrokenPlugin" not in list(reg.plugins) + assert reg.plugins["BiosPlugin"] is BiosPlugin + with pytest.raises(KeyError): + reg.plugins["BrokenPlugin"] + + +def test_lazy_non_plugin_entry_point_is_treated_as_missing(): + bad_ep = _entry_point("NotAPlugin", "nodescraper.plugins.x:NotAPlugin", dict) + with _patch_plugin_entry_points([bad_ep]): + reg = PluginRegistry( + lazy_load=True, + load_internal_plugins=False, + load_entry_point_connection_managers=False, + ) + assert "NotAPlugin" not in reg.plugins + assert len(reg.plugins) == 0 + + +def test_lazy_load_env_var(monkeypatch): + monkeypatch.setenv("NODESCRAPER_LAZY_LOAD", "1") + assert PluginRegistry(load_entry_point_plugins=False).lazy + monkeypatch.setenv("NODESCRAPER_LAZY_LOAD", "0") + assert not PluginRegistry(load_entry_point_plugins=False).lazy From 343d746ff601eaa713ed69153d5d5b616f3585dc Mon Sep 17 00:00:00 2001 From: graepaul Date: Fri, 30 Oct 2026 10:45:05 -0700 Subject: [PATCH 6/6] Remove all os.env usage and remove all mentions of lazy load env var --- README.md | 5 +---- nodescraper/cli/cli.py | 13 ++++--------- nodescraper/pluginregistry.py | 18 ++++-------------- .../framework/test_plugin_registry_lazy.py | 7 ------- 4 files changed, 9 insertions(+), 34 deletions(-) diff --git a/README.md b/README.md index d6f0106c..52bdfbef 100644 --- a/README.md +++ b/README.md @@ -170,14 +170,11 @@ options: ### Lazy plugin loading -By default node-scraper imports every plugin at startup this can be high consumption to run a single plugin run. +By default node-scraper imports every plugin at startup this can be inefficient to run a single plugin run. Lazy loading imports a plugin only when it is first needed, so runs that use a few plugins start faster and use less memory. ```sh -# enable with a flag ... node-scraper --lazy-load run-plugins OsPlugin -# ... or with an environment variable (the flag/variable is off by default) -NODESCRAPER_LAZY_LOAD=1 node-scraper run-plugins OsPlugin ``` What to expect in lazy mode: diff --git a/nodescraper/cli/cli.py b/nodescraper/cli/cli.py index 7240c413..cb917120 100644 --- a/nodescraper/cli/cli.py +++ b/nodescraper/cli/cli.py @@ -73,11 +73,7 @@ from nodescraper.models.pluginresult import PluginResult from nodescraper.pluginexecutor import PluginExecutor from nodescraper.pluginrecipe.discovery import PluginDiscovery -from nodescraper.pluginregistry import ( - LAZY_LOAD_ENV_VAR, - PluginRegistry, - set_default_registry, -) +from nodescraper.pluginregistry import PluginRegistry, set_default_registry def _parse_plugin_configs_csv(value: str) -> list[str]: @@ -213,8 +209,7 @@ def _add_cli_root_globals( "--lazy-load", dest="lazy_load", action="store_true", - help=f"Import plugin modules only when they are needed instead of at startup " - f"(can also be enabled with the {LAZY_LOAD_ENV_VAR}=1 environment variable)", + help="Import plugin modules only when they are needed instead of at startup", ) @@ -501,7 +496,7 @@ def _lazy_load_flag_present(arg_input: Sequence[str]) -> bool: """True if ``--lazy-load`` appears among the top-level args (before ``run-plugins``). The registry must exist before the real parser is built, so this checks for the flag - on its own. ``NODESCRAPER_LAZY_LOAD`` is handled by the registry itself. + on its own. """ top_level = list(arg_input) if "run-plugins" in top_level: @@ -546,7 +541,7 @@ def main( arg_input = sys.argv[1:] # The one registry for this invocation: shared with recipes/discovery and the executor. - plugin_reg = PluginRegistry(lazy_load=True if _lazy_load_flag_present(arg_input) else None) + plugin_reg = PluginRegistry(lazy_load=_lazy_load_flag_present(arg_input)) set_default_registry(plugin_reg) PluginDiscovery.clear_cache() lazy = bool(getattr(plugin_reg, "lazy", False)) diff --git a/nodescraper/pluginregistry.py b/nodescraper/pluginregistry.py index 7f982bb9..5b190f44 100644 --- a/nodescraper/pluginregistry.py +++ b/nodescraper/pluginregistry.py @@ -27,7 +27,6 @@ import importlib.metadata import inspect import logging -import os import pkgutil import threading import types @@ -53,17 +52,9 @@ ENTRY_POINT_PLUGINS = "nodescraper.plugins" ENTRY_POINT_CONNECTION_MANAGERS = "nodescraper.connection_managers" -# Environment variable enabling lazy plugin loading when ``lazy_load`` is not given explicitly -LAZY_LOAD_ENV_VAR = "NODESCRAPER_LAZY_LOAD" - _logger = logging.getLogger(__name__) -def lazy_load_from_env() -> bool: - """Return True if ``NODESCRAPER_LAZY_LOAD`` is set to a truthy value (1/true/yes/on).""" - return os.environ.get(LAZY_LOAD_ENV_VAR, "").strip().lower() in ("1", "true", "yes", "on") - - class LazyPluginMapping(MutableMapping[str, type]): """Mapping of plugin name to plugin class that imports each class on first access. @@ -197,7 +188,7 @@ def __init__( load_internal_plugins: bool = True, load_entry_point_plugins: bool = True, load_entry_point_connection_managers: bool = True, - lazy_load: Optional[bool] = None, + lazy_load: bool = False, ) -> None: """Initialize the PluginRegistry with optional plugin packages. @@ -207,11 +198,10 @@ def __init__( load_entry_point_plugins (bool, optional): Whether to load plugins from entry points. Defaults to True. load_entry_point_connection_managers (bool, optional): Whether to load connection managers from the ``nodescraper.connection_managers`` entry-point group. Defaults to True. - lazy_load (Optional[bool], optional): If True, ``plugins`` is a :class:`LazyPluginMapping` and plugin - modules are imported only when a plugin is first accessed. If None, the - ``NODESCRAPER_LAZY_LOAD`` environment variable decides (default: eager). + lazy_load (bool, optional): If True, ``plugins`` is a :class:`LazyPluginMapping` and plugin + modules are imported only when a plugin is first accessed. Defaults to False. """ - self.lazy: bool = lazy_load_from_env() if lazy_load is None else lazy_load + self.lazy: bool = lazy_load if load_internal_plugins: self.plugin_pkg = [ diff --git a/test/unit/framework/test_plugin_registry_lazy.py b/test/unit/framework/test_plugin_registry_lazy.py index 969f313d..92f17270 100644 --- a/test/unit/framework/test_plugin_registry_lazy.py +++ b/test/unit/framework/test_plugin_registry_lazy.py @@ -152,10 +152,3 @@ def test_lazy_non_plugin_entry_point_is_treated_as_missing(): ) assert "NotAPlugin" not in reg.plugins assert len(reg.plugins) == 0 - - -def test_lazy_load_env_var(monkeypatch): - monkeypatch.setenv("NODESCRAPER_LAZY_LOAD", "1") - assert PluginRegistry(load_entry_point_plugins=False).lazy - monkeypatch.setenv("NODESCRAPER_LAZY_LOAD", "0") - assert not PluginRegistry(load_entry_point_plugins=False).lazy