[Tidy] Tidy up build method of vm.Table and vm.Grid

[maxschulz-COL]

Description

Small tidy of build method to reuse dunder call method instead of calling self.figure

Screenshot

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.

[huong-li-nguyen]

Looks a lot cleaner, but do you mind explaining why this is a better approach to what we've done before? I don't think I have enough context to evaluate these code changes 😄

Also this comment: " The pagination setting (and potentially others) only work when the initially built AgGrid has the same setting as the object that is built on-page-load and rendered finally."

When and how does it affect the user? Do we need to add it to the docs (if not already done)? e.g. I am currently not clear what doesn't work and how I need to fix it as a user?

[petar-qb]

Now everything is in its place. This looks perfect! 😍 🚀

[maxschulz-COL]

Looks a lot cleaner, but do you mind explaining why this is a better approach to what we've done before? I don't think I have enough context to evaluate these code changes 😄

This is a tidy, so it is just a cleaner approach to the same thing: we call the provided function, insert the function arguments, exchange any supplied DF and an id, then render it in the initial build method. Before we did that by calling self.figure and self.figure._arguments (or similar), and manually exchanged the required things to match the __call__ method, now we just actually call the dunder method.

For the table, this was not necessary (we never discovered a bug), but we now do it just to be consistent

Also this comment: " The pagination setting (and potentially others) only work when the initially built AgGrid has the same setting as the object that is built on-page-load and rendered finally."

When and how does it affect the user? Do we need to add it to the docs (if not already done)? e.g. I am currently not clear what doesn't work and how I need to fix it as a user?

The user doesn't need to do anything, that is why it is not in the docs, but just a code comment to remind us of something quirky we discovered in the past. Pretty much what the comment says, hence the reason why we use the provided function with the provded args so that the settings match.

[huong-li-nguyen]

Looks a lot cleaner, but do you mind explaining why this is a better approach to what we've done before? I don't think I have enough context to evaluate these code changes 😄

This is a tidy, so it is just a cleaner approach to the same thing: we call the provided function, insert the function arguments, exchange any supplied DF and an id, then render it in the initial build method. Before we did that by calling self.figure and self.figure._arguments (or similar), and manually exchanged the required things to match the __call__ method, now we just actually call the dunder method.

For the table, this was not necessary (we never discovered a bug), but we now do it just to be consistent

Also this comment: " The pagination setting (and potentially others) only work when the initially built AgGrid has the same setting as the object that is built on-page-load and rendered finally."
When and how does it affect the user? Do we need to add it to the docs (if not already done)? e.g. I am currently not clear what doesn't work and how I need to fix it as a user?

The user doesn't need to do anything, that is why it is not in the docs, but just a code comment to remind us of something quirky we discovered in the past. Pretty much what the comment says, hence the reason why we use the provided function with the provded args so that the settings match.

Thanks for explaining!

I think that's the thing, I don't understand the code comment from just reading it. What's the quirky behaviour you've observed in the past? "And potentially others" - Is it only affecting functionality where we have defined the arguments in the defaults? Imagine someone reading this, who has no context (in this case me) as I haven't reviewed your table PRs😄

[maxschulz-COL]

I think that's the thing, I don't understand the code comment from just reading it. What's the quirky behaviour you've observed in the past? "And potentially others" - Is it only affecting functionality where we have defined the arguments in the defaults? Imagine someone reading this, who has no context (in this case me) as I haven't reviewed your table PRs😄

I struggle to describe it any other way, but let me try 😄 :

This build method is usually overwritten by the on-page-load, so in principle it is possible to put anything here. Before the AgGrid, we actually just put an empty DataTable here (and for the charts an empty figure).

With the AgGrid we realised that the settings of this initially built object, and the final one by on-page-load need to match for the pagination to work. Or in other words, when the inital AgGrid has pagination=False and the user puts pagination=True it doesn't work, but when they match it does. Hence we decided that the natural thing is to build the exact same AgGrid initially (bar the DF). This is what I meant with quirky behaviour above.

