Add Prefect v3 workflow orchestration with verified persistent hardware connection support#4
Add Prefect v3 workflow orchestration with verified persistent hardware connection support#4
Conversation
Co-authored-by: sgbaird <45469701+sgbaird@users.noreply.github.com>
Co-authored-by: sgbaird <45469701+sgbaird@users.noreply.github.com>
Co-authored-by: sgbaird <45469701+sgbaird@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
This PR adds Prefect v3 workflow orchestration as a more robust, production-ready alternative to the existing REST API for controlling the Plate Resort device. The REST API remains fully functional for backward compatibility while Prefect provides enhanced features like automatic retry logic, observability through its UI, async execution, and better state management.
Key changes:
- Complete Prefect implementation following the simpler process-based work pool pattern
- New control interface via remote flow execution without requiring exposed HTTP endpoints
- Enhanced documentation with setup guides and architecture diagrams
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pyproject.toml | Added Prefect>=3.0.0 dependency |
| prefect_flows/device.py | 11 Prefect flows wrapping all plate resort operations (connect, status, hotel control, etc.) |
| prefect_flows/orchestrator.py | Remote API for submitting flow runs to the work pool |
| prefect_flows/deploy_flows.py | Helper script to deploy all flows to work pool in one command |
| prefect_flows/init.py | Package exports for flow functions |
| prefect_flows/README.md | Comprehensive setup guide with architecture diagram |
| README.md | Updated to highlight Prefect as recommended approach while maintaining REST API documentation |
| CHANGELOG.md | Added v2.1.0 entry documenting the new Prefect integration |
| return run_deployment(name="get-position/plate-resort-get-position") | ||
|
|
||
|
|
||
| if __name__ == "__main__": |
There was a problem hiding this comment.
This if __name__ == "__main__" pattern violates the coding guideline to avoid such patterns in package code. Consider removing this block or moving the example usage to documentation.
There was a problem hiding this comment.
Done in 9b9f50d. Removed the if name == "main" block from orchestrator.py.
| return get_position_task() | ||
|
|
||
|
|
||
| if __name__ == "__main__": |
There was a problem hiding this comment.
This if __name__ == "__main__" pattern violates the coding guideline to avoid such patterns in package code. Consider removing this block or moving the usage instructions to documentation.
There was a problem hiding this comment.
Fine, just improves discoverability from a package perspective. Note to self: might go away later if the legacy code is gone
| ) | ||
|
|
||
|
|
||
| def deploy_all(work_pool_name: str = "plate-resort-pool"): |
There was a problem hiding this comment.
Unwrap and make this a top-level script without the if name == main pattern. I don't expect people to be reusing this code as a module
There was a problem hiding this comment.
Done in 9b9f50d. Removed the function wrapper and if name == "main" pattern. The script now runs top-level code directly.
| print("-" * 60) | ||
|
|
||
| for flow, deployment_name in flows: | ||
| try: |
There was a problem hiding this comment.
I don't like the try except here. Let error bubble naturally so we can debug.
There was a problem hiding this comment.
Done in 9b9f50d. Removed try/except block - errors now bubble naturally for debugging.
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| work_pool = sys.argv[1] if len(sys.argv) > 1 else "plate-resort-pool" |
There was a problem hiding this comment.
Hmm.. do we really need this CLI? Let's remove for now. I could see this being a slight help, but remove for now.
There was a problem hiding this comment.
Done in 9b9f50d. Removed CLI argument parsing and sys.argv logic. Script now uses a hardcoded work pool name.
| import os | ||
| from prefect import flow, task | ||
|
|
||
| sys.path.append(os.path.join(os.path.dirname(__file__), '..')) |
There was a problem hiding this comment.
Necessary? Did you try this out and it turned out to be necessary? Verify
There was a problem hiding this comment.
Removed in 9b9f50d. The sys.path.append was unnecessary since the package structure already makes plate_resort importable.
|
|
||
| def get_resort(): | ||
| """Get or create the PlateResort instance""" | ||
| global resort_instance |
There was a problem hiding this comment.
Thoughts on this use of global pattern across this file? Seem ok?
| from prefect import flow, task | ||
|
|
||
| sys.path.append(os.path.join(os.path.dirname(__file__), '..')) | ||
| from plate_resort.core import PlateResort |
There was a problem hiding this comment.
Note to self: maybe we instead want to modify the PlateResort class itself, decorating the class methods.
There was a problem hiding this comment.
I think this gets the point across, but I think we could also just modify the plate resort class directly. Look up prefect docs using your #context7 MCP tool. Maybe decorating class methods isn't the right pattern, but this file, while readable, could benefit from DRY principle. Hence suggestion to modify the package itself
| return run_deployment(name="get-position/plate-resort-get-position") | ||
|
|
||
|
|
||
| if __name__ == "__main__": |
…deploy script Co-authored-by: sgbaird <45469701+sgbaird@users.noreply.github.com>
Done in 9b9f50d. Applied DRY principle throughout:
|
| work_pool_name = "plate-resort-pool" | ||
|
|
||
| # Get a resort instance to access flow methods | ||
| resort = PlateResort() |
There was a problem hiding this comment.
How is this going to work from a singleton perspective, keeping in mind this is tied to physical hardware?
There was a problem hiding this comment.
Fixed in 806ae9f. Now uses flow.from_source() to deploy flows without instantiating PlateResort, avoiding hardware initialization during deployment. This is safe to run from any machine.
Co-authored-by: sgbaird <45469701+sgbaird@users.noreply.github.com>
There was a problem hiding this comment.
@copilot how does this work from a singleton perspective, keeping in mind there's a single physical instrument that probably has a state or connection of some sort? (Not sure, verify if it's a stateful workflow or not). Use your #context7 MCP tool to look up prefect docs
There was a problem hiding this comment.
The workflow is stateful - PlateResort maintains connection state (self.port, self.packet_handler) and position state (self.current_hotel).
Each flow run creates a new PlateResort instance when using flow.from_source(), which is correct. The hardware connection is established in the connect() flow (not init), so deployment doesn't initialize hardware.
However, to prevent concurrent access to the physical hardware, you should add a concurrency limit:
from prefect import flow, concurrency
@flow(name="activate-hotel")
def activate_hotel(self, hotel, tolerance=None, timeout=None):
with concurrency("plate-resort-hardware", occupy=1):
# existing codeSet up: prefect gcl create plate-resort-hardware 1
This ensures only one flow can control the hardware at a time. State between flows is not shared by design - each workflow is independent. If you need persistent state across workflows, you'd use a Prefect Block or external state store, but for this hardware control use case, the ephemeral per-workflow state is appropriate.
There was a problem hiding this comment.
@copilot noting I guess that using serve instead of deploy would allow us to reuse the instance (maybe?) except that we can't use serve because of other issues I haven't explained here. Since we're using deploy, it's more or less confined to these (re)connect loops as I'm understanding it.
…path); bump to 2.0.7
sgbaird
left a comment
sgbaird
left a comment
There was a problem hiding this comment.
Did you end up needing to remove flow decorators?
|
potentially, still not functioning yet. but it kept crashing during runs. something wasnt right, need to take a break and bang my head on something else for a bit |
…ts and scripts (v2.0.9)
… pool configurable via PLATE_RESORT_POOL; update docs/scripts to reference env override
…ection flows; chore: cleanup obsolete scripts; version 2.0.22
…docs: changelog 2.0.23; bump version
…refect schema; bump to v2.0.61
- Create clean separation between Prefect and REST API interfaces - Move workflows to plate_resort/interfaces/prefect/ - Move REST API server to plate_resort/interfaces/rest_api/ - Add motor auto-detection (XM430/XL430) with current control support - Update console scripts with clear naming (prefect vs rest prefixes) - Consolidate key generation to REST-specific keygen_rest.py - Update all import paths and documentation - Add comprehensive interface documentation
- Add REST client usage examples to DEPLOYMENT.md showing correct commands - Clarify available commands: connect, disconnect, status, position, activate - Remove any references to unavailable commands like 'home' - Ensure deployment docs match actual CLI implementation
- Help troubleshoot why server shows 'changeme' despite environment variable being set - Temporary debug output to see what environment key is being read
- Add activate_hotel.py test script for direct hotel activation - Add status.py test script for system status checking - Add move_to.py test script for manual positioning - Add test_two_stage.py to verify precise movement works directly - Update tests/README.md with usage examples - Set default_speed to 10 for safe, controlled movement
- Remove DEBUG print from load_api_key function - Ensure clean production-ready code - REST API now fully uses activate_hotel_precise() for two-stage movement
- Clarified client_user_guide.md is for REST API interface - Added complete command reference with table format - Updated troubleshooting with specific HTTP error codes - Used actual sdl3 Pi hostname in examples - Added missing commands: connect, position, move, disconnect - Removed GUI development section for simpler focus
- Convert command list to easy-to-read table format - Update troubleshooting with specific error codes - Add REST API title clarification
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.