refactor: Remove toolz dependency

[dangotbanned]

This PR is a proof-of-concept, entirely* removing the hard dependency on toolz.
*Excluding usage in tests and twice for a deprecation warning.

I've tried to provide reasoning on each commit, but generally, it seemed to me that the behaviour altair needs is easily replicated within stdlib.

Additional Notes

• pipe and curry were the only imports
  • pipe is trivial to replace.
  • curry seems overkill when the only non-default argument is always data
    • Also has the drawback of less type safety
• While this is documented under Advanced Usage, I haven't come across any more complex examples.
  • I'd imagine users could see a benefit to performance, given the complexity of curry.
  • If anyone has some sample code that heavily uses data transformers, it could be helpful to benchmark any changes.

If there was any desire to pursue this further, in b9dc070 I mentioned that this could be implemented as a decorator.
I stopped short of that for now, but would be happy to go down that route to simplify the @overload usage.

Edit

@mattijn is this worth opening an issue for discussion, or can that take place here?

AFAIK the last remaining quirks would be:

• Handling the existing deprecation functions in bf99d72.
  • I would assume 4 years is enough for a scheduled removal.
  • Otherwise these could use proper optional imports (rather than the nesting solution I've tentatively used).
• Updating pyproject.toml.
  • Not entirely sure on the interaction with existing dependency groups.
  • Just want to highlight it as an extra item to-do.

[mattijn]

Hi @dangotbanned, this looks good! Thanks for working on this. Reducing the hard dependencies where possible is much welcome.
If types and tests are happy than we are very far in the progress already.
Could you explain why this PR requires the introduction of a new ._to_text() function, see here: https://github.com/vega/altair/pull/3426/files#diff-478a9046db15a358587cf77454cd2cf9cc4723435276cad0864c268d17e07650R295. It seems to be introduced during the development process?

[dangotbanned]

Hi @dangotbanned, this looks good! Thanks for working on this. Reducing the hard dependencies where possible is much welcome.

Thanks @mattijn!

Could you explain why this PR requires the introduction of a new ._to_text() function, see here: #3426 (files). It seems to be introduced during the development process?

Sure thing.

So, previously to_csv, to_json had almost identical code and required 2x TypedDict's per function:

• to_csv
  • _CsvFormatDict
  • _ToCsvReturnUrlDict
• to_json
  • _JsonFormatDict
  • _ToJsonReturnUrlDict

I split out the generic behaviour to _to_text and simplified the types (4 -> 2) to reduce code duplication.

This helped with managing keyword-only args for line 251/line 287 which would have previously been curried.

The addition of _to_text isn't required, but as a comparison, below is the additional code without it (not including the overloads):

def to_json(
    data: Optional[DataType] = None,
    prefix: str = "altair-data",
    extension: str = "json",
    filename: str = "{prefix}-{hash}.{extension}",
    urlpath: str = "",
) -> Union[partial, _ToFormatReturnUrlDict]:
    """
    Write the data model to a .json file and return a url based data model.
    """
    if data is None:
        return partial(
            to_json, # diff 1
            prefix=prefix,
            extension=extension,
            filename=filename,
            urlpath=urlpath,
        )
    else:
        data_str = _data_to_json_string(data) # diff 2
        data_hash = _compute_data_hash(data_str)
        filename = filename.format(prefix=prefix, hash=data_hash, extension=extension)
        Path(filename).write_text(data_str)
        url = str(Path(urlpath, filename))
        return _ToFormatReturnUrlDict({"url": url, "format": _FormatDict(type="json")}) # diff 3

def to_csv(
    data: Optional[NonGeoDataType] = None,
    prefix: str = "altair-data",
    extension: str = "csv",
    filename: str = "{prefix}-{hash}.{extension}",
    urlpath: str = "",
) -> Union[partial, _ToFormatReturnUrlDict]:
    """Write the data model to a .csv file and return a url based data model."""
    if data is None:
        return partial(
            to_csv,
            prefix=prefix,
            extension=extension,
            filename=filename,
            urlpath=urlpath,
        )
    else:
        data_str = _data_to_csv_string(data)
        data_hash = _compute_data_hash(data_str)
        filename = filename.format(prefix=prefix, hash=data_hash, extension=extension)
        Path(filename).write_text(data_str)
        url = str(Path(urlpath, filename))
        return _ToFormatReturnUrlDict({"url": url, "format": _FormatDict(type="csv")})

I am happy to revert back to this, but thought it might be more maintainable in the future - while still preserving the public interface.

[mattijn]

Thanks for the explanation! That makes sense, leave it in please.

I'm in favor of removing the functions that have been officially deprecated for such a long time.

[dangotbanned]

@mattijn No rush on the review, just wanted to update that all traces of toolz are now gone 😅

[mattijn]

Hi @dangotbanned. Since you have multiple PRs open, do you have a certain order in mind how we should review and merge these?

[dangotbanned]

Hi @dangotbanned. Since you have multiple PRs open, do you have a certain order in mind how we should review and merge these?

#3426

Hi @mattijn this one is ready to go.

I'm in favor of removing the functions that have been officially deprecated for such a long time.

I'm not sure if you wanted me to remove pipe, curry, the new optional import and tests - or if the removal needs any more input from other maintainers.

Personally, I agree and think they should be removed.

Other PRs

#3431

• I'm pretty happy with where it's at now, but the changes I've made in the tools package relating to Optional, Parameter, SchemaBase likely need further review.
• They were somewhat "hacky" so as to not alter the surrounding code, and someone more familiar with the code generation may be able to improve upon it.
• This one (I think) has addressed all the points from @binste original review

#3427

• I've left this on draft, not ready yet, but would be good to hear your thoughts on a couple of things

1. alt.when signature
   • To preserve compatibility with alt.condition, the first predicate arg behaves identically - but *more_predicates is more restrictive in the types allowed - as they all must implement &.
   • I've left this to bake before writing docs, as I'm not sure whether the trade-off in added complexity (for the user) is worth preserving identical semantics as alt.condition for a single argument.
2. The new parts added for alt.expr comment which are "an" implementation, but not sure it would be the right way to go.

[dangotbanned]

@mattijn

I would be I favor of removing pipe and curry as well and instead of a deprecation warning use a deprecation error.

@joelostblom

I am also in favor of removing pipe and curry if we can replicate the functionality with stlib.

Thank you both for reviewing. All of your comments should be resolved now.

I have added comments suggesting the parts to remove. This excludes the imports, which would be highlighted by any IDE anyway afterwards.

Should be ready to merge after 650ee0e finishes

[mattijn]

I think we need something else if we want a NonGeoDataType type, since the following supports the SupportsGeoInterface and also is postive as a NonGeoDataType being:

from altair.utils.data import NonGeoDataType

from shapely import geometry
import geopandas as gpd

data_geoms = [
    {"color": "#F3C14F", "geometry": geometry.Polygon([[1.45, 3.75], [1.45, 0], [0, 0], [1.45, 3.75]])},
    {"color": "#4098D7", "geometry": geometry.Polygon([[1.45, 0], [1.45, 3.75], [2.57, 3.75], [2.57, 0], [2.33, 0], [1.45, 0]])},
    {"color": "#66B4E2", "geometry": geometry.Polygon([[2.33, 0], [2.33, 2.5], [3.47, 2.5], [3.47, 0], [3.2, 0], [2.57, 0], [2.33, 0]])},
    {"color": "#A9CDE0", "geometry": geometry.Polygon([[3.2, 0], [3.2, 1.25], [4.32, 1.25], [4.32, 0], [3.47, 0], [3.2, 0]])},
]

gdf_geoms = gpd.GeoDataFrame(data_geoms)
isinstance(gdf_geoms, SupportsGeoInterface), isinstance(gdf_geoms, NonGeoDataType)

(True, True)

Example data taken from docs here: https://altair-viz.github.io/user_guide/data.html#spatial-data-gdf

[dangotbanned]

I think we need something else if we want a NonGeoDataType type, since the following supports the SupportsGeoInterface and also is postive as a NonGeoDataType being:

@mattijn I think this would be unavoidable given that GeoDataFrame is a subclass of pd.DataFrame and python's typing doesn't support discriminated unions.

So it would pass NonGeoDataType = Union[dict, pd.DataFrame, DataFrameLike]

• DataFrameLike as it has .__dataframe__
• pd.DataFrame as it is a subclass

I'm only finding this out now btw, in 8a9d593 I just attached a name to an existing type annotation.

For the actual usage in to_csv, SupportsGeoInterface is rejected in https://github.com/dangotbanned/altair/blob/ed13af6d4598c96d1f5e5a816b95a85ff82f81f3/altair/utils/data.py#L376.

So the impact of this would be static type checking doesn't pick up the incompatibility - but at runtime the existing code does.

[mattijn]

Thanks for responding! Let me think about it too. Just wondering, do we really need this NonGeoDataType? Maybe this constrain within to_csv is too conservative?

[dangotbanned]

Just wondering, do we really need this NonGeoDataType?

I've no strong feelings either way @mattijn.

The only thing I'd add to #3426 (comment) is that having an alias here at least hints the user towards something that we can't express as a type - if that makes sense?

I wouldn't expect passing a GeoDataFrame would work as a parameter labelled NonGeo....

Looking back to the original signature, there is no indication that to_csv doesn't accept SupportsGeoInterface:

altair/altair/utils/data.py

Lines 198 to 205 in 596b282

	def to_csv(
	data: Union[dict, pd.DataFrame, DataFrameLike],
	prefix: str = "altair-data",
	extension: str = "csv",
	filename: str = "{prefix}-{hash}.{extension}",
	urlpath: str = "",
	) -> _ToCsvReturnUrlDict:
	"""Write the data model to a .csv file and return a url based data model."""

[mattijn]

I've opened #3441. Maybe we can rephrase the NotImplementedError with a link tot that github issue. Then we don't need the newly introduced NonGeoDataType type in this PR. Thanks for still working on this @dangotbanned, almost there!

[mattijn]

Tests are happy too after synching with main. Merging! Thanks @dangotbanned!