-
Notifications
You must be signed in to change notification settings - Fork 2
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
Set up blueapi #95
Set up blueapi #95
Conversation
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.
Thank you! Couple of comments
@@ -65,7 +64,8 @@ def _coerce_to_path(path: Path | str) -> Path: | |||
|
|||
|
|||
@log.log_on_entry | |||
def initialise_extruderi24(args=None): | |||
def initialise_extruder() -> MsgGenerator: | |||
setup_logging() |
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.
Comment: It's messy we have to set this up at the start of every plan, I have put DiamondLightSource/blueapi#494 on the i24 backlog to fix
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.
I just linked the issue for now
def run_extruderi24(args=None): | ||
# Get dodal devices | ||
zebra = i24.zebra() | ||
def run_extruder_plan(zebra: Zebra) -> MsgGenerator: |
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.
Should: I think we can just default this so we don't need to always provide it https://diamondlightsource.github.io/blueapi/main/how-to/write-plans.html#injecting-defaults. There's only one zebra we would ever do this with after all.
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.
I think this is probably true for most of the device arguments throughout actually
# TODO have this in each function | ||
# Really tempted to make a wrapper ... |
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.
Yes, you do need to add it for now but I think then prioritise the blueapi issue
@@ -3834,7 +3834,7 @@ font "arial-bold-r-14.0" | |||
buttonLabel "Move on Click" | |||
numCmds 1 | |||
command { | |||
0 "python SCRIPTS_LOCATION/fixed_target/i24ssx_moveonclick.py" | |||
0 "blueapi -c CONFIG_LOCATION controller run start_viewer '\{\"oav\":\"oav\",\"pmac\":\"pmac\"\}'" |
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.
Must: I don't think you can run this through BlueAPI
because it has a GUI. I think you need to just keep it outside BlueAPI
for now
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.
Mmm, this is actually one of the things I tested on the beamline and the GUI seemed to work fine...
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.
Edit: the GUI works fine, the issue comes when clicking on one of the move to
buttons, which run other plans... Here blueAPI
will fail with this error:
Traceback (most recent call last):
File "/dls_sw/i24/software/bluesky/mx_bluesky_testing/mx_bluesky/.venv/bin/blueapi", line 8, in <module>
sys.exit(main())
^^^^^^
File "/dls_sw/i24/software/bluesky/mx_bluesky_testing/mx_bluesky/.venv/lib/python3.11/site-packages/click/core.py", line 1130, in __call__
return self.main(*args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/dls_sw/i24/software/bluesky/mx_bluesky_testing/mx_bluesky/.venv/lib/python3.11/site-packages/click/core.py", line 1055, in main
rv = self.invoke(ctx)
^^^^^^^^^^^^^^^^
File "/dls_sw/i24/software/bluesky/mx_bluesky_testing/mx_bluesky/.venv/lib/python3.11/site-packages/click/core.py", line 1657, in invoke
return _process_result(sub_ctx.command.invoke(sub_ctx))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/dls_sw/i24/software/bluesky/mx_bluesky_testing/mx_bluesky/.venv/lib/python3.11/site-packages/click/core.py", line 1657, in invoke
return _process_result(sub_ctx.command.invoke(sub_ctx))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/dls_sw/i24/software/bluesky/mx_bluesky_testing/mx_bluesky/.venv/lib/python3.11/site-packages/click/core.py", line 1404, in invoke
return ctx.invoke(self.callback, **ctx.params)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/dls_sw/i24/software/bluesky/mx_bluesky_testing/mx_bluesky/.venv/lib/python3.11/site-packages/click/core.py", line 760, in invoke
return __callback(*args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/dls_sw/i24/software/bluesky/mx_bluesky_testing/mx_bluesky/.venv/lib/python3.11/site-packages/blueapi/cli/cli.py", line 107, in wrapper
func(*args, **kwargs)
File "/dls_sw/i24/software/bluesky/mx_bluesky_testing/mx_bluesky/.venv/lib/python3.11/site-packages/click/decorators.py", line 38, in new_func
return f(get_current_context().obj, *args, **kwargs)
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/dls_sw/i24/software/bluesky/mx_bluesky_testing/mx_bluesky/.venv/lib/python3.11/site-packages/blueapi/cli/cli.py", line 212, in run_plan
updated = client.update_worker_task(WorkerTask(task_id=task_id))
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/dls_sw/i24/software/bluesky/mx_bluesky_testing/mx_bluesky/.venv/lib/python3.11/site-packages/blueapi/cli/rest.py", line 90, in update_worker_task
return self._request_and_deserialize(
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
File "/dls_sw/i24/software/bluesky/mx_bluesky_testing/mx_bluesky/.venv/lib/python3.11/site-packages/blueapi/cli/rest.py", line 124, in _request_and_deserialize
raise BlueskyRemoteError(error_message)
blueapi.cli.event_bus_client.BlueskyRemoteError: Response failed with text: {"detail":"Worker already active"},
with error code: 409
which corresponds to Conflict
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.
The GUI won't work when it's being run by the BlueAPI
server that's running on the control machine...
# if __name__ == "__main__": | ||
# start_viewer() |
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.
Must: As above, I think this needs to stay and not be in BlueAPI
# Start the blueapi worker using the config file in this module | ||
echo "Starting the blueapi runner" | ||
blueapi -c "${current}/blueapi_config.yaml" serve & | ||
|
||
# Wait for blueapi to start | ||
for i in {1..30} | ||
do | ||
echo "$(date)" | ||
curl --head -X GET http://localhost:25565/status >/dev/null | ||
ret_value=$? | ||
if [ $ret_value -ne 0 ]; then | ||
sleep 1 | ||
else | ||
break | ||
fi | ||
done | ||
|
||
if [ $ret_value -ne 0 ]; then | ||
echo "$(date) BLUEAPI Failed to start!!!!" | ||
exit 1 | ||
else | ||
echo "$(date) BLUEAPI started" | ||
fi |
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.
Should: Can we move all this into it's own script for starting the server? Then have this and run_fixed_target
both call that script (which will eventually move to control machine)
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.
Thank you! Couple of small comments but generally good.
@@ -243,13 +256,16 @@ def save_screen_map(litemap_path: Path | str = LITEMAP_PATH) -> MsgGenerator: | |||
@log.log_on_entry | |||
def upload_parameters( | |||
chipid: str = "oxford", | |||
litemap_path: Path | str = LITEMAP_PATH, | |||
map_path: Optional[str] = None, |
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.
Should: I think this string stuff is messy. Could you either add an issue here or on blueAPI
to think of a nicer way to do it? It may be that using https://docs.pydantic.dev/latest/api/types/#pydantic.types.FilePath will help
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.
Opened issue here #107 . I was going to open one in blueAPI
but since the error came from pydantic, it might be worth to first try this.
@@ -921,16 +939,15 @@ def check_dir(val): | |||
else: | |||
pmac.home_stages() | |||
logger.debug("CSmaker done.") | |||
yield from bps.null() | |||
yield from bps.sleep(5) |
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.
Should: Why is this sleep here?
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.
Ah, that was from testing on the beamline, issue turned out to be somewhere else, should not have been committed. Removed now.
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.
Great, thank you!
Closes #93
Note. This should be a first step #45 . The blueapi worker need to be run on the control machine as gda2 for th permission issues to go away.