[Feat] Improve validation of CapturedCallable

antonymilne commented 3 days ago

Description

~@petar-qb @huong-li-nguyen @maxschulz-COL would be great if one of you could do a quick 10 mins manual test just to check it works as expected. I've done pretty thorough testing and have lots of unit tests but would like to be extra sure I haven't broken something that was working well before (e.g. features dashboard). Just one person checking it in addition to me should be fine.~ Done, thank you @huong-li-nguyen!

Improved error messages

Before: CapturedCallable mode mismatch. Expected graph but got table. Now (small improvement): CapturedCallable was defined with @capture('table') rather than @capture('graph') and so is not compatible with the model.

Before: You must provide a valid CapturedCallable object. If you are using a plotly express figure, ensure that you are using import vizro.plotly.express as px. If you are using a table figure, make sure you are using from vizro.tables import dash_data_table. If you are using a custom figure or action, that your function uses the @capture decorator. Now (big improvement): Invalid CapturedCallable. Supply a function imported from vizro.plotly.express or defined with decorator @capture('graph')

Nice refactor

https://github.com/mckinsey/vizro/pull/340 introduced mode to CapturedCallable to ensure that e.g. you don't accidentally supply a dash_data_table to AgGrid. This was done using a reusable validator.

While working on this I realised things would be much cleaner and we can generate better error messages if we did the validation inside CapturedCallable validators itself, so I moved it. The control flow and responsibility of CapturedCallable's validation is now much clearer. The only CaptureCallable related validator that hasn't moved to CaptureCallable and so remains duplicated across Figure/Graph/Table/AgGrid is_process_callable_data_frame which would take more thought before moving so I haven't touched.

Notice

[x] 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.

huong-li-nguyen commented 3 days ago

Hello, first of all - this all sounds amazing! 👏 🚀

I've been manually testing all the demo dashboards (features/demo/KPI) and their yaml version; if it exists, all works fine. I've also been playing around with the dev example and I think I need more clarification. There are two cases where I would have expected error messages, but there are currently none. I might also not have the proper understanding here, so to clarify:

Case 1: Example with no capture call—This currently runs through without any errors, but the update_layout has no effect. I would have expected it to throw an error stating that I need to use the @capture("graph").

def f(data_frame):
    """Blah"""
    return px.bar(data_frame, "Actual").update_layout(title="x")

page = vm.Page(
    title="KPI Indicators",
    components=[
        vm.Graph(
            figure=f(df_kpi),
        ),
        vm.Figure(figure=kpi_card(data_frame=df_kpi, value_column="Actual", title="KPI with value")),
    ],
)

Case 2: Example with function directly provided to figure—this currently runs through without errors, but the update_layout has no effect. I would have expected that it would throw an error here that I need to use the @capture("graph"). I think users commonly try this. We did have a user question on this, and back then, they thought all the update_layout calls were not compatible with Vizro as it wasn't clear that people have to use a captured callable for any post update call.

page = vm.Page(
    title="KPI Indicators",
    components=[
        vm.Graph(
            figure=px.bar(df_kpi, "Actual").update_layout(title="x"),
        ),
        vm.Figure(figure=kpi_card(data_frame=df_kpi, value_column="Actual", title="KPI with value")),
    ],
)

antonymilne commented 3 days ago

Thank you for trying it all out! The two cases you raise are great ones to understand 😀

First of all, your two cases are really exactly the same. When there's no @capture decorator, calling f(df_kpi) is not doing anything other than producing px.bar(data_frame, "Actual").update_layout(title="x"). There's nothing magical happening here - it's just calling a normal Python function and getting the value. It doesn't matter where that value comes from.

What happens next depends on what your px import is:

if it's plotly.express then the value supplied to figure will have type go.Figure and this should raise an error telling you to use vizro.plotly.express instead
if it's vizro.plotly.express then the value supplied to figure will have type _DashboardReadyFigure and no error is raised

(please do check the above works for you as I expect it to depending on what your px import is)

When px is vizro.plotly.express, what happens is:

px.bar produces a _DashboardReadyFigure that is a subclass of go.Figure and has a _captured_callable attribute that contains the thing the dashboard actually needs to function
update_layout applies to the go.Figure part of this but does nothing to the _captured_callable attribute
hence if you put this code in a Jupyter notebook it will show the graph with updated layout as you would expect
but if you put it into a vm.Graph model the new CapturedCallable._extract_from_attribute extracts the _captured_callable attribute that doesn't have the updated layout attached in any way, so it doesn't work as expected in a dashboard

This is indeed very confusing and exactly the point about post-figure updates we discussed in https://github.com/McK-Internal/vizro-internal/issues/937 and that meeting a couple of weeks ago. Ideally we would still solve this case or raise a good error message but it's not a super quick and easy fix so we didn't do anything about it yet.

huong-li-nguyen commented 3 days ago

This is indeed very confusing and exactly the point about post-figure updates we discussed in McK-Internal/vizro-internal#937 and that meeting a couple of weeks ago. Ideally we would still solve this case or raise a good error message but it's not a super quick and easy fix so we didn't do anything about it yet.

Got it - thanks for clarifying!

Yeah, I see. Let's pick this up next sprint then!

mckinsey / vizro