-
Notifications
You must be signed in to change notification settings - Fork 5
Get feature flags from config service #1444
Conversation
src/hyperion/parameters/gridscan.py
Outdated
run_up_distance_mm=self.panda_runup_distance_mm, | ||
transmission_fraction=self.transmission_frac, | ||
) | ||
|
||
def do_set_stub_offsets(self, value: bool): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really like having a special private field in our parameter classes, and feel this is ugly but then we do so many other things in these - maybe this just says more about me than the code though...
return _CONFIG_SERVER | ||
|
||
|
||
def best_effort_get_feature_flag(flag_name: str, fallback: T = None) -> bool | T: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why does this method exist? get rid of it
set_stub_offsets: bool = False | ||
|
||
@classmethod | ||
def best_effort(cls): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should have a bulk get REST call rather than get the feature flags one by one, if we are always going to be getting them all in one go anyway.
set_stub_offsets: bool = False | ||
|
||
@classmethod | ||
def best_effort(cls): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method gets invoked every time we want only one of the feature flags, but it gets all of the feature flags. Why don't we just get all of the feature flags right at the start and put them all in the parameters in one go?
I realise that while it is a rest call this is just going to likely be quite fast, but at the end of the day we are in the long run ultimately trying to get end-to-end times of < 10s and say a few hundred ms extra if you end up with a lot of calls baked in to the code due to bad API design choices early on, it isn't completely insignificant.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comments with code
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lovely
* Add FeatureFlags class for params * Makes use of the daq-config-server to fetch data for it
Fixes #1397
Does a best effort attempt at getting feature flags (
set_stub_offsets
) from the new config serviceTo test: