Create a CLI for connecting to a beamline #519

callumforrester · 2024-05-10T08:21:09Z

Fixes #506

Instructions to reviewer on how to test:

Run unit tests
Run CLI system tests against all beamlines

Checks for reviewer

Would the PR title make sense to a scientist on a set of release notes
If a new device has been added does it follow the standards
If changing the API for a pre-existing device, ensure that any beamlines using this device have updated their Bluesky plans accordingly

callumforrester · 2024-05-13T11:46:49Z

src/dodal/beamlines/__init__.py

+ALL_BEAMLINES: set[str] = {
+    "i03",
+    "i04",
+    "i04_1",
+    "i20_1",
+    "i22",
+    "i23",
+    "i24",
+    "p38",
+    "p45",
+}


This list of all beamline modules was previously maintained in the tests directory. It has been moved here so the CLI can reuse it.

I'm not sure how much I like maintaining a list of all beamline modules as it seems prone to error. The advantage is that typing dodal connect --help will include a list of beamlines you can connect to.

The alternatives are some fancy auto-detection of beamline modules, perhaps by finding all modules in dodal.beamlines that match a pattern or simply parsing any string the user enters and printing an error message if it's not a beamline.

I dislike the first because baking in beamline naming conventions has tended to lead to trouble in the past.

We could move _device_helpers.py etc. into some other location and just have a convention that only beamline modules live in beamlines. Then we can just auto-detect based on all the module names in that folder?

I felt that was worth its own PR: #544

callumforrester · 2024-05-13T11:50:48Z

src/dodal/cli.py

+
+
+@main.command(name="connect")
+@click.argument("beamline", type=click.Choice(list(ALL_BEAMLINES)), required=True)


Passing the beamline module in as an argument may be confusing in cases where it conflicts with the ${BEAMLINE} environment variable. However, in some cases the two are intentionally different. For example if running against s03 you load the i03 beamline module but with BEAMLINE=s03, so ${BEAMLINE} and the beamline module are two separate things with the same name and the same value in lots of cases, which seems a bit confusing. Not sure if it's worth tackling here or raising another issue.

Could we default to using ${BEAMLINE} (which will be the case for 90%) of our users then bomb out if it's not set?

Except for i04-1, say, ${BEAMLINE} is i04-1 and the beamline module is i04_1, so I think they are the same in much less than 90% given that we already have two branchlines in dodal

Yh, you're right, let's just require it

I think it's still confusing given #519 (comment)

Some other options are:

Completely remove the ${BEAMLINE} variable from dodal, base everything on the beamline module

Have s03 as some kind of profile for i03 rather than base on the environment variable so you could do something like dodal connect --profile tickit i03, that would also be a good solution to Automatically keep I22 and P38 in sync #502

Agreed, we then just move to solution 2 eventually as part of #502

@DominicOram I think I've probably misunderstood this because what I've done is the same thing again. I made it so you can type:

dodal connect i04-1

which will try to connect to s04-1 or

export BEAMLINE=i04-1 dodal connect

which try to connect to i04. That feels about as confusing and probably not what you had in mind.

Have just thought of another requirement: the training rigs. They are all identical so should have one dodal module (I believe @paula-mg has prototyped one) and rely on ${BEAMLINE} or similar to differentiate.

Sorry, I wasn't clear my suggestion is literally to do os.environ["BEAMLINE"] = beamline inside connect

Sounds good, also works for the training rig case

DominicOram

Thanks. Code looks good but it doesn't behave exactly how I would expect. If I do dodal connect i03 it attempts to connect to s03 rather than i03. I'm also not convinced that it tries to connect to subsequent devices after the first failure. Additionally:

Could you add a line to the README that let's people know this exists?
I guess we can now remove all the test_XXX_system.pys?

callumforrester · 2024-05-13T13:25:18Z

@DominicOram makes sense, the dodal connect i03 behaviour mirrors the i03 system tests, which try to connect to s03 by default unless ${BEAMLINE} is i03. I agree its confusing, which is why I was hoping for a better solution for #519 (comment)

callumforrester · 2024-05-13T13:36:21Z

@DominicOram re: connecting to subsequent devices after the first failure: This is an issue in general with make_all_devices, hopefully one that @ZohebShaikh will solve as part of DiamondLightSource/blueapi#444