Potentially others refers to the unknown - we are just aware of the pagination, but the experience there taught us that there might be more, so that is why we just transfer all the settings, and not just the pagination settings.

In summary: "The pagination setting (and potentially others) only work when the initially built AgGrid has the same setting as the object that is built on-page-load and rendered finally." 😜

One could also argue that it is probably generally natural to build the initial object and the final object with the same setting, so the way it is now is good regardless of the bug. Hence why I think a small comment suffices warning us that it should not be taken away without thinking about this, but it is also not such a fundamental thing that warrants a very detailed explanation.

If you have a suggestion for a better comment, then let's implement that! Happy to put whatever would make it clearer

[huong-li-nguyen]

If you have a suggestion for a better comment, then let's implement that! Happy to put whatever would make it clearer

Let me try to rephrase in my own words and share one observation 😄

So from what I understood and tried to replicate in code: The quirky behaviour you've explained happens in the scenario below. In that case, the app does not break, but the table is empty. However, if we remove the dashGridOptions={"pagination": False} from the build, then all works fine. Did I understand that correctly?

1. In the build of the ag-grid we provide arguments e.g. pagination=False:
   html.Div(self.__call__(data_frame=pd.DataFrame(), dashGridOptions={"pagination": False}), id=self.id, className="table-container")
2. In the app configuration the user deviates from that default and sets:

vm.AgGrid(
            id="custom_ag_grid",
            title="Custom Dash AgGrid",
            figure=dash_ag_grid(data_frame=df, dashGridOptions={"pagination": True}),
        )

When I tried to replicate that quirky behavior, I've noticed that what the user inputs actually does not matter for that quirky behavior to be visible. What's leading to the quirky behavior seems to be a deviation from what we insert as arguments to the build and the default settings that are defined here.

For example, the user could configure pagination=True in their ag-grid function, and we could define pagination=False in the ag-grid build and this would actually work as long as the default settings here match. What I mean is the following scenario:

1. User Input:

 vm.AgGrid(
            id="custom_ag_grid",
            title="Custom Dash AgGrid",
            figure=dash_ag_grid(data_frame=df, dashGridOptions={"pagination": True}),
        ),

2. Default setting in _dash_ag_grid.py:

        "dashGridOptions": {
            "dataTypeDefinitions": _DATA_TYPE_DEFINITIONS,
            "animateRows": False,
            "pagination": False,
        },

3. Argument setting in dash-ag grid build:

html.Div(self.__call__(data_frame=pd.DataFrame(), dashGridOptions={"pagination": False}), id=self.id, className="table-container"),

Above example works fine even though the user input deviates from the argument setting in the build. So it does not seem to be the deviation to the user configuration that leads to that quirky behaviour but a deviation to the defaults of the ag-grid.

In that case I would rephrase the comment as follows:

Do not provide any default arguments here, but use the dict inside the _dash_ag_grid.py instead, as deviations from the default arguments here can lead to the table not being displayed correctly.

[antonymilne]

Thanks for making the change 🙏

[antonymilne]

Not related to this change so feel free to ignore here, but just spotted a weird looking inconsistency that I guess is probably correct but worth checking...

The structure here is:

dcc.Loading(
[
    html.H3(), html.Div(id=self.id, className="table-container")
])

while for Table it's this:

dcc.Loading(
    html.Div(
        [
            html.H3(), html.Div(id=self.id),
         ],
    className="table-container",
))

So there seems to be an extra Div in the Table case the the className="table-container" doesn't apply to the table itself.

Is that correct how it is?

[huong-li-nguyen]

You can definitely try removing it from the Table as well.

However, I remember there were some overflowing issues when the Table was used inside the Container inside the Tab, but I am not sure if it's related to this outer container. So if you remove it and it still looks fine and passes all of Alexeys screenshot tests, I think it's all good 👍

[maxschulz-COL]

Let's do this in another PR - I think this may drag things out, and is off-topic for this one. But will uncomment on merge

[antonymilne]

