From 1b8dc518c8ce5479573144721746a3b583abf9b3 Mon Sep 17 00:00:00 2001 From: Julian Geiger Date: Wed, 12 Nov 2025 12:36:04 +0100 Subject: [PATCH 1/2] support monitors without version --- .../engine/processes/calcjobs/calcjob.py | 2 +- .../processes/calcjobs/test_calc_job.py | 25 +++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/src/aiida/engine/processes/calcjobs/calcjob.py b/src/aiida/engine/processes/calcjobs/calcjob.py index e2d2bf24f8..b79f9ac8e6 100644 --- a/src/aiida/engine/processes/calcjobs/calcjob.py +++ b/src/aiida/engine/processes/calcjobs/calcjob.py @@ -647,7 +647,7 @@ def _setup_version_info(self) -> dict[str, Any]: entry_point = monitor.base.attributes.get('entry_point') entry_point_string = format_entry_point_string('aiida.calculations.monitors', entry_point) monitor_version_info = self.runner.plugin_version_provider.get_version_info(entry_point_string) - version_info['version'].setdefault('monitors', {})[key] = monitor_version_info['version']['plugin'] + version_info['version'].setdefault('monitors', {})[key] = monitor_version_info['version'].get('plugin') cache_version_info = {} diff --git a/tests/engine/processes/calcjobs/test_calc_job.py b/tests/engine/processes/calcjobs/test_calc_job.py index fda1b99e81..4dadebf8bc 100644 --- a/tests/engine/processes/calcjobs/test_calc_job.py +++ b/tests/engine/processes/calcjobs/test_calc_job.py @@ -1195,6 +1195,31 @@ def test_monitor_version(get_calcjob_builder): assert node.base.attributes.get('version')['monitors'] == {'monitor': __version__} +def empty_monitor(node, transport): + """Empty monitor that returns None and has no __version__ in its module.""" + return None + + +def test_monitor_without_version(get_calcjob_builder, entry_points): + """Test that monitors without version information don't cause a KeyError. + + This test ensures that monitors from packages without a __version__ attribute + can be used without raising a KeyError when setting up version info. + """ + entry_points.add(empty_monitor, group='aiida.calculations.monitors', name='core.empty_monitor') + + builder = get_calcjob_builder() + builder.monitors = {'monitor': orm.Dict({'entry_point': 'core.empty_monitor'})} + _, node = launch.run_get_node(builder) + + # The monitor should be in the version info, but with None as the version + # since the module doesn't have __version__ + assert 'monitors' in node.base.attributes.get('version') + assert 'monitor' in node.base.attributes.get('version')['monitors'] + # The value should be None since the module doesn't have __version__ + assert node.base.attributes.get('version')['monitors']['monitor'] is None + + def monitor_skip_parse(node, transport, **kwargs): """Kill the job and skip the parsing of retrieved output files.""" return CalcJobMonitorResult(message='skip parsing', parse=False) From 8802e683cdf54572f9a15d860800ce6496a302fe Mon Sep 17 00:00:00 2001 From: Julian Geiger Date: Tue, 25 Nov 2025 10:59:27 +0100 Subject: [PATCH 2/2] fix tests --- .../engine/processes/calcjobs/calcjob.py | 9 +- src/aiida/engine/processes/calcjobs/tasks.py | 3 + .../processes/calcjobs/test_calc_job.py | 90 +++++++++++++++---- 3 files changed, 86 insertions(+), 16 deletions(-) diff --git a/src/aiida/engine/processes/calcjobs/calcjob.py b/src/aiida/engine/processes/calcjobs/calcjob.py index b79f9ac8e6..42f866733a 100644 --- a/src/aiida/engine/processes/calcjobs/calcjob.py +++ b/src/aiida/engine/processes/calcjobs/calcjob.py @@ -647,7 +647,14 @@ def _setup_version_info(self) -> dict[str, Any]: entry_point = monitor.base.attributes.get('entry_point') entry_point_string = format_entry_point_string('aiida.calculations.monitors', entry_point) monitor_version_info = self.runner.plugin_version_provider.get_version_info(entry_point_string) - version_info['version'].setdefault('monitors', {})[key] = monitor_version_info['version'].get('plugin') + plugin_version = monitor_version_info['version'].get('plugin') + if plugin_version is None: + self.logger.warning( + f"Monitor '{key}' (entry point '{entry_point}') does not have version information. " + f'The package does not define __version__. Provenance tracking will be incomplete. ' + f'Consider adding __version__ to ensure reproducibility.' + ) + version_info['version'].setdefault('monitors', {})[key] = plugin_version cache_version_info = {} diff --git a/src/aiida/engine/processes/calcjobs/tasks.py b/src/aiida/engine/processes/calcjobs/tasks.py index 9c00f486ae..18565b3539 100644 --- a/src/aiida/engine/processes/calcjobs/tasks.py +++ b/src/aiida/engine/processes/calcjobs/tasks.py @@ -243,6 +243,9 @@ async def task_monitor_job( """ state = node.get_state() + # import traceback + # traceback.print_stack() + # breakpoint() if state in [CalcJobState.RETRIEVING, CalcJobState.STASHING]: logger.warning(f'CalcJob<{node.pk}> already marked as `{state}`, skipping task_monitor_job') return None diff --git a/tests/engine/processes/calcjobs/test_calc_job.py b/tests/engine/processes/calcjobs/test_calc_job.py index 4dadebf8bc..36d90f0c34 100644 --- a/tests/engine/processes/calcjobs/test_calc_job.py +++ b/tests/engine/processes/calcjobs/test_calc_job.py @@ -1195,29 +1195,89 @@ def test_monitor_version(get_calcjob_builder): assert node.base.attributes.get('version')['monitors'] == {'monitor': __version__} -def empty_monitor(node, transport): - """Empty monitor that returns None and has no __version__ in its module.""" - return None - - def test_monitor_without_version(get_calcjob_builder, entry_points): """Test that monitors without version information don't cause a KeyError. This test ensures that monitors from packages without a __version__ attribute - can be used without raising a KeyError when setting up version info. + can be used without raising a KeyError when setting up version info. A warning + is emitted to inform the user about incomplete provenance tracking. + """ + import sys + import types + import uuid + + # Create a monitor function + def empty_monitor(node, transport): + """Empty monitor that returns None and has no __version__ in its module.""" + return None + + # Create a dynamic module without __version__ attribute + module_name = f'test_monitor_module_{uuid.uuid4().hex[:8]}' + dynamic_module = types.ModuleType(module_name, 'Test module without version') + + # Set the monitor's module to the dynamic module + empty_monitor.__module__ = module_name + setattr(dynamic_module, 'empty_monitor', empty_monitor) + + # Make the module importable (but without __version__) + sys.modules[module_name] = dynamic_module + + try: + # Register the monitor as an entry point + entry_points.add(empty_monitor, group='aiida.calculations.monitors', name='core.empty_monitor') + + builder = get_calcjob_builder() + builder.monitors = {'monitor': orm.Dict({'entry_point': 'core.empty_monitor'})} + _, node = launch.run_get_node(builder) + + # The monitor should be in the version info, but with None as the version + # since the module doesn't have __version__ + assert 'monitors' in node.base.attributes.get('version') + assert 'monitor' in node.base.attributes.get('version')['monitors'] + # The value should be None since the module doesn't have __version__ + assert node.base.attributes.get('version')['monitors']['monitor'] is None + + # Note: A warning is emitted to inform users about incomplete provenance tracking, + # but it comes from the plumpy.processes logger which is not captured by pytest's + # caplog fixture. The warning can be seen in stderr during test execution. + finally: + # Clean up the dynamically created module + sys.modules.pop(module_name, None) + + +def test_monitor_requires_entry_point(get_calcjob_builder): + """Test that monitors MUST be registered as entry points and cannot be simple functions. + + This test demonstrates that the current design requires all monitors to be registered + as entry points. Attempting to pass a monitor without an entry point should fail + during validation at builder assignment time. """ - entry_points.add(empty_monitor, group='aiida.calculations.monitors', name='core.empty_monitor') + + def simple_monitor(node, transport): + """A simple monitor function that we try to use without an entry point.""" + return None builder = get_calcjob_builder() - builder.monitors = {'monitor': orm.Dict({'entry_point': 'core.empty_monitor'})} - _, node = launch.run_get_node(builder) - # The monitor should be in the version info, but with None as the version - # since the module doesn't have __version__ - assert 'monitors' in node.base.attributes.get('version') - assert 'monitor' in node.base.attributes.get('version')['monitors'] - # The value should be None since the module doesn't have __version__ - assert node.base.attributes.get('version')['monitors']['monitor'] is None + # Try to pass a monitor without an entry_point - this should fail validation + # because CalcJobMonitor requires 'entry_point' to be present + # The ValueError is raised during builder assignment, not during execution + with pytest.raises(ValueError, match=r".*unexpected keyword argument 'function'.*"): + builder.monitors = {'monitor': orm.Dict({'function': simple_monitor})} + + +def test_monitor_invalid_entry_point(get_calcjob_builder): + """Test that monitors with non-existent entry points fail validation. + + This ensures that the entry point must exist and be loadable. + Validation happens at builder assignment time. + """ + builder = get_calcjob_builder() + + # Try to use a non-existent entry point + # The ValueError is raised during builder assignment, not during execution + with pytest.raises(ValueError, match=r'.*not found in group.*'): + builder.monitors = {'monitor': orm.Dict({'entry_point': 'non.existent.monitor'})} def monitor_skip_parse(node, transport, **kwargs):