callumforrester · 2024-05-20T10:14:00Z

CI failure should be fixed by #544

DominicOram

Great, thank you. Some comments in code. I think we need to fix the simulated beamlines being used so doing the os.environ["BEAMLINE"] = beamline is a Must unless you have a cleaner solution.

As I said before, can you remove the test_iXX_system.py files please?

DominicOram · 2024-05-21T16:22:15Z

README.rst

+    # On any workstation:
+    dodal connect <BEAMLINE>
+
+    # On a beamline workstation, this should suffice:
+    dodal connect ${BEAMLINE}


Should: Sorry to scope creep a bit can we add this to the PR template while it's not in CI. As a reviewer it's basically the first thing I run and a lot of the time it fails.

DominicOram · 2024-05-21T16:37:47Z

src/dodal/cli.py

+
+    module_name = module_name_for_beamline(beamline)
+
+    from bluesky import RunEngine


Nit: Does pylance not give you an error here about RunEngine not being exported from bluesky? I have to use from bluesky.run_engine import RunEngine to fix

Interesting, yes it does, even though RunEngine is exported from the top level: https://github.com/bluesky/bluesky/blob/27ca36cc60b718ad3f2f2bb343b1d086aec77708/src/bluesky/__init__.py#L4

I suspect pylance is only looking for __all__, will change it to reduce the nasty red lines.

Also unsure why I'm doing the import inside the function, will change that too

DominicOram · 2024-05-21T16:40:23Z

src/dodal/cli.py

+
+
+@main.command(name="connect")
+@click.argument("beamline", type=click.Choice(list(ALL_BEAMLINES)), required=True)


Sorry, I wasn't clear my suggestion is literally to do os.environ["BEAMLINE"] = beamline inside connect

callumforrester · 2024-05-22T08:02:36Z

As I said before, can you remove the test_iXX_system.py files please?

Whoops, sorry!

callumforrester · 2024-05-22T12:10:43Z

tests/unit_tests/test_cli.py

+# We need to mock the environment because the CLI edits it and this could break other
+# tests


Setting os.environ["BEAMLINE"] caused unintended side effects, will make an issue to find a better way

Done in #569

DominicOram

