-
Notifications
You must be signed in to change notification settings - Fork 178
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(robot-server): Refactor hardware wrapper and other lifetime dependencies #8213
Closed
Closed
Changes from 1 commit
Commits
Show all changes
49 commits
Select commit
Hold shift + click to select a range
e77dd78
Create `slow_initializing` module.
SyntaxColoring 686b1fa
Make `API.register_callback()` non-async.
SyntaxColoring 3e89b33
Eliminate HardwareWrapper.
SyntaxColoring fca92e0
WIP: Comment out tests.
SyntaxColoring 752fabb
WIP.
SyntaxColoring 656a100
Work around `request` being inaccessible from WebSocket endpoints.
SyntaxColoring 0f2ea16
Rename hardware_wrapper.py -> hardware_initialization.py.
SyntaxColoring d7b29eb
Make _make_event_publisher() non-async, and leave .close() todo.
SyntaxColoring bf04ca4
Cleanup.
SyntaxColoring 4dfa729
Uncomment and start fixing up tests.
SyntaxColoring bf31cc0
Todo for hardware shutdown.
SyntaxColoring e6bc2e4
Make _init_event_watchers non-async.
SyntaxColoring ab183c3
Port test_slow_initializing.py to Decoy.
SyntaxColoring 3e0f901
Delete unnecessary comments about BaseException.
SyntaxColoring e3eb9bd
WIP: refactor app lifetime dependencies.
SyntaxColoring 9428465
Tests: override motion lock dependency with a mock.
SyntaxColoring 6a33398
Documentation and minor cleanup.
SyntaxColoring c389c36
Rename app_dependencies.py to lifetime_dependencies.py.
SyntaxColoring 7c62bf8
Improve module docstring.
SyntaxColoring bcb6397
More WIP of lifetime dependencies.
SyntaxColoring f2d54fb
Clarify comment.
SyntaxColoring e42916b
Move some stuff around.
SyntaxColoring 602eb75
Cleanup, and fix circular import.
SyntaxColoring 0afd56a
Formatting.
SyntaxColoring c0dfca8
Linting.
SyntaxColoring c818120
Delete unnecessary comment.
SyntaxColoring 697e2fd
Format and lint.
SyntaxColoring 5ab454b
Rewrite `HardwareWrapper` tests for `hardware_initialization` module.
SyntaxColoring 1aa55bd
Format and lint.
SyntaxColoring 6552b3c
Merge lifetime dependency refactors.
SyntaxColoring 774cfb4
Lint.
SyntaxColoring 3a95844
Override with a *real* motion lock, to preserve existing behavior.
SyntaxColoring b6d95af
Port ProtocolManager and SessionManager cleanup code.
SyntaxColoring 3b854b2
Delete obsolete comment.
SyntaxColoring dc5032d
Handle `InitializationOngoingError` in `get_rpc_server()` (sort of).
SyntaxColoring b7ac395
Clean up some of `lifetime_dependencies`' imports.
SyntaxColoring 8827b2f
Delete unnecessary todo comment.
SyntaxColoring 3573b80
Don't unnecessarily wrap `motion_lock` in a context manager.
SyntaxColoring 59d46f3
Clarify a comment.
SyntaxColoring 615e06a
Remove now-unnecessary `@call_once` decorators.
SyntaxColoring 8d06237
Remove print debug statements.
SyntaxColoring abd5d19
Remove unused import.
SyntaxColoring 72a47dd
Remove resolved todo.
SyntaxColoring 1c8d3b9
Rename `lifetime_dependency_set` args to `lifetime_dependencies`.
SyntaxColoring 84a6ff6
Add some docstrings to `lifetime_dependencies` internal functions.
SyntaxColoring a960dd1
Make small improvements to how we store dependencies on `app`.
SyntaxColoring b3593da
Fix startup accidentally waiting for `ThreadManager` to be ready.
SyntaxColoring 233ef3d
Reformat.
SyntaxColoring 5343b36
Make `API.register_callback()` async again.
SyntaxColoring File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
More WIP of lifetime dependencies.
- Loading branch information
commit bcb6397663e0f3d678438671ec0372201ccd9620
There are no files selected for viewing
This file contains 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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,23 +28,34 @@ | |
from fastapi import FastAPI | ||
from starlette.requests import Request | ||
|
||
from . import slow_initializing | ||
from .slow_initializing import ( | ||
InitializationFailedError, | ||
SlowInitializing, | ||
start_initializing as start_slow_initializing | ||
) | ||
|
||
from notify_server.clients import publisher as notify_server_publisher | ||
from notify_server.settings import Settings as NotifyServerSettings | ||
from opentrons.hardware_control import ThreadedAsyncLock, ThreadManager | ||
from robot_server.hardware_wrapper import HardwareWrapper | ||
from robot_server.hardware_initialization import initialize as initialize_hardware | ||
from robot_server.service.legacy.rpc import RPCServer | ||
|
||
|
||
_YieldT = typing.TypeVar("_YieldT") | ||
# Helpers to be used as the return type annotations of factory functions that are | ||
# provided to contextlib.contextmanager or contextlib.asynccontextmanager, respectively. | ||
_CMFactory = typing.Generator[_YieldT, None, None] | ||
_ACMFactory = typing.AsyncGenerator[_YieldT, None] | ||
|
||
|
||
# todo(mm, 2021-08-04): Port get_session_manager() and get_protocol_manager() | ||
# dependencies, and clean them up with their .remove_all() methods. | ||
@dataclass(frozen=True) | ||
class AppDependencySet: | ||
"""All app dependencies that are exposed to the request layer.""" | ||
|
||
hardware_wrapper: slow_initializing.SlowInitializing[HardwareWrapper] | ||
rpc_server: slow_initializing.SlowInitializing[RPCServer] | ||
thread_manager: SlowInitializing[ThreadManager] | ||
rpc_server: SlowInitializing[RPCServer] | ||
motion_lock: ThreadedAsyncLock | ||
|
||
|
||
|
@@ -78,80 +89,79 @@ def get_app_dependencies(request: Request) -> AppDependencySet: | |
return _get_app_dependencies(request.app) | ||
|
||
|
||
@contextlib.asynccontextmanager | ||
async def _event_publisher() -> typing.AsyncGenerator[ | ||
notify_server_publisher.Publisher, None | ||
]: | ||
"""A dependency creating a single notify-server event publisher instance.""" | ||
@contextlib.contextmanager | ||
def _event_publisher() -> _CMFactory[notify_server_publisher.Publisher]: | ||
notify_server_settings = NotifyServerSettings() | ||
event_publisher = notify_server_publisher.create( | ||
notify_server_settings.publisher_address.connection_string() | ||
) | ||
# fixme(mm, 2021-07-29): Should close, and currently does not. | ||
yield event_publisher | ||
try: | ||
yield event_publisher | ||
finally: | ||
event_publisher.close() | ||
|
||
|
||
@contextlib.asynccontextmanager | ||
async def _hardware_wrapper( | ||
async def _thread_manager( | ||
event_publisher: notify_server_publisher.Publisher, | ||
) -> typing.AsyncGenerator[slow_initializing.SlowInitializing[HardwareWrapper], None]: | ||
async def factory() -> HardwareWrapper: | ||
hardware_wrapper = HardwareWrapper(event_publisher=event_publisher) | ||
await hardware_wrapper.initialize() | ||
return hardware_wrapper | ||
) -> _ACMFactory[SlowInitializing[ThreadManager]]: | ||
async def factory() -> ThreadManager: | ||
return await initialize_hardware(event_publisher) | ||
|
||
handle = slow_initializing.start_initializing(factory) | ||
slow_initializing = start_slow_initializing(factory) | ||
try: | ||
yield handle | ||
yield slow_initializing | ||
finally: | ||
# todo(mm, 2021-08-04): Any cleanup of hardware wrapper needed here? | ||
# Probably yes. It has a ThreadManager, which has a thread that needs to be | ||
# joined. | ||
pass | ||
try: | ||
fully_initialized_result = await slow_initializing.get_when_ready() | ||
except InitializationFailedError: | ||
pass | ||
else: | ||
fully_initialized_result.clean_up() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ℹ️ This |
||
# To do: Justify why we don't attempt to cancel. | ||
# Codebase generally isn't prepared to handle it. Need something like task groups. | ||
|
||
|
||
@contextlib.asynccontextmanager | ||
async def _rpc_server( | ||
hardware_wrapper: slow_initializing.SlowInitializing[HardwareWrapper], | ||
thread_manager: SlowInitializing[ThreadManager], | ||
lock: ThreadedAsyncLock, | ||
) -> typing.AsyncGenerator[slow_initializing.SlowInitializing[RPCServer], None]: | ||
) -> _ACMFactory[SlowInitializing[RPCServer]]: | ||
async def factory() -> RPCServer: | ||
# todo(mm, 2021-08-04): Eliminate wrapper and this indirection. | ||
assert (await hardware_wrapper.get_when_ready()).get_hardware() is not None | ||
|
||
# todo(mm, 2021-08-04): Inherited from previous code. Why imported here? | ||
SyntaxColoring marked this conversation as resolved.
Show resolved
Hide resolved
|
||
from opentrons.api import MainRouter | ||
|
||
hardware = typing.cast( | ||
ThreadManager, (await hardware_wrapper.get_when_ready()).get_hardware() | ||
) | ||
root = MainRouter(hardware=hardware, lock=lock) | ||
root = MainRouter(hardware=(await thread_manager.get_when_ready()), lock=lock) | ||
return RPCServer(loop=None, root=root) | ||
|
||
handle = slow_initializing.start_initializing(factory) | ||
slow_initializing = start_slow_initializing(factory) | ||
try: | ||
yield handle | ||
yield slow_initializing | ||
finally: | ||
# todo(mm, 2021-08-04): Waits to clean up. Expected? | ||
await (await handle.get_when_ready()).on_shutdown() | ||
try: | ||
fully_initialized_result = await slow_initializing.get_when_ready() | ||
except InitializationFailedError: | ||
pass | ||
else: | ||
await fully_initialized_result.on_shutdown() | ||
|
||
|
||
@contextlib.asynccontextmanager | ||
async def _all_app_dependencies() -> typing.AsyncGenerator[AppDependencySet, None]: | ||
async def _all_app_dependencies() -> _ACMFactory[AppDependencySet]: | ||
async with contextlib.AsyncExitStack() as stack: | ||
motion_lock = ThreadedAsyncLock() | ||
event_publisher = await stack.enter_async_context(_event_publisher()) | ||
hardware_wrapper = await stack.enter_async_context( | ||
_hardware_wrapper(event_publisher) | ||
event_publisher = stack.enter_context(_event_publisher()) | ||
thread_manager = await stack.enter_async_context( | ||
_thread_manager(event_publisher) | ||
) | ||
# todo(mm, 2021-08-04): If wrong arguments are provided to the _rpc_server | ||
# factory, MyPy doesn't catch it. Why not? | ||
rpc_server = await stack.enter_async_context( | ||
_rpc_server(hardware_wrapper=hardware_wrapper, lock=motion_lock) | ||
_rpc_server(hardware_wrapper=thread_manager, lock=motion_lock) | ||
) | ||
|
||
complete_result = AppDependencySet( | ||
hardware_wrapper=hardware_wrapper, | ||
thread_manager=thread_manager, | ||
rpc_server=rpc_server, | ||
motion_lock=motion_lock, | ||
) | ||
|
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ℹ️ This
.close()
was omitted before this PR. The new pattern made its omission more obvious.