Fix monitor ratemeter workflow to use two-phase workflow registration #536
+155
−129
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ration Split monitor_interval_timeseries workflow registration into the two-phase spec+factory pattern for consistency with the rest of the codebase: Phase 1 (Spec registration - lightweight, in Instrument.__post_init__): - Added register_monitor_interval_timeseries_spec() to monitor_workflow_specs.py - Defines MonitorTimeseriesParams and MonitorTimeseriesOutputs models - Only registers when nexus file is available (required by GenericTofWorkflow) Phase 2 (Factory attachment - heavyweight, in Instrument.load_factories): - Added create_monitor_interval_timeseries_factory() to monitor_data_handler.py - Contains workflow logic with ess.reduce dependencies - Automatically attached for all instruments with nexus geometry Key improvements: - Fixed type safety: MonitorTimeseriesOutputs now uses sc.DataArray instead of Any - Uses WorkflowOutputsBase for consistency with other workflows (bifrost, dream, etc.) - Removed old-style registration from workflows.py (now just has TimeseriesAccumulator) - Conditional registration based on nexus file availability All tests pass (1407 passed, 28 skipped). Original prompt: Please review uncommitted changes, splitting broken (old style) workflow registration into new spec+factory pattern. Is this correct? Follow-up: Please fix as you suggest! Follow-up: Great, please commit 🤖 Generated with Claude Code Co-Authored-By: Claude <[email protected]>
The guards in register_monitor_workflow_specs and register_timeseries_workflow_specs checked if a workflow was already registered and returned early to avoid duplicate registration errors. This was defensive programming for a scenario that never occurs in practice - no instrument specs.py calls these registration functions after Instrument.__post_init__() auto-registers them. Simplify the code by removing: - The idempotency checks and WorkflowId construction in both functions - References to "two-phase registration" in docstrings - Unused inline SpecHandle imports in timeseries_workflow_specs.py All tests pass. Functionality is unchanged. Help me figure out why the "guard" in register_monitor_workflow_specs checking if the workflow is already registered is necessary? Why would there be sth. else registering the same workflow?
SimonHeybrock
changed the title
SimonHeybrock
changed the title
Replace the dedicated availability check with a try/except/else block that uses the nexus_file property directly. This eliminates code duplication and ensures the property's caching behavior in _nexus_file is used consistently. Original prompt: Please see diff to main, can we avoid adding _can_load_nexus_file, or at least avoid duplication by using it in the nexus_file property? 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR refactors the monitor interval timeseries workflow to use two-phase registration and renames it to MonitorRatemeter. It was broken recently and I only noticed now.
Key changes
config/workflows.pytohandlers/monitor_data_handler.pyregister_monitor_ratemeter_spec()for phase 1 (spec registration)create_monitor_ratemeter_factory()for phase 2 (factory attachment)Instrument.__post_init__()for instruments with nexus files -> I think this will be useful for multiple instruments, patterns mirrors registration of regular monitor workflows.