Great, thank you! Minor nit-picks in code but otherwise good. I Tested a few of the beamlines. The _1 beamlines are broken for a different reason I think (#570) but others passed.

src/dodal/beamlines/__init__.py

DominicOram · 2024-05-22T13:44:17Z

src/dodal/cli.py

+
+    module_name = module_name_for_beamline(beamline)
+
+    RE = RunEngine()  # noqa: F841


Could: A comment here about why we need to make a RE would be good. Also do we actually need to assign it? Just RunEngine() works fine right, then we can remove the noqa?

DominicOram · 2024-05-22T17:02:52Z

src/dodal/cli.py

+
+    RE = RunEngine()  # noqa: F841
+
+    devices = make_all_devices(


Nit: Would be nice if we could print something just before this saying that it's started and will take some time. The i03 ones take ~30s so not obvious it hasn't just hung.

Co-authored-by: Dominic Oram <dominic.oram@diamond.ac.uk>

callumforrester force-pushed the cli-system-tests branch 2 times, most recently from f3c2bfe to dc2d361 Compare May 13, 2024 10:35

callumforrester commented May 13, 2024

View reviewed changes

callumforrester marked this pull request as ready for review May 13, 2024 11:54

callumforrester requested review from DominicOram and dperl-dls May 13, 2024 11:54

DominicOram requested changes May 13, 2024

View reviewed changes

callumforrester force-pushed the cli-system-tests branch from d314fc5 to 4395567 Compare May 15, 2024 17:14

callumforrester mentioned this pull request May 15, 2024

Move non-beamline modules out of beamlines package #543

Closed

callumforrester force-pushed the cli-system-tests branch 4 times, most recently from 1fec9e4 to 4492d1b Compare May 20, 2024 10:09

callumforrester mentioned this pull request May 21, 2024

Add System tests for I22 #498

Closed

2 tasks

callumforrester force-pushed the cli-system-tests branch from 4492d1b to c99dc77 Compare May 21, 2024 15:26

DominicOram requested changes May 21, 2024

View reviewed changes

callumforrester force-pushed the cli-system-tests branch 2 times, most recently from 24d7d98 to 4423626 Compare May 22, 2024 11:50

callumforrester commented May 22, 2024

View reviewed changes

callumforrester mentioned this pull request May 22, 2024

Make CLI beamline mappings work without mutating environment #569

Open

callumforrester force-pushed the cli-system-tests branch from 79519a8 to d080cb3 Compare May 22, 2024 12:14

callumforrester added 4 commits May 22, 2024 14:32

Create a CLI for connecting to a beamline

6520858

Move ALL_BEAMLINES environment variable out into common module

4cd433f

Remove post-init connection from CLI

a766f40

Add CLI tests

f062c31

callumforrester added 13 commits May 22, 2024 14:32

Fix formatting

48d9141

Auto generate list of beamlines

c467fa9

Fix formatting

5334e5a

Restore beamline as a requirement

103079b

Allow for multiple beamlines per dodal module

7b7550d

Note the CLI in the README

aa75e17

Delete old system tests

993bf18

Add connection tests to PR template checklist

520d0c3

Fix RunEngine import in CLI

e76daed

Sync beamline module and BEAMLINE environment variable in CLI

6a2b2e1

Refactor tests and add tests for the beamline name mapping

5af1279

Fix imports

c7f364e

Prevent tests breaking environment variables

151c6e8

callumforrester force-pushed the cli-system-tests branch from d080cb3 to 151c6e8 Compare May 22, 2024 13:32

callumforrester requested a review from DominicOram May 22, 2024 13:32

DominicOram mentioned this pull request May 22, 2024

Beamline prefix in *_01 beamlines are wrong #570

Closed

DominicOram approved these changes May 22, 2024

View reviewed changes

callumforrester and others added 4 commits May 23, 2024 07:29

Fix typo in comment

e87e6c1

Co-authored-by: Dominic Oram <dominic.oram@diamond.ac.uk>

Explain the need for a RunEngine in the CLI

fe44ffa

Print starting connection message

694e379

Fix CLI tests

82e1bb0

callumforrester merged commit e5efc4e into main May 23, 2024
11 checks passed

callumforrester deleted the cli-system-tests branch May 23, 2024 08:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create a CLI for connecting to a beamline #519

Create a CLI for connecting to a beamline #519

callumforrester commented May 10, 2024

callumforrester May 13, 2024

DominicOram May 13, 2024

callumforrester May 15, 2024

callumforrester May 13, 2024

DominicOram May 13, 2024

callumforrester May 13, 2024

DominicOram May 13, 2024

callumforrester May 13, 2024 •

edited

Loading

callumforrester May 15, 2024

callumforrester May 15, 2024

callumforrester May 20, 2024

DominicOram May 21, 2024

callumforrester May 22, 2024

DominicOram left a comment

callumforrester commented May 13, 2024

callumforrester commented May 13, 2024

callumforrester commented May 20, 2024

DominicOram left a comment

DominicOram May 21, 2024

DominicOram May 21, 2024

callumforrester May 22, 2024

callumforrester May 22, 2024

DominicOram May 21, 2024

callumforrester commented May 22, 2024

callumforrester May 22, 2024

callumforrester May 22, 2024

DominicOram left a comment

DominicOram May 22, 2024

DominicOram May 22, 2024



		@main.command(name="connect")
		@click.argument("beamline", type=click.Choice(list(ALL_BEAMLINES)), required=True)


		module_name = module_name_for_beamline(beamline)

		from bluesky import RunEngine

		# We need to mock the environment because the CLI edits it and this could break other
		# tests


		module_name = module_name_for_beamline(beamline)

		RE = RunEngine() # noqa: F841

Create a CLI for connecting to a beamline #519

Create a CLI for connecting to a beamline #519

Conversation

callumforrester commented May 10, 2024

Instructions to reviewer on how to test:

Checks for reviewer

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

callumforrester May 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DominicOram left a comment

Choose a reason for hiding this comment

callumforrester commented May 13, 2024

callumforrester commented May 13, 2024

callumforrester commented May 20, 2024

DominicOram left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

callumforrester commented May 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DominicOram left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

callumforrester May 13, 2024 •

edited

Loading