Signature redefinition of branca.Element.render method

[hansthen]

Currently we have a mypy warning that we redefine the render method of branca.element in several places in Folium. I have been trying to understand why this is for a few days now.

The signature redefinition seems to start already in Branca. See branca.element.Figure for instance. The children are void rendered (without actually returning anything). Then the _template of Figure is rendered. This will render just the header, html and script attributes and return the result.

    def render(self, **kwargs):
        """Renders the HTML representation of the element."""
        for name, child in self._children.items():
            child.render(**kwargs)
        return self._template.render(this=self, kwargs=kwargs)

In branca.element.MacroElement, the children are also void rendered, but here the render method itself also has a void signature.

What it looks like is that as soon as we reach Figure and MacroElement the render method changes signature. All children of Figure need to be MacroElements. The render method of a MacroElement has a different purpose than that of an Element. Element.render will just generate the template of that element. MacroElement.render performs structural manipulation. It adds template parts to the header, html and script attributes.

[hansthen]

I think we could clean this up by renaming the render method of MarcoElement to prepare or something like that. It would make the structure of Branca somewhat easier to comprehend and also make mypy happy.

@Conengmo what do you think?

[Conengmo]

That does make sense, but I dislike that we have to rename it everywhere in Folium as well. Maybe I'd rather rename it on the Element class, since:

• I would guess the fallout on our neighbours would be smaller for Branca than for Folium
• it's much less work: only a couple of classes in branca/elements.py.

[hansthen]

It is certainly less work. I would be hesitant for one main reason: render as a method name on Element makes intuitive sense (at least to me) and it has a nice analogy to jinja.Template.render , whereas render on MacroElement does not. This is not a blocking concern for me, but it would make me a bit sad.

Is there a natural reason to call render on MacroElements from client code? I know streamlit-folium does it for some deep magic, but I personally only use map.get_root().render() or _repr_html.

[Conengmo]

whereas render on MacroElement does not.

'render' on MacroElement does also render Jinja2 templates... It's that we render them twice: first the values get inserted in MacroElement.render, then the tree gets combined in Element.render. They are both rendering templates, so I wouldn't know if Element or MacroElement deserves the word 'render' more than the other.

Is there a natural reason to call render on MacroElements from client code?

No, I think not. But backwards compatibility in this aspect is not as much about client code, and more about external plugins and extentions. An external plugin may define a MacroElement inheritence that has a custom render method.

We could do backwards compatibility by still having a typeless render method on MacroElement that redirects to the new method name.

class MacroElement(Element):

     def new_render_method():
        do_the_rendering()

    def render():   # type: ignore
        raise DeprecationWarning()
        self.new_render_method()

The same could also work for Element if we decide to rename it there.

I'm also still thinking about changing the class structure as a solution, what do you think about that? Thread here: #2061 (comment).

[hansthen]

I commented on changing the class structure in #2061 (comment). I also like your proposal to start with a typeless render method that forwards to the new name.

[Conengmo]

Before diving into the technical side of this, I wanted to mention that we have to consider the downstream ecosystem. The most prominent example is streamlit-folium, that has hard-coded assumptions about the object tree structure: https://github.com/randyzwitch/streamlit-folium/blob/master/streamlit_folium/__init__.py#L431. Or FEMlium: https://github.com/FEMlium/FEMlium/blob/eca24f8b1123d2f60d34c5f30ef621fb508b3ddd/femlium/utils/geojson_with_arrows.py#L64. Or a plugin like folium-glify-layer: https://github.com/onaci/folium-glify-layer/blob/main/folium_glify_layer/__init__.py.

Any changes we consider must take into account their impact on neighboring projects. We should then evaluate whether the potential improvements justify the challenges they might introduce for others.

[hansthen]

Yes that is a valid concern. I am somewhat familiar with streamlit-folium (I wrote the code to load the JS and CSS links dynamically for that project). Not so familiar with the other two projects. Did you ever contribute to them?

