Migrate BeamlineParameters to use config server by jacob720 · Pull Request #1773 · DiamondLightSource/dodal

jacob720 · 2025-12-09T10:44:45Z

Fixes DiamondLightSource/mx-bluesky#1504

Required by DiamondLightSource/mx-bluesky#1511

Instructions to reviewer on how to test:

Test beamline parameters files are not read from the file system in dodal

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
Have the connection tests for the relevant beamline(s) been run via dodal connect ${BEAMLINE}

codecov · 2026-01-12T16:42:11Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.06%. Comparing base (a1bcbe9) to head (286a17c).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1773      +/-   ##
==========================================
+ Coverage   99.04%   99.06%   +0.01%     
==========================================
  Files         308      313       +5     
  Lines       11672    11720      +48     
==========================================
+ Hits        11561    11610      +49     
+ Misses        111      110       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

DominicOram

Thanks, this looks good, some comments in code. This also means that we are now reliant on the config server so I think we need to do DiamondLightSource/mx-bluesky#478 before we put this on the beamline. Would you be able to pick that up and put in a meeting with Neil, Mark, Rob and Myself?

DominicOram · 2026-01-20T08:31:40Z

src/dodal/common/beamlines/beamline_parameters.py

-from dodal.log import LOGGER
 from dodal.utils import get_beamline_name

 BEAMLINE_PARAMETER_KEYWORDS = ["FB", "FULL", "deadtime"]


Should: This list is no longer used, can we remove it?

DominicOram · 2026-01-20T08:34:16Z

src/dodal/common/beamlines/beamline_parameters.py

+        )


 def get_beamline_parameters(beamline_param_path: str | None = None):


Could: This is a bit of scope creep but the way we're doing this is a bit messy. I think we can do it like we do

dodal/src/dodal/beamlines/i03.py

Line 92 in d245f0a

def daq_configuration_path() -> str:

and load it in as part of the beamline file. If that makes sense? Can you either do here or make a new issue?

This makes sense. So when devices need beamline parameters and before would have called get_beamline_parameters(), I'm guessing now they'd be passed in when the device is constructed.

How would this work when we need to get beamline parameters inside common plans? Or should these params always be attached to devices by that point?

So when devices need beamline parameters and before would have called get_beamline_parameters(), I'm guessing now they'd be passed in when the device is constructed.

Yes

How would this work when we need to get beamline parameters inside common plans? Or should these params always be attached to devices by that point?

Yh, that is a bit harder. I think we would need to have a look at it, which maybe means it should be in another issue

Maybe a similar thing can be done for the config server itself? So have a get_config_server() in every ixx.py that returns a config client set up for that beamline

DominicOram · 2026-01-20T08:35:15Z

src/dodal/common/beamlines/beamline_parameters.py

-        return cls(params=dict(config_pairs))
-
    @classmethod
    def from_file(cls, path: str):


Nit: It's a little misleading that this is still called from_file when it's not reading directly from the file

DominicOram · 2026-01-20T08:40:04Z

src/dodal/common/beamlines/beamline_parameters.py

-    @classmethod
-    def parse_value(cls, value: str):
-        return ast.literal_eval(value.replace("Yes", "True").replace("No", "False"))
+        config_server = ConfigServer(url="https://daq-config.diamond.ac.uk")


Should: Can you pull the URL into a constant? From DiamondLightSource/mx-bluesky#478 (comment) it may be that we will need it to be beamline specific too

DominicOram · 2026-01-20T08:59:21Z

tests/conftest.py

+        print(contents)
+        if desired_return_type is str:
+            return contents
+        elif desired_return_type is dict:
+            print("return type is dict")


Should: Remove print statements

DominicOram · 2026-01-20T09:04:37Z

tests/conftest.py

+        elif desired_return_type is dict:
+            print("return type is dict")
+            return json.loads(contents)
+        elif issubclass(desired_return_type, ConfigModel):  # type: ignore


Should: As far as I can see we're never trying to return a ConfigModel so why do we need this bit? Or is it for later PRs?

Yeah this is for later PRs. We either get a dict or a ConfigModel depending on what we request from the server, other config like LUTs or display config will be returned as a subclass of ConfigModel, this fixture emulates that

…to_config_server

src/dodal/common/beamlines/beamline_parameters.py

rtuck99 · 2026-02-09T18:02:52Z

src/dodal/plan_stubs/check_topup.py

 def _load_topup_configuration_from_properties_file() -> dict[str, Any]:
-    params = get_beamline_parameters()
-    return params.params
+    params = get_beamline_parameters(get_beamline_name("i03"))


I don't really like this practice that we have in the code of inserting arbitrary default beamline names into random bits of code. By removing the default from get_beamline_parameters() this has just spread it around more.

I think it would be better if we never assumed it anywhere in production code and instead took it from the BEAMLINE environment variable in a single function which we can patch out for unit tests.

So basically remove the 'default' argument from get_beamline_name()?

I've had a quick go at doing this and it's looking like a big change, and I'm not 100% sure of the consequences, so I think we should pull it out into a new issue. For now, I'll make the default argument optional so that we don't have to arbitrarily decide on defaults.

rtuck99 · 2026-02-09T18:07:49Z

