[Tidy] Convert actions to classes #363

petar-qb · 2024-03-12T13:51:56Z

Description

For those who will review this PR:

Here is a small recommendation on how to familiarise yourself with the PR changes in the fastest way and how to review the PR. It is only a recommendation and feel free to skip some of the suggested steps if you are already familiar with them or if you are not interested in them.

Don't rush to review the changes right away.
Read the entire PR description.
Watch the Team Learning session.
Run all examples in vizro-core/examples/_dev/ folder. They are already sorted alphabetically in the way they should run. For each example: read file docstrings, explore application configuration, and run and play with application UI.
Investigate CapturedActionCallable → useful Gist that @antonymilne made: https://gist.github.com/antonymilne/174c4c49ace2dae1f1d7674183b8d140
Link to the PS session -> https://mckinsey.box.com/s/fg1sdrpw6emprihhtg19ww4lft2ljma8
Open and understand one of the action implementations (e.g. export_data_action)
Search for TODO-AV2 and read everything. (especially since some "TODO-AV2" refer to multiple similar pieces of code, but are only written in one place)
Start with discussing open questions ("TODO-AV2-OQ")
Feel completely free to contribute:
- Implementing some better architecture solutions,
- Open any new kind of question.
- Solving TODOs,
- Cleaning the code,
- Applying linting stuff,
- Fixing/adding tests
- Literally anything else
Start with the standard code reviewing.

This PR is in the MVP phase because:

There are still some open questions that should be discussed.
The code is not completely tidied up.
tests, docs and linting is not completed.

The most important changes:

Implementation of class based action using CapturedActionCallable.
Actions validation and eager input arguments calculation.
Make filter_action and parameter_action public,
Change _on_page_load with a public update_figures action.
Expose Page.actions so it can be overwritten.
model_manager._get_model_page_id method improved

Bugfixes:

