Skip to content

update stale_parameter of top controller in subcontroller update#60

Merged
jsouter merged 7 commits intomainfrom
stale
Mar 21, 2025
Merged

update stale_parameter of top controller in subcontroller update#60
jsouter merged 7 commits intomainfrom
stale

Conversation

@jsouter
Copy link
Contributor

@jsouter jsouter commented Mar 4, 2025

Closes #54

tested against eiger sim, will try and test against a real detector soon

@codecov
Copy link

codecov bot commented Mar 4, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.30%. Comparing base (a1ac38c) to head (bff6ef5).
Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #60      +/-   ##
==========================================
+ Coverage   84.61%   85.30%   +0.68%     
==========================================
  Files           4        4              
  Lines         312      313       +1     
==========================================
+ Hits          264      267       +3     
+ Misses         48       46       -2     

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

Copy link
Contributor

@GDYendell GDYendell left a comment

Choose a reason for hiding this comment

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

Keep the naming consistent between the method and attribute / parent and child just to make it clear they are all the same thing.

Remove the subsystem stale parameters as discussed and then I think it should be good to go.

@GDYendell
Copy link
Contributor

GDYendell commented Mar 6, 2025

I have realised this idea is still not quite right, because the sub controllers will fight over setting stale parameters and it could get set False by one sub controller when another is still stale.

I think we also need to have only one queue and have the top level do the updates for the queue and set stale_parameters. So what we pass down to the sub_controllers is the queue. And perhaps that queue should store attributes, rather than parameter names. What do you think @jsouter ?

@jsouter
Copy link
Contributor Author

jsouter commented Mar 6, 2025

I have realised this idea is still not quite right, because the sub controllers will fight over setting stale parameters and it could get set False by one sub controller when another is still stale.

Yeah, that's why I thought we might as well keep the stale_parameters attribute on the subcontrollers since the top controller needs to keep track of all their states anyway.

I think we also need to have only one queue and have the top level do the updates for the queue and set stale_parameters. So what we pass down to the sub_controllers is the queue. And perhaps that queue should store attributes, rather than parameter names. What do you think @jsouter ?

I can have a try, I think it makes sense to still have the queue separate the parameters for each subsystem, I don't believe there's any response to any put request that suggests that you need to update a parameter belonging to a different subsystem.

@GDYendell
Copy link
Contributor

GDYendell commented Mar 6, 2025

I don't believe there's any response to any put request that suggests that you need to update a parameter belonging to a different subsystem

This is true, but if they are each fetching parameters in their own queue then can't just set the parent stale_parameters because there could be two subsystems that are stale at the same time and the first one that finishes fetching will wrongly set the top level to False. It needs to be an AND of the subsystem states, but also be synchronous and not updated on a poll loop.

@jsouter
Copy link
Contributor Author

jsouter commented Mar 7, 2025

This is true, but if they are each fetching parameters in their own queue then can't just set the parent stale_parameters because there could be two subsystems that are stale at the same time and the first one that finishes fetching will wrongly set the top level to False. It needs to be an AND of the subsystem states, but also be synchronous and not updated on a poll loop.

Okay, now have it so that the subcontrollers queue_updates sets the stale parameter and adds the config_update coros to an asyncio.Queue that is handled by the top controller which clears stale when the queue is empty. Now the subcontrollers don't need or have an update method of their own.

@jsouter jsouter force-pushed the stale branch 3 times, most recently from 20c383e to ff0ced8 Compare March 7, 2025 11:57
James Souter added 2 commits March 18, 2025 10:12
… to queue updates

Remove stale_parameters from subcontrollers
…ock and stale_parameter to subsystem controllers
Copy link
Contributor

@GDYendell GDYendell left a comment

Choose a reason for hiding this comment

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

I think we are close now!

@jsouter jsouter requested a review from GDYendell March 19, 2025 13:35
Copy link
Contributor

@GDYendell GDYendell left a comment

Choose a reason for hiding this comment

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

Thanks for iterating on this with me! I think it has turned out pretty nicely.

Merge when you are ready. It will break main when merged, though, because setuptools_dso is broken.

@jsouter
Copy link
Contributor Author

jsouter commented Mar 21, 2025

Thanks for iterating on this with me! I think it has turned out pretty nicely.

Merge when you are ready. It will break main when merged, though, because setuptools_dso is broken.

No problem! I've got another local branch to bring it up to compatibility with new FastCS, though I'll wait until something has been merged to settle how the handlers deal with controllers.

@jsouter jsouter merged commit 1ffabfe into main Mar 21, 2025
19 checks passed
@jsouter jsouter deleted the stale branch March 21, 2025 10:40
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.

Stale Parameters is now not set immediately after put

2 participants