Skip to content

Ensure backend creates sender callbacks with the correct controller#134

Closed
GDYendell wants to merge 1 commit intomainfrom
130-handler-controller-fix
Closed

Ensure backend creates sender callbacks with the correct controller#134
GDYendell wants to merge 1 commit intomainfrom
130-handler-controller-fix

Conversation

@GDYendell
Copy link
Contributor

@GDYendell GDYendell commented Mar 11, 2025

Fixes #130

This is not the ideal fix, but tried many different things and they were all quite horrible:

  • Adding the controller back into ControllerAPI - the transport should not have access to it
  • Adding the ScanCallbacks to ControllerAPI - this just doesn't really make sense, only the Backend needs them
  • Make the Backend manually parse methods and attribute out of the Controller and only create the ControllerAPI at the end - this was OK, but meant repeating this code multiple times.

I think the interface of the handlers may not be correct, which is making this painful. It doesn't seem necessary for the whole controller to be passed into the handler put/update methods.

I don't know why that tango test is suddenly failing on 3.12 😢 It doesn't happen on main or locally.

@GDYendell GDYendell requested review from evvaaaa and jsouter March 11, 2025 15:27
@codecov
Copy link

codecov bot commented Mar 11, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.56%. Comparing base (759fa13) to head (7622301).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #134      +/-   ##
==========================================
+ Coverage   90.52%   90.56%   +0.04%     
==========================================
  Files          41       41              
  Lines        1920     1930      +10     
==========================================
+ Hits         1738     1748      +10     
  Misses        182      182              

☔ 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.

@evvaaaa
Copy link
Contributor

evvaaaa commented Mar 12, 2025

It doesn't seem necessary for the whole controller to be passed into the handler put/update methods.

I agree. I think that if a controller is necessary for the Handler to function then it should be an instance member on the Handler.

@evvaaaa
Copy link
Contributor

evvaaaa commented Mar 12, 2025

I don't know why that tango test is suddenly failing on 3.12 😢 It doesn't happen on main or locally.

Feel free to cherry pick this.

I remember having this problem in ophyd-async too, I'm looking through pytango issues now.

Edit

I was thinking of a DeprecationWarning on pytango 3.12. This problem seems more fundamental to some slow cleanup when exiting a test... I tried playing around with the fixtures to no avai.

Main still passes, so this failure is probably a consequence of the new test added here.

Copy link
Contributor

@evvaaaa evvaaaa left a comment

Choose a reason for hiding this comment

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

GTM once CI is passing...

@jsouter
Copy link
Contributor

jsouter commented Mar 13, 2025

EigerHandler.update is still receiving the top controller in fastcs-eiger with this fix, I think you also need to update _get_scan_coros to use the correct controller.

Copy link
Contributor

@jsouter jsouter left a comment

Choose a reason for hiding this comment

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

Something like

def _get_scan_coros(
    root_controller_api: ControllerAPI, root_controller: Controller
) -> list[Callable]:
    scan_dict: dict[float, list[Callable]] = defaultdict(list)

    for controller_api in root_controller_api.walk_api():
        controller = root_controller.get_controller_by_path(controller_api.path)
        _add_scan_method_tasks(scan_dict, controller_api)
        _add_attribute_updater_tasks(scan_dict, controller_api, controller)

    scan_coros = _get_periodic_scan_coros(scan_dict)
    return scan_coros

@GDYendell
Copy link
Contributor Author

Closing in preference for #135

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sender/Updater/Handler now incorrectly take the root controller

3 participants