fix: upgrade alpyca to PyPI release#137
Open
dgegen wants to merge 5 commits into
Open
Conversation
…isation Upgrade alpyca from a git pin to the stable PyPI release (>=3.1.2). Move device instantiation from __init__ to run() so that the alpyca device object (which opens network connections) is created inside the subprocess rather than in the parent, avoiding pickling issues and shared-socket hazards under multiprocessing. Initialise subprocess-local state in run(): patch Device._put/_get to default to a 60 s timeout and seed client IDs with a PID-derived random value to prevent conflicts across processes. Simplify callable dispatch in get__: replace the no_kwargs sentinel with `if callable(data): data = data(**kwargs)`, which handles both zero-arg methods (empty kwargs → data()) and methods with arguments, and correctly skips the call for plain property reads.
dgegen
changed the title
There was a problem hiding this comment.
Pull request overview
This PR upgrades alpyca to the PyPI 3.x release and adjusts the Alpaca device subprocess wrapper for safer multiprocessing behavior.
Changes:
- Replaces the git-pinned
alpycadependency withalpyca>=3.1.2,<4. - Moves Alpaca device construction into the subprocess
run()path and adds timeout/client ID setup. - Simplifies
get__callable handling and updates related observatory calls/tests.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
pyproject.toml |
Updates the alpyca dependency specifier. |
uv.lock |
Refreshes locked dependency metadata for alpyca and transitive packages. |
src/astra/alpaca_device_process.py |
Moves device instantiation to subprocess runtime and changes callable dispatch behavior. |
src/astra/observatory.py |
Updates calls affected by the new callable execution behavior. |
tests/test_alpaca_device_process.py |
Adds coverage for auto-executing a callable device method. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+522
to
+526
| if data == "not get": | ||
| data = getattr(self.device, method) | ||
|
|
||
| # if kwargs, call method with kwargs | ||
| if kwargs: | ||
| if "no_kwargs" in kwargs: | ||
| data = data() | ||
| else: | ||
| data = data(**kwargs) | ||
| if callable(data): | ||
| data = data(**kwargs) |
Comment on lines
+393
to
+406
| else: | ||
| self.queue.put( | ||
| ( | ||
| self.metadata, | ||
| { | ||
| "type": "log", | ||
| "data": ( | ||
| "warning", | ||
| f"{self.device_type} is not a valid device type", | ||
| ), | ||
| }, | ||
| ) | ||
| ) | ||
| return |
Comment on lines
+525
to
+526
| if callable(data): | ||
| data = data(**kwargs) |
There are 4 occurrences of `self.telescope.get("PulseGuide")(...)`.
The new behaviour, get now auto-calls the callable in the subproces).
Consequently, this needed to be updated
If getattr() succeeded but the subsequent call raised, data was left holding the bound method rather than the "not get" sentinel, causing the retry loop and final attempt to be silently skipped, then failing to pickle the method on send. Use a temporary variable so data is only updated from the sentinel once the full operation completes.
The previous late check in run() left the parent holding open pipes to a dead subprocess, resulting in an EOFError on the next get() or set() call rather than a meaningful error. Raising ValueError in __init__ fails fast before start() is called and any pipes are created, giving callers an immediate and unambiguous error message.
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
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.
Pull Request
Description
Upgrades alpyca from a git-pinned pre-release (2.0.4) to the stable PyPI release (
>=3.1.2,<4), and fixes several issues that surfaced during the migration. The most significant fix moves device instantiation out of__init__and intorun(), which is where it should live in a multiprocessing context, as creating a networked device object in the parent process before forking risks shared sockets and pickle failures. Subprocess initialisation now also sets a 60 s default timeout and randomises client IDs per process to prevent conflicts when multiple device processes run concurrently.As a secondary cleanup, the
no_kwargssentinel pattern inget__is replaced with a straightforwardif callable(data): data = data(**kwargs), which is both simpler and safer (plain property reads are no longer at risk of a spurious call attempt).Changes Made
>=3.1.2,<4from PyPI instead of a git dependencyAlpacaDevicedevice instantiation from__init__torun()so the device object is created inside the subprocess_ensure_60s_default_timeouts(): monkey-patchesDevice._put/_getto default to a 60 s timeout (applied once per subprocess)_seed_client_ids(): randomisesDevice._client_idand_client_trans_idusing a PID-derived seed to avoid inter-process conflictsget__: replace theno_kwargssentinel withif callable(data): data = data(**kwargs); removes the only caller site (observatory.py) that passedno_kwargs=TrueAbortExposurecall inobservatory.py— was incorrectly double-called viacamera.get("AbortExposure")()tests/test_alpaca_device_process.pycovering the callable auto-execution pathChecklist