Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feat] Recognise controls outside page.controls #903

Merged
merged 18 commits into from
Dec 2, 2024
Merged

Conversation

antonymilne
Copy link
Contributor

@antonymilne antonymilne commented Nov 28, 2024

Description

Following a conversation with @huong-li-nguyen and @maxschulz-COL, this should make the functionality of control groups as in #82 work again - see scratch_dev example.

While I was at it I did some tidying and refactoring to the model_manager:

  • now there's a function to look up the page of a given model and a function to get all the models of one type on a given page
  • we return models rather than model IDs since you can still get model.id directly from the model itself and it means we don't need lots of the model_manager[model_id] stuff we did before
  • some of these changes to actions might not be a good idea and could change back again in work like [Tidy] Convert actions to classes #363 but let's see
  • the model_manager changes are an improvement but definitely need more work - hopefully @maxschulz-COL will work on this in the nearish future after we're on pydantic v2

@petar-qb please could you check the failing unit test and see if it's a problem or not? I think there was a bug before in the model_manager so don't know if I've fixed that bug or broken something else or if it doesn't matter. Here's the full mesage: https://gist.github.com/antonymilne/7f3a3015772f0c992dd8b1194e6f9489.

Notice

  • I acknowledge and agree that, by checking this box and clicking "Submit Pull Request":

    • I submit this contribution under the Apache 2.0 license and represent that I am entitled to do so on behalf of myself, my employer, or relevant third parties, as applicable.
    • I certify that (a) this contribution is my original creation and / or (b) to the extent it is not my original creation, I am authorized to submit this contribution on behalf of the original creator(s) or their licensees.
    • I certify that the use of this contribution as authorized by the Apache 2.0 license does not violate the intellectual property rights of anyone else.
    • I have not referenced individuals, products or companies in any commits, directly or indirectly.
    • I have not added data or restricted code in any commits, directly or indirectly.

Copy link
Contributor

github-actions bot commented Nov 28, 2024

View the example dashboards of the current commit live on PyCafe ☕ 🚀

Updated on: 2024-12-02 17:30:20 UTC
Commit: e3360b1

Link: vizro-core/examples/dev/

Link: vizro-core/examples/scratch_dev

Link: vizro-core/examples/visual-vocabulary/

Link: vizro-ai/examples/dashboard_ui/

@github-actions github-actions bot added the Vizro-AI 🤖 Issue/PR that addresses Vizro-AI package label Nov 28, 2024
@antonymilne antonymilne changed the title Surprisingly big refactor [CI] Recognise controls outside page.controls Nov 28, 2024
@antonymilne antonymilne marked this pull request as ready for review November 28, 2024 11:15
@antonymilne antonymilne changed the title [CI] Recognise controls outside page.controls [Feat] Recognise controls outside page.controls Nov 28, 2024
Copy link
Contributor

@huong-li-nguyen huong-li-nguyen left a comment

Choose a reason for hiding this comment

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

Little refactoring, but big change! 🚀 This is amazing and fixes so many limitations that we've encountered this week.

Understood the main change, and checked all our demos, but I'll leave the action unit tests to @petar-qb :)

@antonymilne antonymilne mentioned this pull request Nov 29, 2024
20 tasks
@antonymilne antonymilne mentioned this pull request Nov 29, 2024
1 task
Copy link
Contributor

@petar-qb petar-qb left a comment

Choose a reason for hiding this comment

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

This is a huge improvement! It's so intuitive to use model_manager helper methods now. 🎖️