src/dodal/devices/beamlines/i03/undulator_dcm.py

        # Nb this parameter is misleadingly named to confuse you
        self.dcm_fixed_offset_mm = get_beamline_parameters(
-            daq_configuration_path + "/domain/beamlineParameters"
+            get_beamline_name("i03"),


Why is undulator special, it's the only one that uses the extra parameter on get_beamline_parameters(), isn't it just getting the same beamline parameters file that everything else uses? The only other places it's used are tests.

I think we should drop the filename argument unless you can think of a good reason to keep it.

…to_config_server

rtuck99 · 2026-02-12T11:19:15Z

tests/common/beamlines/test_beamline_parameters.py



-def test_get_beamline_parameters_works_with_no_environment_variable_set():
+def test_get_beamline_parameters_errors_with_no_environment_variable_set():


Probably best to do something like patch.dict(environ, {k: v for k, v in environ.items() if k != "BEAMLINE"}, clear=True) if only to avoid any possibility of strange downstream effects from mutating our environment (although we should hopefully be agnostic to this environment variable)

or perhaps even simpler use monkeypatch as you have used below

rtuck99 · 2026-02-12T11:35:43Z

tests/common/beamlines/test_beamline_parameters.py

-
-
-def test_get_beamline_parameters_works_with_no_environment_variable_set():
+def test_get_beamline_parameters_errors_with_no_environment_variable_set():


This test doesn't actually do what it says on the tin - because we specify the beamline parameter it never errors.

rtuck99

Approved, subject to fixing test to cover the error condition, and minor observation about environment

…to_config_server

jacob720 requested a review from a team as a code owner December 9, 2025 10:44

jacob720 marked this pull request as draft December 9, 2025 10:44

jacob720 mentioned this pull request Dec 10, 2025

Read beamline parameters through config server DiamondLightSource/mx-bluesky#1511

Open

1 task

jacob720 marked this pull request as ready for review January 12, 2026 16:45

DominicOram requested changes Jan 20, 2026

View reviewed changes

DominicOram mentioned this pull request Jan 20, 2026

Use config server for undulator config #1768

Open

4 tasks

jacob720 mentioned this pull request Jan 23, 2026

Get beamline params from beamline files #1855

Open

jacob720 added 11 commits January 23, 2026 17:27

Add mock config server fixture

b4dbd8e

Use config server to read beamlineParmaeters

bf08ced

Remove tests for removed functions

91fc322

Convert test beamlineParameters files

386b521

Fix test

acd5805

Fix lint

28b7823

Fix

84f0836

Require latest daq-config-server

afd4fbc

PR comments WIP

2d92790

Parameterise config server URL

e3f6fad

Refactor get_beamline_parameters

3dbb881

jacob720 force-pushed the mx_bluesky_1504_migrate_beamline_parameters_to_config_server branch from c87a30e to 3dbb881 Compare January 23, 2026 17:33

Update lockfile

9517b6a

jacob720 requested a review from DominicOram January 26, 2026 09:59

jacob720 added 4 commits January 29, 2026 16:51

Use server deployed on i03 beamline cluster for i03 config

bc858b2

typo

9529dc1

Merge branch 'main' into mx_bluesky_1504_migrate_beamline_parameters_…

4aac691

…to_config_server

Merge branch 'main' into mx_bluesky_1504_migrate_beamline_parameters_…

cba50ab

…to_config_server

rtuck99 self-assigned this Feb 9, 2026

rtuck99 reviewed Feb 9, 2026

View reviewed changes

src/dodal/common/beamlines/beamline_parameters.py Show resolved Hide resolved

rtuck99 reviewed Feb 9, 2026

View reviewed changes

src/dodal/common/beamlines/beamline_parameters.py Show resolved Hide resolved

rtuck99 reviewed Feb 9, 2026

View reviewed changes

jacob720 added 5 commits February 11, 2026 16:22

WIP

eaa20a2

Reset cache between tests

0d29dbe

Merge branch 'main' into mx_bluesky_1504_migrate_beamline_parameters_…

feae3b6

…to_config_server

Fix

d4fda7b

Fix lint

a947f60

jacob720 requested a review from rtuck99 February 11, 2026 16:59

Fix lint

d5bd077

rtuck99 reviewed Feb 12, 2026

View reviewed changes

rtuck99 approved these changes Feb 12, 2026

View reviewed changes

jacob720 added 3 commits February 13, 2026 10:49

Merge branch 'main' into mx_bluesky_1504_migrate_beamline_parameters_…

4b4a3f1

…to_config_server

PR comments and coverage

357a6c9

Lint

286a17c

		)


		def get_beamline_parameters(beamline_param_path: str \| None = None):



		def test_get_beamline_parameters_works_with_no_environment_variable_set():
		def test_get_beamline_parameters_errors_with_no_environment_variable_set():

Conversation

jacob720 commented Dec 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Instructions to reviewer on how to test:

Checks for reviewer

Uh oh!

codecov bot commented Jan 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

DominicOram left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jacob720 Jan 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jacob720 Feb 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rtuck99 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Comments

jacob720 commented Dec 9, 2025 •

edited

Loading

codecov bot commented Jan 12, 2026 •

edited

Loading

jacob720 Jan 20, 2026 •

edited

Loading

jacob720 Feb 10, 2026 •

edited

Loading