Use object specific counter instead of a global counter for unnamed parameter & view names

[lukasmasuch]

What is your suggestion?

Unnamed parameters and sub-charts are automatically named by using a global counter:

Parameter:

altair/altair/vegalite/v5/api.py

Lines 196 to 201 in f345cd9

	_counter: int = 0
	@classmethod
	def _get_name(cls) -> str:
	cls._counter += 1
	return f"param_{cls._counter}"

Subcharts/Views:

altair/altair/vegalite/v5/api.py

Lines 2885 to 2890 in f345cd9

	_counter: int = 0
	@classmethod
	def _get_name(cls) -> str:
	cls._counter += 1
	return f"view_{cls._counter}"

This will lead to different specs for the same charts in a multi-threaded/multi-user environment (e.g., Streamlit). In Streamlit, we rely on the spec to be stable to calculate a deterministic element identity across app reruns. Having this global counter makes the usage of Altair complicated. It would be awesome if the global counter would be replaced with a counter that's specific to the chart object -> which would make the chart spec stable independent of the global state.

Have you considered any alternative solutions?

No response

[jonmmease]

Hi @lukasmasuch, thanks for the report.

Unfortunately it's not quite as straightforward as moving the counters from class-level to instance-level. The purpose of these is to make sure that subcharts and params have unique names across concatenation.

For example:

chart1.to_dict()
chart2.to_dict()
(chart1 | chart2).to_dict()

In the result of (chart1 | chart2).to_dict(), the params associated with chart1 must be named differently than those associated with chart2.

To avoid a global counter, we might need to refactor to delay assigning these names until the conversion to a dictionary.

[lukasmasuch]

@jonmmease Thanks for the quick response :)

To avoid a global counter, we might need to refactor to delay assigning these names until the conversion to a dictionary.

This sounds like a reasonable solution.

Our temporary solution in Streamlit will probably be some hacky replacements in the vega-lite spec, but it would be much better if it were eventually solved with a counter that is applied only in the to_dict() logic.

[mattijn]

It's the same issue with data referencing, that one was stabilized by using the hash value of the dataset. We might be able to do this by introducing a hash method on the chart specification, and use that somehow when referencing views and params. Thanks for raising!

[binste]

I agree, it would be cleaner and useful for web apps if there is no global counter. We have the same issue in dash-vega-components (although less severe as there is no "rerun" of the whole app) and maybe there are other use cases which would benefit from this too.

I'm worried that delaying the parameter naming breaks user code. In the documentation, and many examples on other sites, use the pattern of accessing parameter names in expressions via the .name attribute. Search for .name in https://altair-viz.github.io/user_guide/interactions.html, e.g.:

bind_range = alt.binding_range(min=100, max=300, name='Slider value:  ')
param_width = alt.param(bind=bind_range)

# Examples of how to write both js and python expressions
param_color_js_expr = alt.param(expr=f"{param_width.name} < 200 ? 'red' : 'black'")

So I think names already need to be available when a parameter is instantiated. We could port the fix of @lukasmasuch in streamlit/streamlit#8628. Doing this on the JSON string feels hacky but I also can't come up with something better, see https://github.com/streamlit/streamlit/pull/8628/files#diff-611496cd0d1726cf41aed099fd6723e35d40833db11afbafef9be914fc425f26R270 for reasoning on why working on the dict does not work. Same goes for chart objects. Basically, parameters can just be referenced in too many places and even in strings as shown above and hence the replacement needs to happen on strings anyway.

It could be a new argument to to_dict which by default should be False as it slows down the conversion and would be a breaking change. chart.to_dict(reset_unnamed_parameter_names=True)` (need a better name for it) would do the following:

• chart.to_json()
• The fix from @lukasmasuch
• return json.loads(fixed_spec) <- this can be skipped if to_json was called which is what Streamlit would need.

For Streamlit, not much would change compared to the current fix apart from that it's something officially supported and maintained in Altair, but the upside would be that it's also available for other libraries.

Thoughts?

[jonmmease]

Thanks @binste, I like the solution of adding an optional argument to to_dict/to_json. Maybe something like stable_names=True?

[mattijn]

Streamlit has to introduce custom code that utilise internal altair functions. This might be one of reasons that #3554 is popping up as an issue. I say, might, because this specific issue might not be related.
But if we want to restructure the altair internal code, it is a good idea to first address this issue so we produce vega-lite output of altair specifications which are stable when they include parameters.

[dangotbanned]

Streamlit has to introduce custom code that utilise internal altair functions. This might be one of reasons that #3554 is popping up as an issue. I say, might, because this specific issue might not be related.

We found a different source for that particular bug, which was fixed in #3637

(@mattijn you can ignore this, just making sure it is recorded here)