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

Work out what to do with registering pages and model manager #111

Open
antonymilne opened this issue Oct 12, 2023 · 4 comments
Open

Work out what to do with registering pages and model manager #111

antonymilne opened this issue Oct 12, 2023 · 4 comments
Assignees

Comments

@antonymilne
Copy link
Contributor

antonymilne commented Oct 12, 2023

Rough notes for now... A few somewhat related problems:

@antonymilne antonymilne self-assigned this Oct 12, 2023
@maxschulz-COL
Copy link
Contributor

maxschulz-COL commented Oct 13, 2023

Adding #109 for visibility

@petar-qb
Copy link
Contributor

Here's a short recap of the conversation I had with @antonymilne last week on the following topic: "Reorganising models pre_build method invoking system and models callbacks creation so we can get rid of eager page.build inside the vm.Dashboard.build() method":

Here is a list of tests and code changes I made:

  1. I defined vm.Filter(column="year", selector=vm.RangeSlider(title="Select timeframe")) inside the example.
  2. I commented out the page.build() code from vm.Dashboard.build(),
  3. TESTED - and it did not work.
  4. Then I moved the creation of the RangeSlider callback inside its pre_build method.
  5. TESTED - and it did work.
  6. I redefined the filer without propagating a selector manually, so vm.Filter(column="year"),
  7. TESTED - and it did not work.
  8. I've set a logic inside the Filter that calls self.selector.pre_build() if the selector is artificially added.
  9. TESTED - and it did work.

I came up with two suggestions for the general purpose of solving the problem described above:

  1. Should we relocate the internal callback creation within the model's pre_build method?
  2. Do we wish to establish a convention where the object initiating the creation of a new object (during its pre_build phase) is also responsible for invoking the pre_build method of the newly created object?

And here is @antonymilne's opinion on the topic (also summarised):

"""
Relocating the callback creation to pre_build works nicely but it breaks the rule that pre_build should not have any dash dependency. It also feels a bit wrong because it’s tightly coupled to the components generated in the build method. I wonder whether we need some other mechanism for eagerly registering these callbacks like build_callbacks or something. I can imagine there might be other cases in future also so it’s not specific to the sliders?

The second question of where and when pre_build is executed is also related to some ongoing issues about where and when models should be registered to the model manager, which needs more thought too. Since if we change how registration to model manager works then we might have a new function whose responsibility it is to do instantiate + register to model manager and maybe we’d end up adding + pre_build to that also.
"""

@antonymilne
Copy link
Contributor Author

Notes from recent chat with @petar-qb: the important information we need is the relationship between a page and the components on it. Page is the key object in the hierarchy (e.g. tabs are not). We should definitely try to leave the model manager as read-only once the dashboard is running; this should be possible but means that models should probably not be registered to the model manager automatically. If we allow models to be instantiated during page.build (tbd whether we should, but think of Turo prototype) then re-running page.build would alter model manager state.

@petar-qb
Copy link
Contributor

Adding #254 (review) for visibility.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants