Skip to content

Conversation

@jacob720
Copy link
Contributor

@jacob720 jacob720 commented Nov 11, 2025

Fixes #810

Requires DiamondLightSource/dodal#1750

Instructions to reviewer on how to test:

  1. Check tests pass and thaw happens when redis connection fails.

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 November 11, 2025 11:12
@jacob720 jacob720 marked this pull request as draft November 11, 2025 11:13
@jacob720 jacob720 changed the title 810 continue thaw if redis is down Murko: continue thaw if redis is down Nov 11, 2025
@jacob720 jacob720 added the murko Related to murko integration on i04 label Nov 11, 2025
@jacob720 jacob720 force-pushed the 810_continue_thaw_if_redis_is_down branch from ed70b21 to eaac852 Compare November 17, 2025 16:01
@jacob720 jacob720 marked this pull request as ready for review November 17, 2025 16:01
@codecov
Copy link

codecov bot commented Nov 17, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 92.30%. Comparing base (c1346e4) to head (f75054e).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1437      +/-   ##
==========================================
+ Coverage   92.28%   92.30%   +0.01%     
==========================================
  Files         142      142              
  Lines        8092     8107      +15     
==========================================
+ Hits         7468     7483      +15     
  Misses        624      624              
Components Coverage Δ
i24 SSX 78.56% <ø> (ø)
hyperion 97.93% <ø> (ø)
other 98.09% <100.00%> (+<0.01%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jacob720 jacob720 force-pushed the 810_continue_thaw_if_redis_is_down branch 3 times, most recently from 5488b16 to dc130dd Compare November 17, 2025 16:54
Copy link
Contributor

@DominicOram DominicOram left a comment

Choose a reason for hiding this comment

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

Thanks. This works but I think it might be cleaner if it was handled inside the MurkoCallback and the MurkoResultsDevice. They can both just do nothing and warn, then the MurkoResultsDevice can keep the positions all at 0 and the plan would never have to know there's a redis issue

run_engine: RunEngine,
caplog: pytest.LogCaptureFixture,
):
for plan in (thaw_and_murko_centre, thaw_and_stream_to_redis):
Copy link
Contributor

Choose a reason for hiding this comment

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

Could: Would be better to put this in a parametrize

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ended up deleting this test

@DominicOram
Copy link
Contributor

I should add my reasoning here. Basically, what if we wanted to make use of the MurkoCallback again? We would have to put a similar check everywhere we wanted to use it, whereas if we make the callback itself handle it then we don't have to worry about it. Also, we would have less places we're trying to make redis connections, which is nice

@jacob720
Copy link
Contributor Author

That makes sense, I'll refactor

Copy link
Contributor

@DominicOram DominicOram left a comment

Choose a reason for hiding this comment

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

Thanks, couple of minor things, take them or leave them

return False

def start(self, doc: RunStart) -> RunStart | None:
if not self.redis_connected:
Copy link
Contributor

Choose a reason for hiding this comment

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

Could: It might be better to re-check the connection on start. For other callbacks we only create them once when we start up and never recreate them. I know we currently don't for MurkoCallback but if we did want to this would mean that we would have to restart the whole env to pick up redis coming back up

Comment on lines 629 to 630
assert patch_rotate_in_one_direction_and_stream_to_redis.call_count == 2
patch_rotate_in_one_direction_and_stream_to_redis.reset_mock()
Copy link
Contributor

Choose a reason for hiding this comment

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

Could: I don't think it's that clear with this patch that you're definitely testing the right outcomes. Instead I would do something like:

        omega_put = get_mock_put(smargon.omega.user_setpoint)

        assert omega_put.call_args_list == [
            call(360.0, wait=True),
            call(0.0, wait=True),
        ]

        omega_put.reset_mock()

and remove the patch of _rotate_in_one_direction_and_stream_to_redis all together

@DominicOram DominicOram enabled auto-merge (squash) December 19, 2025 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

murko Related to murko integration on i04

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Murko: Continue thawing if valkey is down

3 participants