Skip to content

Use daq config server for UndulatorDCM config#1497

Merged
jacob720 merged 42 commits intomainfrom
use_daq_config_server
Mar 24, 2026
Merged

Use daq config server for UndulatorDCM config#1497
jacob720 merged 42 commits intomainfrom
use_daq_config_server

Conversation

@jacob720
Copy link
Contributor

@jacob720 jacob720 commented Dec 5, 2025

Fixes #1494

Link to dodal PR (if required): DiamondLightSource/dodal#1768
Also requires https://gitlab.diamond.ac.uk/MX-GDA/hyperion-system-testing/-/merge_requests/7
Reads UndulatorDCM config through the config server.

Also adds system tests and changes existing system tests to use a locally deployed real config server rather than mocking the client as is done in unit tests.

Instructions to reviewer on how to test:

  1. Check tests pass
  2. Check system tests pass

Checks for reviewer

  • Would the PR title make sense to a user on a set of release notes

@jacob720 jacob720 requested a review from a team as a code owner December 5, 2025 10:29
@jacob720 jacob720 marked this pull request as draft December 5, 2025 10:58
@jacob720 jacob720 force-pushed the use_daq_config_server branch from c4613e8 to 7c21294 Compare December 5, 2025 11:00
@jacob720 jacob720 added dev experience Changes relating to developer experience i03 Changes relating to I03 labels Dec 5, 2025
@jacob720 jacob720 force-pushed the use_daq_config_server branch from 7c21294 to 0ec62f4 Compare December 9, 2025 09:07
@jacob720 jacob720 removed the i03 Changes relating to I03 label Dec 10, 2025
@jacob720 jacob720 marked this pull request as ready for review January 12, 2026 13:27
@codecov
Copy link

codecov bot commented Jan 12, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.87%. Comparing base (7b58d00) to head (cbec4e9).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1497      +/-   ##
==========================================
- Coverage   92.88%   92.87%   -0.01%     
==========================================
  Files         155      155              
  Lines        8498     8492       -6     
==========================================
- Hits         7893     7887       -6     
  Misses        605      605              
Components Coverage Δ
i24 SSX 77.32% <ø> (ø)
hyperion 98.59% <100.00%> (-0.01%) ⬇️
other 98.30% <100.00%> (-0.01%) ⬇️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@rtuck99 rtuck99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me, however would be nice to see some minimal system tests with the config server so that we can verify we can read configs. I think also that having such tests also does help with thinking about endpoint configuration.

@jacob720 jacob720 force-pushed the use_daq_config_server branch from a66a068 to c887f44 Compare March 2, 2026 11:25
@jacob720 jacob720 changed the base branch from main to 1504_read_beamlineParameters_through_config_server March 2, 2026 11:26
Base automatically changed from 1504_read_beamlineParameters_through_config_server to main March 13, 2026 16:21
@jacob720 jacob720 force-pushed the use_daq_config_server branch from b4a55b8 to 33f5b44 Compare March 17, 2026 16:05
@jacob720 jacob720 requested a review from rtuck99 March 20, 2026 16:37
@@ -8,8 +8,7 @@


Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could consider renaming the file here to config_client

d_spacing_a: float = yield from bps.rd(
undulator_dcm.dcm_ref().crystal_metadata_d_spacing_a
)
config_client = get_config_client("i03")
Copy link
Contributor

@rtuck99 rtuck99 Mar 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think echoing what other people have said elsewhere, we should be able to locate the config client without having to specify the beamline. At a device level we could inject the config client via a fixture with the correct endpoint and not need to know the specific beamline or have get_config_client(), at least in devices experiment plans, we may need something to retrieve the correct client in a plan occasionally, but the plan shouldn't need to know which beamline it's on, only the type of resource it needs.

@jacob720 jacob720 requested a review from rtuck99 March 23, 2026 17:07
Copy link
Contributor

@rtuck99 rtuck99 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved, just need changes in the dodal PR as discussed

@jacob720 jacob720 enabled auto-merge (squash) March 24, 2026 11:11
@jacob720 jacob720 merged commit 829a5ce into main Mar 24, 2026
9 of 10 checks passed
@jacob720 jacob720 deleted the use_daq_config_server branch March 24, 2026 11:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dev experience Changes relating to developer experience

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Migrate Undulator and UndulatorDCM config to use the daq config server

2 participants