[Conengmo]

I am somewhat familiar with streamlit-folium

that's useful here!

Not so familiar with the other two projects. Did you ever contribute to them?

No I haven't. Just know of them.

[Conengmo]

How does rendering work?

In this example we have:

Figure
└── Map
    └── TileLayer
        └── ElementAddToElement (added on `TileLayer.render()`)
    ├── Marker
    ├── Marker

When I say 'figure attributes' I mean the Figure.header, Figure.html and Figure.script elements. The collect strings that will be concatenated later.

Full step description

• We start with map.save(), which executes Element.save()
• map has a figure object as its parent. In Element.save() it calls render on that object: html = root.render(**kwargs).
• Figure.render() first calls render on its children, then renders its own template. It's children in our example are just the map object.
• Map.render adds some elements to the figure attributes, then calls super().render() which points ultimately to MacroElement.render().
• MacroElement.render():
  • first actually does some Jinja2 rendering: the header, html and script macros are evaluated to strings and are added to the figure attributes.
  • then it loops over the children of the object and calls render() on those. In this case we are still in the map object, so the children are the two markers.
• In TileLayer there's no render method, but there is in its superclass Layer. We create an ElementAddToElement object and add it as a child to the tilelayer object. Then we go to super().render(), which is on MacroElement.
• Now in MacroElement.render() again, same as before: evaluate macros and add to figure attributes, loop over children.
• render() is called on the ElementAddToElement object, which is MacroElement.render(). Same as before. No children this time.
• We are now back at where we loop over the children of the map object. Next is a marker. This happens in MacroElement.render(), same as before.
• And same for the second marker.
• We finished calling render on the children of the map object. We also finished a similar loop one level higher on the children of the figure object. Back to Figure.render(), where we now call self._template.render(this=self, kwargs=kwargs).
• Figure._template contains things like this: {{this.header.render(**kwargs)}}. This means we evaluate a template within a template.
• Figure.header is an Element, so we are now in Element.render(), which just evaluates its template: self._template.render(this=self, kwargs=kwargs). Figure.header contains all the strings of the children of figure that we evaluated before as part of MacroElement.
• Figure._template contains a for-loop, which loops over its children (the macros that were evaluated before) and renders those.
• So we are now in our third nested template evaluation. The macros that were evaluated before are themselves Element objects with as template just the evaluated string. So we go from figure._template -> figure.header._template -> rendered macro element._template.
• After these, we get things like JavascriptLink objects that were added to the figure attributes during rendering. These now also get their templates evaluated (in Element.render()) as part of for example figure.header.render().
• After we go through all these, we are basically finished!

Summary

• Map.save() -> Figure.render() -> loop over children recursively, depth-first.
• First step: evaluate macros. Folium objects inherit from MacroElement. Evaluate the macros in their templates to plain strings and add them as new Element objects to Figure.header, Figure.html and Figure.script, which are themselves Element objects aimed at collecting them.
• Second step: evaluate Figure.header, Figure.html and Figure.script. These contain the strings-as-Element objects from the evaluated MacroObjects, as well as non-MacroElement children like JavascriptLink.
  • For the strings-as-Element objects the evaluation is three levels deep: Element._template (containing the macro result string) -> Figure.header._template -> Figure._template.

[Conengmo]

Can we somehow rename either MacroElement.render() or Element.render() and the elements that inherit from them? Probably! Classes that inherit from Element like Javascript in practice are never added as a child of a MacroElement-like class. That's because their render functions operate differently.

But renaming things threatens backwards compatibility. As a first step we could make sure at least the methods don't conflict with each other: We could do:

class TreeElement:
    # contains stuff like parents, children, add_to


class Element(TreeElement):
    
    def render() -> str:
        return self._template.render()


class MacroElement(TreeElement):

    def render() -> None:
         figure = self.get_root()
         # evaluate macros and add to figure.header, etc.

That way we are backwards compatible since we are not renaming any methods. But it's clear that there are two different types of render methods.

[hansthen]

At first glance this still seems confusing to me. For the final situation I would prefer to have separate method names. It does have the advantages of not redefining a method signature and not having to change surrounding code. It could be a good first step.

If we do go this route I would suggest to make the root class' name Element and to think of a new name for Element.

[Conengmo]

I would suggest to make the root class' name Element and to think of a new name for Element.

Downside of that is that we have to rename everywhere Element is used, plus it's a backwards incompatible change. If we split off a base class, nobody has to change anything.

[hansthen]

I originally started this discussion because I think the intelligibility of the Folium code should be improved. However, I also note that we sometimes propose sub-optimal solutions (at least from intelligibility perspective) because changes are backwards incompatible. This makes me wonder if the soup is worth the cabbage.

Perhaps we can make our lives a lot simpler my accumulating these and other cleanup changes into a backwards incompatible release Folium v1.0.0?

[Conengmo]

I don't think the issue lies in a version number, or in bundling backwards incompatible changes. It's about what do we ask the Folium-using world if we make backwards incompatible changes. However we bundle or version them, they will have to deal with them.

This makes me wonder if the soup is worth the cabbage.

I would think this about backwards incompatible changes. For us it may seem like a restructuring will have great benefits, but I would be very critical how much of that results in actual user benefits, and how much of it is our software developer curiosity and drive to build.

we sometimes propose sub-optimal solutions (at least from intelligibility perspective) because changes are backwards incompatible

That makes sense. In those cases maybe we are often better of not making a change.

Wanted to bring this as counterweight. I do see your point, and if we decide a backwards incompatible change is a good idea, we should think how to communicate it.

[hansthen]

I don't think the issue lies in a version number, or in bundling backwards incompatible changes. It's about what do we ask the Folium-using world if we make backwards incompatible changes. However we bundle or version them, they will have to deal with them.

I rather meant that we continue developing packages from both releases, somewhat similar to SQLAlchemy 1.4 and 2.0 (and python 2 and 3). We could make backwards incompatible changes in the 1.* packages and keep everything backwards compatible for 0.*. That way our surrounding ecosystem has a chance to adapt to the changes in their own time.

I would think this about backwards incompatible changes. For us it may seem like a restructuring will have great benefits, but I would be very critical how much of that results in actual user benefits, and how much of it is our software developer curiosity and drive to build.

Valid point. However, I also have heard you yourself saying that Folium and Branca are getting difficult to understand. I have the same, sometimes it feels slightly too big for my brain. Simplification (even if it breaks backwards compatibility) can be a valid goal. Even if it does not immediately improves user experience.

Wanted to bring this as counterweight. I do see your point, and if we decide a backwards incompatible change is a good idea, we should think how to communicate it.
Debate makes all ideas better.

[Conengmo]

Another possible simplification is to consolidate some of the logic in the render methods and in the templates. Ideally calls to render and for-loops happen only on the Python side, not in the templates themselves.

• We could try if we can loop over children and render them as part of Element.render, instead of having that in the template.
• We could see if we can move logic out of the templates.

[Conengmo]

All children of Figure need to be MacroElements.

That's true and not explicitly in the code! It should be!

[hansthen]

It is also not consistent in the code. The JSCSSMixin for instance inherits from Element but then impersonates a MacroElement.

[Conengmo]

Good point. We could make issues/PRs for these, seem like straightforward improvements.

[Conengmo]

For example in Figure.add_child, it would be nice to have it typed, so type checkers also know you can only add MacroElements. But we can't do that without breaking up the class structure. So maybe for now just doing checking on runtime is sufficient.

[hansthen]

Or we could rename add_child on Figure to something like add_macro.

I don't remember where exactly, but somewhere else in this discussion you mentioned moving Figure and MacroElement from Branca to Folium. I also like that idea. I think it will increase the cohesion of both packages.