[Feat] Enable dynamic filter #879
Conversation
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
…h minimal loading - tests pass
…ter.pre_build - tests pass
for more information, see https://pre-commit.ci
# Conflicts: # vizro-core/src/vizro/models/_controls/filter.py
…_manager to the Filter.pre_build data_manager _multi_load
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
| # Choose from 0-50 | ||
| setosa: 5 | ||
| versicolor: 10 | ||
| virginica: 15 | ||
|
|
||
| # Choose from: 4.8 to 7.4 | ||
| min: 5 | ||
| max: 7 | ||
|
|
||
| # Choose from: | ||
| # 2020-01-01 to 2020-05-29 | ||
| date_min: 2024-01-01 | ||
| date_max: 2024-05-29 |
There was a problem hiding this comment.
Adjusting these values, the input data is changed for the scratch/app.py example.
There was a problem hiding this comment.
By adjusting setosa, versicolor and virginica (from 0 to 50), example graphs that use the load_from_file_species will be affected (page_1, page_2, page_5, page_6).
By adjusting min, max (from 4.8 to 7.4), example graphs that use the load_from_file_sepal_length will be affected (page_3).
TODO: The following still doesn't work so enable page_4 and vm.DatePicker to support dynamic mode.
By adjusting date_min, date_max (from 2020-01-01 to 2020-05-29), example graphs that use the load_from_file_date_column will be affected (page_4).
There was a problem hiding this comment.
Love this method of testing, it's much better than just selecting random points and refreshing the page lots of times 💯
… into feat/dynamic-filter
| @@ -54,7 +54,7 @@ def validate_value(cls, value, values): | |||
| [entry["value"] for entry in values["options"]] if isinstance(values["options"][0], dict) else values["options"] | |||
| ) | |||
|
|
|||
| if value and not is_value_contained(value, possible_values): | |||
| if value and ALL_OPTION not in value and not is_value_contained(value, possible_values): | |||
There was a problem hiding this comment.
The possible_values here is always a self.options prop that does not contain "ALL", and this validation function is invoked from the _build_dynamic_placeholder method. Previously self.value was not set anywhere from code, so this didn't fail.
| # TODO: Align inner and outer ids to be handled in the same way as for other figure components. | ||
| selector_build_obj = self.selector.build() | ||
| return dcc.Loading(id=self.id, children=selector_build_obj) if self._dynamic else selector_build_obj |
There was a problem hiding this comment.
Is the inner/outer id mechanism here aligned with how it's done for other Vizro figure components?
| # TODO: Align inner and outer ids to be handled in the same way as for other figure components. | ||
| selector_build_obj = self.selector.build() | ||
| return dcc.Loading(id=self.id, children=selector_build_obj) if self._dynamic else selector_build_obj |
There was a problem hiding this comment.
Are there any UI improvements we should introduce until the "Universal Vizro placeholder component" comes?
| multi_data_source_name_load_kwargs: list[tuple[DataSourceName, dict[str, Any]]] = [ | ||
| (model_manager[target]["data_frame"], {}) for target in proposed_targets | ||
| ] |
There was a problem hiding this comment.
It's possible to send default DFP values from the model_manager into the data_manager._multi_load() with the following code:
(Disclaimer: The code is just a PoC and could be optimised a lot)
from vizro.models._controls import Parameter
multi_data_source_name_load_kwargs: list[tuple[DataSourceName, dict[str, Any]]] = []
# One tuple per filter.target
# [
# ('data_1', {'arg_1': 1, 'arg_2': 2,}),
# ('data_2', {'X': "ASD"}),
# ('data_2', {'X': "qwe"}),
# ]
# TODO-NEXT: The code below is just a PoC and could be improved a lot.
page_obj = model_manager[model_manager._get_model_page_id(model_id=ModelID(str(self.id)))]
for target in proposed_targets:
data_source_name = model_manager[target]["data_frame"]
load_kwargs = {}
for page_parameter in page_obj.controls:
if isinstance(page_parameter, Parameter):
for parameter_targets in page_parameter.targets:
if parameter_targets.startswith(f'{target}.data_frame'):
argument = parameter_targets.split('.')[2]
# argument is explicitly defined
if parameter_value := getattr(page_parameter.selector, 'value', None):
load_kwargs[argument] = parameter_value
# find default value
else:
parameter_selector = page_parameter.selector
default_parameter_value = None
if isinstance(parameter_selector, Dropdown):
default_parameter_value = parameter_selector.options if parameter_selector.multi else parameter_selector.options[0]
elif isinstance(parameter_selector, Checklist):
default_parameter_value = parameter_selector.options
elif isinstance(parameter_selector, RadioItems):
default_parameter_value = parameter_selector.options[0]
elif isinstance(parameter_selector, Slider):
default_parameter_value = parameter_selector.min
elif isinstance(parameter_selector, RangeSlider):
default_parameter_value = [parameter_selector.min, parameter_selector.max]
load_kwargs[argument] = default_parameter_value
multi_data_source_name_load_kwargs.append((data_source_name, load_kwargs))There was a problem hiding this comment.
Thanks for posting this here! Just to add my latest thoughts and FYI @maxschulz-COL.
Currently dynamic data functions require a default value for every argument. Even when there is a dataframe parameter, the default value is used when pre-build the filter e.g. to find the targets, column type (and hence selector) and initial values.
There are three possible solutions here:
- leave this "default value must be supplied" restriction so arguments passed to dynamic data function (through
_multi_load) are just{} - lift the restriction and take values used from the
model_managerto pass through to_multi_load. This should always work because a parameter always has a default value even if it's not explicitly specified by the user. It introduces more coupling with the model manager though. - lift the restriction and just don't prebuild anything much for a dynamic filter. This is simplest/best but probably doesn't work at the moment because you need to know
targetsupfront to build actions etc. And you can't obtaintargetswithout loading data, which requires some values for the parametrised dynamic data loading functions.
For now we're going for option 1 but may be revisited in future, especially after we've changed how model_manager works.
@petar-qb please could you put a TODO (no NEXT needed) in here just summarising and referring to this github comment? It can replace the existing TODO NEXT comment. Make sure that the reminder about static data needing {} remains though 🙏
There was a problem hiding this comment.
This is awesome work! 💯 As I said yesterday, the code changes here don't do credit to the many hours of understanding required to get this all working.
I've left quite a few initial comments and will then do another review. I think all is on track for approval though 👍 Let me know if you want to discuss any of the comments in a call or over slack and we can do so before Thursday.
Let me look through your questions from the PR description and answer them separately.
| # Choose from 0-50 | ||
| setosa: 5 | ||
| versicolor: 10 | ||
| virginica: 15 | ||
|
|
||
| # Choose from: 4.8 to 7.4 | ||
| min: 5 | ||
| max: 7 | ||
|
|
||
| # Choose from: | ||
| # 2020-01-01 to 2020-05-29 | ||
| date_min: 2024-01-01 | ||
| date_max: 2024-05-29 |
There was a problem hiding this comment.
Love this method of testing, it's much better than just selecting random points and refreshing the page lots of times 💯
| def _build_static(self, new_options=None, **kwargs): | ||
| options = new_options if new_options else self.options |
There was a problem hiding this comment.
Unless there's good reason to not do it this way let's make these arguments non-optional and just supply them all the time. Hiding the defaulting behaviour inside the _build_static function is more confusing I think.
| def _build_static(self, new_options=None, **kwargs): | |
| options = new_options if new_options else self.options | |
| def _build_static(self, options): |
Similar comment across all selectors.
There was a problem hiding this comment.
This is definitely a better approach. I did it for the options, min and max.
However, I'm still propagating the current_value to the numerical selectors only to support dcc.Input and dcc.Store components to work properly with the persistence in numerical selectors when they work in the dynamic mode. The current_value propagation can be removed when the dash persistence bug is fixed. (I added this as a comment in the filter.py where the current_value is propagated to the selector.__call__())
|
|
||
| def _build_static(self, new_options=None, **kwargs): | ||
| options = new_options if new_options else self.options | ||
| full_options, default_value = get_options_and_default(options=options, multi=True) |
There was a problem hiding this comment.
Is it right that we might change the value supplied here? Is this the case where it only works because we're lucky and we have the ALL option?
There was a problem hiding this comment.
Sorry, but I don't get you..
The get_options_and_default retrieves options without the "ALL" and returns the options with the "ALL" option (if the selector is multi=True). It also returns "ALL" as the default_value for the multi=True, and options[0] for the multi=False selectors.
| @@ -62,3 +67,15 @@ def build(self): | |||
| ), | |||
| ] | |||
| ) | |||
|
|
|||
| def _build_dynamic_placeholder(self): | |||
There was a problem hiding this comment.
Note to myself to read through this again to understand it.
| def _build_static(self, current_value=None, new_min=None, new_max=None, **kwargs): | ||
| _min = new_min if new_min else self.min | ||
| _max = new_max if new_max else self.max | ||
| init_value = current_value or self.value or _min |
There was a problem hiding this comment.
Can you remind me what's happening here? Why do we need current_value here and not just the min and max?
Also, following comment elsewhere let's do something like this unless there's good reason to not do so:
| def _build_static(self, current_value=None, new_min=None, new_max=None, **kwargs): | |
| _min = new_min if new_min else self.min | |
| _max = new_max if new_max else self.max | |
| init_value = current_value or self.value or _min | |
| def _build_static(self, value, min, max, **kwargs): | |
| init_value = value or min |
There was a problem hiding this comment.
As it's described in this thread, the current_value is here to enable persistence to work properly with the slider and range_slider.
These two selectors have a specific way of handling the persistence due to "two values bound problem". The current_value propagation can be removed when the dash bug is fixed.
I'll also write down all the potential improvements that will be possible after the dash persistence bugfix 😃
Definitely not urgent. Let's wait for this and fix plotly/dash#2678 first.
How many of these will still be problems once we fix plotly/dash#2678?
Good idea - let's discuss how this works now and how it should work.
Added to the comment there but basically let's leave as a TODO and open a ticket. Not urgent.
Would love to do this soon (like by end of year if possible).
Not as high priority - let's make a ticket and forget for now.
Good question but I think not super high priority unless we have an immediate use for it.
Very good question and presumably required for us to get DFPs to update filters without reloading the page? So this one needs to be worked out soon.
Let's discuss but I think basically the "common sense" way that we initially expected things to work.
Not sure I understand this, please could you explain? Basically I think roughly ordered priorities are:
wdyt? |
Description
This PR introduces
dynamicfilters for the following selectors (so, all Vizro selectors exceptvm.DatePicker):vm.Dropdown,vm.RadioItems,vm.Checklist,vm.Sliderandvm.RangeSliderYou can test the feature by altering values in the
scratch_dev/data.yaml(in the way that's described in the comment) and running thescratch_dev/app.py.TODOs:
This PR TODOs:
This or next PR TODOs (we should decide it):
vm.DatePicker. (Maybe it's better to wait for the dash persistence bugfix as persistence doesn't work for the vm.DatePicker even on themainbranch)Next PRs:
selector.valuehandling for new users. There are a few cases marked with 🟠 in this Issue -> https://github.com/McK-Internal/vizro-internal/issues/1356. There are two inconsistencies listed at the bottom of the Issue description. This TODO points to thesecondinconsistency.multi=False vm.Dropdowndynamic selector bug when the value is cleared. This will be solved when Vizro universal placeholder component become introduced.data_frame Parameterdefault values from themodel_managerinto theDM._multi_load()that's called from thevm.Filter.pre_build(). -> PoC can be found in the comment.References:
Open questions:
update_figures(targets=[”filter_1_id”])data_frameproperty for the vm.Filter component, and if thedata_frame Parameterschange the targeting form todata_manager_key.function_argument, does is mean that our dynamic Filters could be handled in a same way as any other dynamic figure components?Notice
I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":