Looks a lot cleaner, but do you mind explaining why this is a better approach to what we've done before? I don't think I have enough context to evaluate these code changes 😄

FYI @petar-qb @maxschulz-COL @huong-li-nguyen @nadijagraca just as a general point, because I've made similar suggestions in a few places before, when it comes to using CapturedCallable we should always prefer to use the highest-level interface possible to avoid delving into private/protected things. There's basically three categories of attributes here:

• dunder attributes like __call__ and __getitem__: these are the main point of entry to any callers and should be used wherever possible
• protected attributes like _function and _arguments: ok to use if needed but will be removed or made into proper public things in due course, so put some thought into exactly what you're trying to do and whether you really need to use them or if you can already achieve it just with dunder attributes
• private attributes like __arguments: you should never need to use these

[huong-li-nguyen]

Looks a lot cleaner, but do you mind explaining why this is a better approach to what we've done before? I don't think I have enough context to evaluate these code changes 😄

FYI @petar-qb @maxschulz-COL @huong-li-nguyen @nadijagraca just as a general point, because I've made similar suggestions in a few places before, when it comes to using CapturedCallable we should always prefer to use the highest-level interface possible to avoid delving into private/protected things. There's basically three categories of attributes here:

• dunder attributes like __call__ and __getitem__: these are the main point of entry to any callers and should be used wherever possible
• protected attributes like _function and _arguments: ok to use if needed but will be removed or made into proper public things in due course, so put some thought into exactly what you're trying to do and whether you really need to use them or if you can already achieve it just with dunder attributes
• private attributes like __arguments: you should never need to use these

Thank you - that's very useful! Yes, I think I have been missing some discussions as I didn't review these PRs. Would be great to have these conclusions either shared during team learning or even just written in Slack as a heads-up for everyone :) Or just giving a heads up if there are any PRs where everyone should take a look, because it contains general technical decision records would also be helpful!

[antonymilne]

Thank you - that's very useful! Yes, I think I have been missing some discussions as I didn't review these PRs. Would be great to have these conclusions either shared during team learning or even just written in Slack as a heads-up for everyone :) Or just giving a heads up if there are any PRs where everyone should take a look, because it contains general technical decision records would also be helpful!

Agreed! The reason I haven't done this yet is that the current state of CapturedCallable is awaiting a bit of refactoring which will hopefully make this all much clearer by making public the things that we actually want to keep and removing those that we don't and adding better docs so that people won't need to guess any more which are the right things to use.

That's actually what https://github.com/McK-Internal/vizro-internal/issues/450 is for and this is part of the refactoring work I mentioned to you yesterday that I'd like to focus on after I've done the data manager.

[maxschulz-COL]

When I tried to replicate that quirky behavior, I've noticed that what the user inputs actually does not matter for that quirky behavior to be visible. What's leading to the quirky behavior seems to be a deviation from what we insert as arguments to the build and the default settings that are defined here.

This is incorrect, the user input does matter. I revisited that and the correct chain is:

• when the initial object is created with the same id as the final object, it must agree on pagination setting, else the table renders empty
• the defaults matter as much as the user input - it is the final setting that counts
• this is only imporant if original is set to false (default), and final set to true

In that case I would rephrase the comment as follows:

Do not provide any default arguments here, but use the dict inside the _dash_ag_grid.py instead, as deviations from the default arguments here can lead to the table not being displayed correctly.

This is then actually incorrect.

I would go for the slightly adjusted version:

The pagination setting (and potentially others) of the initially built AgGrid (in the build method, with the same ID as the final object) must have the same setting as the object that is built on-page-load and rendered finally. Otherwise the grid is not rendered correctly.

[huong-li-nguyen]

The pagination setting (and potentially others) of the initially built AgGrid (in the build method, with the same ID as the final object) must have the same setting as the object that is built on-page-load and rendered finally. Otherwise the grid is not rendered correctly.

Maybe let's jump on a call and clarify 😄 Because I have a different understanding. You can also leave it unchanged and merge as is, as I think I understood the issue now.