Run BlueskyContext and Worker in subprocess #343
Merged
Conversation
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
|
These changes do allow the run engine and worker to run in a subprocess but I'm not sure this is the best design as it ends up with many implementations of these methods. I've tested it though and it does work. Probably something to discuss next week. |
joeshannon
force-pushed
the
sp
branch
3 times, most recently
from
9797074
to
ae4fabc
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #343 +/- ##
==========================================
+ Coverage 87.17% 88.70% +1.53%
==========================================
Files 42 43 +1
Lines 1645 1797 +152
==========================================
+ Hits 1434 1594 +160
+ Misses 211 203 -8 ☔ View full report in Codecov by Sentry. |
joeshannon
commented
Jan 10, 2024
joeshannon
force-pushed
the
sp
branch
2 times, most recently
from
a4873b3
to
afdaadd
Compare
|
Would merging this close #317? |
|
I think the four definitions of functions flaw can be solved by a refactor in the future, this is a good first attempt. |
Yes, is you commenting of that sufficient to auto close the issue when this is merged? |
|
No idea :) |
joeshannon
force-pushed
the
sp
branch
3 times, most recently
from
ae32bbd
to
c4fd452
Compare
This approach is possibly not the best design as it results in four definitions of all the functions.
joeshannon
force-pushed
the
sp
branch
5 times, most recently
from
ab42bbf
to
75925f3
Compare
joeshannon
force-pushed
the
sp
branch
2 times, most recently
from
8a2fd05
to
f94dac4
Compare
Setting environment state to not valid will cause the subprocess context to be reloaded.
Dependency override should import from main.py (app) not handler module.
Added mostly to satisfy type checking as the _subprocess field is set to None before it is started and when it is stopped.
callumforrester
approved these changes
Feb 5, 2024
There was a problem hiding this comment.
Please could you make issues to do the following (unless you feel like doing them now but I think we should get this merged):
- Document this approach, probably requires an explanation page and perhaps an ADR
- Add CLI commands to inspect/restart the environment
- Better error handling if someone calls, say,
/deviceswhile the environment is restarting
Yes I agree - it's probably best to draw a line under this and make new issues. |
callumforrester
added a commit
that referenced
this pull request
Feb 7, 2024
Closes #369 The validated and processed parameters from a plan request were being cached inside the `Task` class because they were needed in two separate threads. Unfortunately, with the introduction of the subprocess (#343), pickling was causing issues with the cached object (see #369 for more details). This PR is the simplest solution: remove the cache and generate the parameters twice. It also adds regression tests for the bug case in #369
Closed
ZohebShaikh
pushed a commit
that referenced
this pull request
May 7, 2024
Create SubprocessHandler as additional BlueskyHandler implementation to forward calls to a subprocess. This is to allow a new REST endpoint with the result of reloading the plans and devices without having to restart the service. Additionally the API version is incremented due to new schema and a reference to delete_task replaced with clear_pending_task which was missed in a previous PR. Further improvements required in later PRs.
ZohebShaikh
pushed a commit
that referenced
this pull request
May 7, 2024
Closes #369 The validated and processed parameters from a plan request were being cached inside the `Task` class because they were needed in two separate threads. Unfortunately, with the introduction of the subprocess (#343), pickling was causing issues with the cached object (see #369 for more details). This PR is the simplest solution: remove the cache and generate the parameters twice. It also adds regression tests for the bug case in #369
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.
This approach is possibly not the best design as it results in four definitions of all the functions.