vizro-ai/hatch.toml Outdated Show resolved Hide resolved
@@ -32,21 +32,27 @@ def _get_inputs_of_controls(page: Page, control_type: ControlType) -> list[State
"""Gets list of `States` for selected `control_type` of triggered `Page`."""
return [
State(component_id=control.selector.id, component_property=control.selector._input_property)
for control in page.controls
if isinstance(control, control_type)
for control in cast(Iterable[ControlType], model_manager._get_models(control_type, page))
Copy link
Contributor

Choose a reason for hiding this comment

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

Would the _get_inputs_of_controls be even more flexible if we searched for all _filter/_parameter Action functions on the page (instead for vm.Filter/vm.Parameter components)? In that case if a _filter action (or its future public counterpart e.g: filter) is assigned to the pure dcc.Dropdown(), this component would behave exactly like the vm.Filter (except the _dynamic behaviour and potentially some other.).

This is more like a theoretical question and I guess its answer mostly depends on how we see Vizro backend in future and it also depends on many other decisions (e.g. how to make a standalone dcc.Dropdown to be dynamic, and many many more). Currently, I can't say what exactly are all the pros/cons of this approach and is any new use-case covered with this approach or another. So, this comment here is just an idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question! I'm actually thinking about this already in #363 right now and not yet sure or how to handle it, so let me keep this unresolved to come back to.

vizro-core/src/vizro/managers/_model_manager.py Outdated Show resolved Hide resolved
@petar-qb
Copy link
Contributor

@antonymilne

please could you check the failing unit test and see if it's a problem or not? I think there was a bug before in the model_manager so don't know if I've fixed that bug or broken something else or if it doesn't matter. Here's the full message:

The test fails because the _get_action_loop_components method now returns a list of dcc.Store and html.Div components in a "random" order , due to how elements are returned from the model_manager._get_models(). So, the assertion assert_component_equal(result, expected) fails because it expects a specific sequence of elements in the list.

Also, couple of these dcc.Store elements have more complex "data" content (e.g. list[str], dict[str, list[str]]) that's also randomised now, which complicates refactoring the test. (It's important that this order of elements does not impact the dashboards' functionality.)

After several refactoring attempts I ended with this stupid solution:

    def test_all_action_loop_components(
        self,
        fundamental_components,
        gateway_components,
        action_trigger_components,
        action_trigger_actions_id_component,
        trigger_to_actions_chain_mapper_component,
    ):
        result = _get_action_loop_components()

        # Validate the action_loop_components_div wrapper
        assert_component_equal(result, html.Div(id="action_loop_components_div"), keys_to_strip={"children"})

        # Validate the children of the result component.
        # Since the order of children is not guaranteed due to how elements are retrieved
        # from the model_manager, we perform order-agnostic comparisons.
        #
        # * fundamental_components, gateway_components, and action_trigger_components:
        #   These are dcc.Store objects with simple "data" attributes, so we compare their string
        #   representations directly.
        # * action_trigger_actions_id_component and trigger_to_actions_chain_mapper_component:
        #   These components have more complex attributes, so we compare their `id` and `data`
        #   fields explicitly. "data" is compared as a set to ignore order.

        # Test: fundamental_components + gateway_components + action_trigger_components
        expected_children = fundamental_components + gateway_components + action_trigger_components
        result_children = result.children[:len(expected_children)]
        assert {str(elem) for elem in expected_children} == {str(elem) for elem in result_children}

        # Test: action_trigger_actions_id_component
        index_action_trigger = len(expected_children)
        action_trigger_result = result.children[index_action_trigger]
        assert action_trigger_actions_id_component.id == action_trigger_result.id
        assert set(action_trigger_actions_id_component.data) == set(action_trigger_result.data)

        # Test: trigger_to_actions_chain_mapper_component
        index_trigger_mapper = index_action_trigger + 1
        trigger_mapper_result = result.children[index_trigger_mapper]
        assert trigger_to_actions_chain_mapper_component.id == trigger_mapper_result.id
        assert set(trigger_to_actions_chain_mapper_component.data) == set(trigger_mapper_result.data)

Copy link
Contributor

@maxschulz-COL maxschulz-COL left a comment

Choose a reason for hiding this comment

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

🚀 This will unlock quite a bit quite soon (ie today) 🚀

antonymilne and others added 7 commits December 2, 2024 16:03
…el_manager

# Conflicts:
#	vizro-ai/hatch.toml
#	vizro-core/examples/scratch_dev/app.py
#	vizro-core/src/vizro/models/_controls/filter.py
#	vizro-core/src/vizro/models/_page.py
@antonymilne antonymilne enabled auto-merge (squash) December 2, 2024 17:45
@antonymilne antonymilne merged commit 14b1aad into main Dec 2, 2024
36 checks passed
@antonymilne antonymilne deleted the tidy/model_manager branch December 2, 2024 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Vizro-AI 🤖 Issue/PR that addresses Vizro-AI package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants