Prevent clashing MPComponent IDs

[janosh]

This PR fixes the error

DuplicateIdError: Duplicate component id found in the initial layout: `CTstructure`

by appending a counter to every duplicate ID and incrementing it until the ID is unique. Not sure if this is an elegant solution. The current code explicit warns against this:

crystaltoolkit/crystal_toolkit/core/mpcomponent.py

Lines 194 to 203 in a291123

	# ensure ids are unique
	# Note: shadowing Python built-in here, but only because Dash does it...
	# TODO: do something else here
	if id is None:
	# TODO: this could lead to duplicate ids and an error, but if
	# setting random ids, this could also lead to undefined behavior
	id = f"{CT_NAMESPACE}{type(self).__name__}"
	elif not id.startswith(CT_NAMESPACE):
	id = f"{CT_NAMESPACE}{id}"
	MPComponent._all_id_basenames.add(id)

But based on the frequency with which I encounter this error in my personal use (and apparently even MP production), maybe worth a try? Perhaps @mkhorton who wrote that comment 2 years ago remembers what other unexpected behavior changing the IDs might have?

Minimal code to trigger the error

from dash import Dash, html
from pymatgen.core import Lattice, Structure

import crystal_toolkit.components as ctc
from crystal_toolkit.settings import SETTINGS

structure = Structure(
    lattice=Lattice.cubic(5),
    species=("Fe", "O"),
    coords=((0, 0, 0), (0.5, 0.5, 0.5)),
)
app = Dash()

struct_comp = ctc.StructureMoleculeComponent(id="structure", struct_or_mol=structure)
struct_comp = ctc.StructureMoleculeComponent(id="structure", struct_or_mol=structure)

app.layout = html.Div([struct_comp.layout()])

ctc.register_crystal_toolkit(app=app, layout=app.layout)


app.run(port=8000)

The double call to StructureMoleculeComponent with identical id seems contrived/non-sensical but is what happens naturally in contexts like a dev server where code changes trigger a hot-reload.

[janosh]

@yang-ruoxi Do you have another example where we could test this fix?

[yang-ruoxi]

thanks @janosh ! You can try running mp_web/apps/materials/materials.py or simply test the test_basic.py

[mkhorton]

Thanks also @janosh.

The underlying issue with the counter, I think, was that a solution that might work in a single-threaded demo context might not working when deploying with gunicorn (if I remember correctly, it was some time ago).

I refer to the catch-all issue #265, but a design improvement would be to try to avoid these str based ids anyway, to start to use dictionary-based ids (since these can be used for pattern-matching callbacks), and perhaps to simply require the user to always specify an id. Before I left MP, I was working on a new MPComponentAIO class which followed the new "all-in-component" recommendations in the Dash docs, and would've made a lot of this cleaner. The only caveat holding this back is that, I think client-side callbacks still do not support pattern matching, unless this has recently changed.

[janosh]

Thanks for the update. All these caveats in different design decisions are really good to know!

The only caveat holding this back is that, I think client-side callbacks still do not support pattern matching, unless this has recently changed.

No change still.

Before I left MP, I was working on a new MPComponentAIO class which followed the new "all-in-component" recommendations in the Dash docs, and would've made a lot of this cleaner.

Could you push that to a branch? Maybe we can pick this up.

[mkhorton]

I think this is good to merge (without the print statement), as an interim fix.

[mkhorton]

Could you push that to a branch? Maybe we can pick this up.

Yes, I'd be happy to; I'd also like to get the TaskDocument component merged (I'm not sure if this has drifted away from the MP implementation, but would be good to use it on the task detail pages), and I also have a branch which updates the example apps with pages support. I need to find a few hours just to tidy up a little bit before I can push however.

[janosh]

I think this is good to merge (without the print statement), as an interim fix.

Oki, merging now.

I'm not sure if this has drifted away from the MP implementation, but would be good to use it on the task detail pages

Not much if at all I think but @yang-ruoxi would know.