Enable any Vizro model to work with actions in the control panel (Custom actions do not work when applied through a component that gets added to a union such as ControlType #376) (model_manager._get_page_actions_chains improved)
export_data is not able anymore to export data from other pages anymore (the bug is not spotted earlier and is fixed in this PR),

TODO-AV2 legend:

TODO-AV2 - I will solve these as the part of this PR,
TODO-AV2-OQ- Open Questions,
TODO-AV2-TICKET-CREATED - These will be solved as a separate PR, and the ticket for these is already created,
TODO-AV2-TICKET-NEW - These will be solved as a separate PR, and the following tickets have to be created,
*- tagged TODOs (contain the characters -*:) are candidate for solving by anyone from the team.

PR TODOs:

the final tidying up (getting rid of code duplications and some TODOs)
tests
docs

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.

… and arguments preparation

…tions_to_class

for more information, see https://pre-commit.ci

…izro into tidy/actions_to_class

for more information, see https://pre-commit.ci

antonymilne · 2024-03-13T17:06:21Z

vizro-core/src/vizro/actions/export_data_class_action.py

+
+
+class ExportDataClassAction(CapturedActionCallable):
+    def __init__(self, *args, **kwargs):


What's this __init__ for?

I used it to save input args and kwargs so they can be validated/adjusted in the _post_init.
Now, I got rid of the constructor and self._arguments is used inside the _post_init.

antonymilne · 2024-03-13T17:10:39Z

vizro-core/src/vizro/models/_action/_action.py

        if self.inputs:
            callback_inputs = [State(*input.split(".")) for input in self.inputs]
+        elif hasattr(self.function, "inputs") and self.function.inputs:


I guess we're going to have a few switches here while we have both the "old" function actions and the new class ones.

Let's have a consistent way of doing this everywhere to make it clearer. I think just isinstance(function, CapturedActionCallable) would work? No need to check if self.function.inputs is Falsey or not I think either.

I guess we're going to have a few switches here while we have both the "old" function actions and the new class ones.

You're right, and after all actions become implemented as CapturedActionCallable, then the following line will be removed:
else: callback_inputs = _get_action_callback_mapping(action_id=ModelID(str(self.id)), argument="inputs")

antonymilne · 2024-03-13T17:13:09Z

vizro-core/src/vizro/actions/export_data_class_action.py

+
+        # Validate and calculate "targets"
+        targets = self._kwargs.get("targets")
+        if targets:


This feels like an improvement on the old function version because we now validate targets upfront rather than at runtime. Is that right?

Yep, that's right! 😄

antonymilne · 2024-03-13T17:13:58Z

vizro-core/src/vizro/actions/export_data_class_action.py

+        # Fake initialization - to let other actions see that this one exists.
+        super().__init__(*args, **kwargs)
+
+    def _post_init(self):


Should this be called directly in __init__ or is that too early?

It's too early to call _post_init validation inside the action initialisation phase. To be able to validate action input arguments properly, all dashboard models have to be initialised. Some of the models are initialised in the _pre_build phase of other models which means that the actions _post_init has to be called within the build phase.

antonymilne · 2024-03-13T17:16:26Z

vizro-core/src/vizro/actions/export_data_class_action.py

+                    raise ValueError(f"Component '{target}' does not exist on the page '{self._page_id}'.")
+        else:
+            targets = model_manager._get_page_model_ids_with_figure(page_id=self._page_id)
+        self._kwargs["targets"] = self.targets = targets


Not sure I understand what's happening with the _kwargs stuff here, please could you explain?

Now this line looks like this:
self._arguments["targets"] = self.targets = targets

targets - represents new validated and calculated action's targets

self._arguments["targets"] = targets - overwrites the pure_function input argument. Since the pure_function is a staticmethod (and I'm pretty sure it should remain), new calculated targets have to be propagated through the self._arguments.

self.targets = targets - Optionally, some of CapturedActionCallable attributes are also calculated in the _post_init phase. In this case, self.targets is created calculated so it can be easily reused inside the outputs and components` calculations.

for more information, see https://pre-commit.ci

…izro into tidy/actions_to_class

for more information, see https://pre-commit.ci

petar-qb added 5 commits March 4, 2024 09:35

First very raw version of class based action function with validation…

6cbf33d

… and arguments preparation

Minor improvements

df0c7cf

Minor refactoring

8fac8e5

PoC - Validation and eager input arguments calculation

9851cb9

Merge branch 'main' of https://github.com/mckinsey/vizro into tidy/ac…

87db5d7

…tions_to_class

petar-qb requested review from Joseph-Perkins, antonymilne, huong-li-nguyen, maxschulz-COL and Anna-Xiong as code owners March 12, 2024 13:51

pre-commit-ci bot and others added 5 commits March 12, 2024 13:52

[pre-commit.ci] auto fixes from pre-commit.com hooks

7b8a6c3

for more information, see https://pre-commit.ci

Minor changes

c40ea85

Merge branch 'tidy/actions_to_class' of https://github.com/mckinsey/v…

99b684d

…izro into tidy/actions_to_class

Minor change

0c260e6

[pre-commit.ci] auto fixes from pre-commit.com hooks

76fe66e

for more information, see https://pre-commit.ci

huong-li-nguyen marked this pull request as draft March 12, 2024 15:01

huong-li-nguyen assigned petar-qb Mar 12, 2024

huong-li-nguyen added the Component: Refactoring 🦸 label Mar 12, 2024

antonymilne reviewed Mar 13, 2024

View reviewed changes

petar-qb and others added 11 commits March 28, 2024 08:27

Fixing a bug in model_manager._get_model_page_id + More refactoring

6d88c50

[pre-commit.ci] auto fixes from pre-commit.com hooks

400ac60

for more information, see https://pre-commit.ci

All actions moved to the class based approach

b823e6b

[pre-commit.ci] auto fixes from pre-commit.com hooks

31bd5a3

for more information, see https://pre-commit.ci

Small TODO refactoring

0c65e2c

Solving conflicts

d0eac24

[pre-commit.ci] auto fixes from pre-commit.com hooks

1107486

for more information, see https://pre-commit.ci

Small TODOs changes

caf5012

Exposing vm.Page.actions argument

6b15541

[pre-commit.ci] auto fixes from pre-commit.com hooks

4d2ba9a

for more information, see https://pre-commit.ci

Examples and other changes added

f1235c4

pre-commit-ci bot and others added 6 commits April 25, 2024 15:12

[pre-commit.ci] auto fixes from pre-commit.com hooks

c8a2618

for more information, see https://pre-commit.ci

Conflicts solved

de20b21

Merge branch 'tidy/actions_to_class' of https://github.com/mckinsey/v…

80238ed

…izro into tidy/actions_to_class

Reverting docstrings for examples files

bedf430

Add more validation + Clearning some TODO-AV2

3ed5f86

[pre-commit.ci] auto fixes from pre-commit.com hooks

387ce00

for more information, see https://pre-commit.ci

petar-qb added the Status: Ready for Review ☑️ label Apr 26, 2024

huong-li-nguyen removed the Status: Ready for Review ☑️ label May 8, 2024

huong-li-nguyen removed the Tidy/Refactoring 🧹 label Jul 9, 2024

huong-li-nguyen changed the title ~~Tidy/actions to class~~ [Tidy] Convert actions to classes Jul 9, 2024

petar-qb mentioned this pull request Aug 21, 2024

Custom action on Page loading #643

Closed

1 task

This was referenced Oct 9, 2024

Dynamic data with sliders stops working with added action #799

Closed

Relationship between control's targets and its actions outputs #800

Closed

This was referenced Nov 15, 2024

[Tidy] Proof of concept replacing ctd for filter and parameter #880

Closed

[Feat] Enable dynamic filter #879

Merged

This was referenced Nov 28, 2024

[Feat] Recognise controls outside page.controls #903

Merged

[POC] Actions to class #911

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Tidy] Convert actions to classes #363

[Tidy] Convert actions to classes #363

petar-qb commented Mar 12, 2024 •

edited

Loading

antonymilne Mar 13, 2024

petar-qb Mar 28, 2024

antonymilne Mar 13, 2024

petar-qb Mar 28, 2024

antonymilne Mar 13, 2024

petar-qb Mar 28, 2024

antonymilne Mar 13, 2024

petar-qb Mar 28, 2024

antonymilne Mar 13, 2024

petar-qb Mar 28, 2024



		class ExportDataClassAction(CapturedActionCallable):
		def __init__(self, args, *kwargs):

[Tidy] Convert actions to classes #363

Are you sure you want to change the base?

[Tidy] Convert actions to classes #363

Conversation

petar-qb commented Mar 12, 2024 • edited Loading

Description

For those who will review this PR:

This PR is in the MVP phase because:

The most important changes:

Bugfixes:

TODO-AV2 legend:

PR TODOs:

Notice

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petar-qb commented Mar 12, 2024 •

edited

Loading