improve robustness of `altair.datasets`

[mattijn]

What is your suggestion?

In #3848 altair.datasets was added.

As was pointed out by @dangotbanned on slack, we should doublecheck that we haven't introduced regressions in the tests.

This commit shows shows the partial diff between 4b348c9 and 628ca56 of only the changes in the altair/datasets/* directory and the tests/test_datasets.py on a separate branch.

Hope this issue helps in checking these.

Have you considered any alternative solutions?

No response

[dangotbanned]

Thanks for opening this @mattijn 🙂

I don't think an issue will work as well as a PR would have.
Since it's just going to be a wall of text - rather than a PR with multiple threads - but I'll do my best to organize things

Show slack message

Hi @mattijn, I'm happy to see my efforts in (#3631) have made their way into altair 🥳

However, could you please revert (#3848)?

Despite our disagreements on the API, I did voice that you were free to take the lead and make the changes you wanted.
I should have been clearer that - even if I were taking a hands off approach - I still expected someone to review the PR.
AFAICT, my original PR was never reviewed and the one you left open for an hour most definitely wasn't either.

I can understand not wanting my opinion on your changes to my changes - but that can't explain why no attempt was made to reach out to another maintainer.
... (comments about tests)
There are other issues I've noticed, but I would much prefer to go over everything in a review

Testing 1

Warning

You merged this PR without adding a single test for this new data API
Existing tests were removed or altered

Existing test failures

To expand on what I mentioned on slack, the full suite of tests were not run by default - since that would require downloading and reading into memory at least 70 datasets x 4 backends.

There are a few mechanisms in place to keep things quick in CI, but still allow the option to go more in depth.
I was only using it locally, but another idea I had was to move it to a separate GH workflow.

Where I ended up in (#3631) was informed directly by all of the edge cases that popped up from my earlier naive 😅 self expecting more consistency between backends.

@datasets_debug

Any test marked with @datasets_debug will not run unless you ask for it, as documented here:

altair/tests/test_datasets.py

Lines 39 to 49 in b3966c3

	datasets_debug: pytest.MarkDecorator = pytest.mark.datasets_debug()
	"""
	Custom ``pytest.mark`` decorator.
	Use for more exhaustive tests that require many requests.
	**Disabled** by default in ``pyproject.toml``:
	[tool.pytest.ini_options]
	addopts = ...
	"""

And here

altair/pyproject.toml

Line 314 in b3966c3

"-m not datasets_debug and not no_xdist",

And here also

altair/pyproject.toml

Line 319 in b3966c3

"datasets_debug: Disabled by default due to high number of requests",

I decided to add a shortcut for this, so you can run all the new tests with:

uv run task test-datasets

altair/pyproject.toml

Line 280 in b3966c3

test-datasets = "task ruff-fix && pytest tests -k test_datasets -m \"not no_xdist\" && task pytest-serial"

Important

If you run that task, it causes 1 failure and 23 warnings

Show output

PS C:\Users\danie\Documents\GitHub\altair> uv run task test-datasets
All checks passed!
116 files left unchanged
========================================================= test session starts =========================================================
platform win32 -- Python 3.12.8, pytest-8.4.1, pluggy-1.6.0
rootdir: C:\Users\danie\Documents\GitHub\altair
configfile: pyproject.toml
plugins: anyio-4.9.0, cov-6.2.1, xdist-3.8.0
ready: 18/24 workers      

24 workers [176 items]   
................................................................................................................................ [ 72%]
.....................................F..........                                                                                 [100%]
============================================================== FAILURES ===============================================================
____________________________________________________ test_all_datasets[icon_7zip] _____________________________________________________
[gw23] win32 -- Python 3.12.8 C:\Users\danie\Documents\GitHub\altair\.venv\Scripts\python.exe

polars_loader = Loader[polars], name = 'icon_7zip'

    @datasets_all
    @datasets_debug
    def test_all_datasets(polars_loader: PolarsLoader, name: Dataset) -> None:
        if name in {"icon_7zip", "ffox", "gimp"}:
            pattern = re.compile(
                rf"Unable to load.+{name}.png.+as tabular data",
                flags=re.DOTALL | re.IGNORECASE,
            )
            with pytest.raises(AltairDatasetsError, match=pattern):
>               polars_loader(name)

tests\test_datasets.py:508: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
altair\datasets\_loader.py:225: in __call__
    return self._reader.dataset(name, suffix, **kwds)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
altair\datasets\_reader.py:193: in dataset
    fn = self.read_fn(meta)
         ^^^^^^^^^^^^^^^^^^
altair\datasets\_reader.py:174: in read_fn
    return self._solve(meta, self._read)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = Reader[polars] <Implementation.POLARS: 'polars'>
read
    read_csv(..., try_parse_dates=True), is_meta('*.csv')
    re...c(...), is_meta('*.arrow')
    read_parquet(...), is_meta('*.parquet')
scan
    scan_parquet(...), is_meta('*.parquet')
meta = {'bytes': 3969, 'dataset_name': 'icon_7zip', 'file_name': '7zip.png', 'has_schema': False, ...}
impls = (BaseImpl[polars?](read_csv(..., try_parse_dates=True), is_meta('*.csv')), BaseImpl[polars_st?](read_file(...), is_met...., separator='\t', try_parse_dates=True), is_meta('*.tsv')), BaseImpl[polars?](read_ipc(...), is_meta('*.arrow')), ...)

    def _solve(
        self, meta: Metadata, impls: Sequence[BaseImpl[R]], /
    ) -> Callable[..., R]:
        """
        Return the first function that satisfies dataset constraints.

        See Also
        --------
        ``altair.datasets._readimpl.BaseImpl.unwrap_or_skip``
        """
        items = meta.items()
        it = (some for impl in impls if (some := impl.unwrap_or_skip(items)))
        if fn_or_err := next(it, None):
            if _is_err(fn_or_err):
                raise fn_or_err.from_tabular(meta, self._name)
            return fn_or_err
>       raise AltairDatasetsError.from_tabular(meta, self._name)
E       altair.datasets._exceptions.AltairDatasetsError: Unable to load '7zip.png' as tabular data.
E       Image data is non-tabular.
E       
E       Instead, try:
E           from altair.datasets import data
E           data.icon_7zip.url

altair\datasets\_reader.py:336: AltairDatasetsError

During handling of the above exception, another exception occurred:

polars_loader = Loader[polars], name = 'icon_7zip'

    @datasets_all
    @datasets_debug
    def test_all_datasets(polars_loader: PolarsLoader, name: Dataset) -> None:
        if name in {"icon_7zip", "ffox", "gimp"}:
            pattern = re.compile(
                rf"Unable to load.+{name}.png.+as tabular data",
                flags=re.DOTALL | re.IGNORECASE,
            )
>           with pytest.raises(AltairDatasetsError, match=pattern):
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
E           AssertionError: Regex pattern did not match.
E            Regex: re.compile('Unable to load.+icon_7zip.png.+as tabular data', re.IGNORECASE|re.DOTALL)
E            Input: "Unable to load '7zip.png' as tabular data.\nImage data is non-tabular.\n\nInstead, try:\n    from altair.datasets import data\n    data.icon_7zip.url"

tests\test_datasets.py:507: AssertionError
========================================================== warnings summary =========================================================== 
tests/test_datasets.py: 21 warnings
  C:\Users\danie\Documents\GitHub\altair\.venv\Lib\site-packages\pyogrio\__init__.py:19: DeprecationWarning: The 'shapely.geos' module is deprecated, and will be removed in a future version. All attributes of 'shapely.geos' are available directly from the top-level 'shapely' namespace (since shapely 2.0.0).
    import shapely.geos  # noqa: F401

tests/test_datasets.py::test_all_datasets[world_110m]
  C:\Users\danie\Documents\GitHub\altair\.venv\Lib\site-packages\pyogrio\raw.py:289: UserWarning: More than one layer found in 'a1ce852de6f2713c94c0c284039506ca2d4f3dee.json': 'countries' (default), 'TopoJSON'. Specify layer parameter to avoid this warning.
    with open_arrow(

tests/test_datasets.py::test_all_datasets[us_10m]
  C:\Users\danie\Documents\GitHub\altair\.venv\Lib\site-packages\pyogrio\raw.py:289: UserWarning: More than one layer found in 'ff7a7e679c46f2d1eb85cc92521b990f1a7a5c7a.json': 'counties' (default), 'states', 'TopoJSON'. Specify layer parameter to avoid this warning.      
    with open_arrow(

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
======================================================= short test summary info ======================================================= 
FAILED tests/test_datasets.py::test_all_datasets[icon_7zip] - AssertionError: Regex pattern did not match.
============================================= 1 failed, 175 passed, 23 warnings in 14.63s ============================================= 

test_reader_cache_exhaustive

With the above context in mind, the change to this test doesn't make much sense to me.

Show before

altair/tests/test_datasets.py

Lines 438 to 488 in 28fb332

	@datasets_debug
	@backends
	def test_reader_cache_exhaustive(
	backend: _Backend,
	monkeypatch: pytest.MonkeyPatch,
	tmp_path: Path,
	polars_loader: PolarsLoader,
	) -> None:
	"""
	Fully populate and then purge the cache for all backends.
	Notes
	-----
	- Does not attempt to read the files
	- Checking we can support pre-downloading and safely deleting
	- Requests work the same for all backends
	- The logic for detecting the cache contents uses ``narhwals``
	- Here, we're testing that these ``narwhals`` ops are consistent
	- `DatasetCache.download_all` is expensive for CI, so aiming for it to run **at most once**
	- 34-45s per call (4x backends)
	"""
	polars_loader.cache.download_all()
	CLONED: Path = tmp_path / "clone"
	fs.mkdir(CLONED)
	fs.copytree(polars_loader.cache.path, CLONED)
	monkeypatch.setenv(CACHE_ENV_VAR, str(tmp_path))
	load = Loader.from_backend(backend)
	assert load.cache.is_active()
	cache_dir = load.cache.path
	assert cache_dir == tmp_path
	assert tuple(load.cache) == (CLONED,)
	load.cache.path = CLONED
	cached_paths = tuple(load.cache)
	assert cached_paths != ()
	# NOTE: Approximating all datasets downloaded
	assert len(cached_paths) >= 70
	assert all(bool(fp.exists() and fp.stat().st_size) for fp in load.cache)
	# NOTE: Confirm this is a no-op
	load.cache.download_all()
	assert len(cached_paths) == len(tuple(load.cache))
	# NOTE: Ensure unrelated files in the directory are not removed
	dummy: Path = tmp_path / "dummy.json"
	dummy.touch(exist_ok=False)
	load.cache.clear()
	remaining = tuple(tmp_path.iterdir())
	assert set(remaining) == {dummy, CLONED}
	fs.rm(dummy, CLONED)

Show after

altair/tests/test_datasets.py

Lines 387 to 420 in b3966c3

	@datasets_debug
	@backends
	def test_reader_cache_exhaustive(
	backend: _Backend,
	monkeypatch: pytest.MonkeyPatch,
	tmp_path: Path,
	) -> None:
	"""
	Test cache operations with a subset of datasets for faster execution.
	This is a simplified version that tests the same functionality
	without downloading all 70+ datasets.
	"""
	monkeypatch.setenv(CACHE_ENV_VAR, str(tmp_path))
	load = Loader.from_backend(backend)
	assert load.cache.is_active()
	# Test with a few small datasets instead of all
	test_datasets = ["cars", "stocks", "movies"]
	for dataset in test_datasets:
	load(dataset)
	cached_paths = tuple(load.cache)
	assert len(cached_paths) >= len(test_datasets)
	assert all(bool(fp.exists() and fp.stat().st_size) for fp in load.cache)
	# Test cache clearing
	dummy: Path = tmp_path / "dummy.json"
	dummy.touch(exist_ok=False)
	load.cache.clear()
	remaining = tuple(tmp_path.iterdir())
	assert set(remaining) == {dummy}
	dummy.unlink() # Remove the dummy file

1. The test has exhaustive in the name, but tests only 3 datasets
2. It removes coverage for an explicitly public part of the API Loader.cache.download_all
3. Since the tests are optional, we don't need to use a subset of datasets - especially those that are used in other tests already

test_load_infer_priority

I dont understand this change, it isn't testing the same thing at all.

If you feel pytest.MonkeyPatch is complex, then please reimplement the same test without it.

I was hesitent to add the load API at first (#3631 (comment)). I prefer to avoid using things that are not understood by a type checker, but I was more comfortable with those tests in place which were the only thing covering the runtime magic

Show before

altair/tests/test_datasets.py

Lines 156 to 200 in 28fb332

	@no_xdist
	def test_load_infer_priority(monkeypatch: pytest.MonkeyPatch) -> None:
	"""
	Ensure the **most reliable**, available backend is selected.
	See Also
	--------
	``altair.datasets._reader.infer_backend``
	"""
	import altair.datasets._loader
	from altair.datasets import load
	assert is_loader_backend(load, "polars")
	monkeypatch.delattr(altair.datasets._loader, "load", raising=False)
	monkeypatch.setitem(sys.modules, "polars", None)
	from altair.datasets import load
	if find_spec("pyarrow") is None:
	# NOTE: We can end the test early for the CI job that removes `pyarrow`
	assert is_loader_backend(load, "pandas")
	monkeypatch.delattr(altair.datasets._loader, "load")
	monkeypatch.setitem(sys.modules, "pandas", None)
	with pytest.raises(AltairDatasetsError, match=r"no.+backend"):
	from altair.datasets import load
	else:
	assert is_loader_backend(load, "pandas[pyarrow]")
	monkeypatch.delattr(altair.datasets._loader, "load")
	monkeypatch.setitem(sys.modules, "pyarrow", None)
	from altair.datasets import load
	assert is_loader_backend(load, "pandas")
	monkeypatch.delattr(altair.datasets._loader, "load")
	monkeypatch.setitem(sys.modules, "pandas", None)
	monkeypatch.delitem(sys.modules, "pyarrow")
	monkeypatch.setitem(sys.modules, "pyarrow", import_module("pyarrow"))
	from altair.datasets import load
	assert is_loader_backend(load, "pyarrow")
	monkeypatch.delattr(altair.datasets._loader, "load")
	monkeypatch.setitem(sys.modules, "pyarrow", None)
	with pytest.raises(AltairDatasetsError, match=r"no.+backend"):
	from altair.datasets import load

Show after

altair/tests/test_datasets.py

Lines 168 to 185 in 944523d

	@no_xdist
	def test_load_infer_priority() -> None:
	"""
	Ensure the **most reliable**, available backend is selected.
	This test verifies that the default backend selection works correctly
	without complex monkeypatch operations.
	"""
	from altair.datasets import load
	# Test that we get a valid loader
	assert hasattr(load, "_reader")
	assert hasattr(load, "cache")
	# Test that we can actually load data
	df = load("cars")
	assert df is not None
	assert len(df) > 0

1. The test has infer_priority in the name, but tests exactly one unknown configuration
2. Both pytest.raises assertions were removed

test_load_call

This was just removed and I guess replaced by overly simplifying both itself and test_load_infer_priority, then merging the two into one test.

Why?

altair/tests/test_datasets.py

Lines 203 to 216 in 28fb332

	@backends
	def test_load_call(backend: _Backend, monkeypatch: pytest.MonkeyPatch) -> None:
	import altair.datasets._loader
	monkeypatch.delattr(altair.datasets._loader, "load", raising=False)
	from altair.datasets import load
	assert is_loader_backend(load, "polars")
	default = load("cars")
	df = load("cars", backend=backend)
	default_2 = load("cars")
	assert nw_dep.is_polars_dataframe(default)
	assert is_frame_backend(df, backend)
	assert nw_dep.is_polars_dataframe(default_2)

[mattijn]

Thanks for adding more context @dangotbanned. I see, I was a bit too aggressive with modifying the tests. Should have been more careful here.

I'm not sure if we should add tests to altair that checks all possible upstream datasets on all possible backends. Maybe something for vega-datasets repository to check if it produces valid datasets if it doesn't do that already?

[dangotbanned]

Typing

I'll take responsibility for this one.

I'd planned to eventually add pyright to our CI, after finishing working through all the existing warnings (not introduced by either PR).
We have that setup in (https://github.com/narwhals-dev/narwhals/blob/726d9775ffd7acd657bbd925f80f478990931c83/.github/workflows/typing.yml) and it is really helpful to make sure we don't have any surprises showing up for users of either

So these 14 new warnings (new total of 44) most likely didn't show up when you were working on it.

These will be simple enough to fix and I'm happy to do that on a follow-up 🙂

uv add --dev pyright
uv run pyright

Show output

/altair/altair/datasets/_data.py:102:40 - error: No overloads for "from_backend" match the provided arguments (reportCallIssue)
  /altair/altair/datasets/_data.py:102:60 - error: Argument of type "str" cannot be assigned to parameter "backend_name" of type "Literal['pyarrow']" in function "from_backend"
    "str" is not assignable to type "Literal['pyarrow']" (reportArgumentType)
  /altair/altair/datasets/_data.py:103:20 - error: Object of type "None" cannot be called (reportOptionalCall)
  /altair/altair/datasets/_data.py:103:41 - error: Argument of type "str" cannot be assigned to parameter "name" of type "Dataset | LiteralString" in function "__call__"       
    Type "str" is not assignable to type "Dataset | LiteralString"
      "str" is not assignable to "LiteralString"
      "str" is not assignable to type "Literal['airports']"
      "str" is not assignable to type "Literal['annual_precip']"
      "str" is not assignable to type "Literal['anscombe']"
      "str" is not assignable to type "Literal['barley']"
      "str" is not assignable to type "Literal['birdstrikes']"
      "str" is not assignable to type "Literal['budget']"
    ... (reportArgumentType)
  /altair/altair/datasets/_data.py:106:27 - error: Argument of type "str" cannot be assigned to parameter "name" of type "Dataset | LiteralString" in function "__call__"
    Type "str" is not assignable to type "Dataset | LiteralString"
      "str" is not assignable to "LiteralString"
      "str" is not assignable to type "Literal['airports']"
      "str" is not assignable to type "Literal['annual_precip']"
      "str" is not assignable to type "Literal['anscombe']"
      "str" is not assignable to type "Literal['barley']"
      "str" is not assignable to type "Literal['birdstrikes']"
      "str" is not assignable to type "Literal['budget']"
    ... (reportArgumentType)
  /altair/altair/datasets/_data.py:106:27 - error: Argument of type "str" cannot be assigned to parameter "name" of type "Dataset | LiteralString" in function "__call__"
    Type "str" is not assignable to type "Dataset | LiteralString"
      "str" is not assignable to "LiteralString"
      "str" is not assignable to type "Literal['airports']"
      "str" is not assignable to type "Literal['annual_precip']"
      "str" is not assignable to type "Literal['anscombe']"
      "str" is not assignable to type "Literal['barley']"
      "str" is not assignable to type "Literal['birdstrikes']"
      "str" is not assignable to type "Literal['budget']" (reportArgumentType)
  /altair/altair/datasets/_data.py:126:36 - error: No overloads for "from_backend" match the provided arguments (reportCallIssue)
  /altair/altair/datasets/_data.py:126:56 - error: Argument of type "str" cannot be assigned to parameter "backend_name" of type "Literal['pyarrow']" in function "from_backend"
    "str" is not assignable to type "Literal['pyarrow']" (reportArgumentType)
  /altair/altair/datasets/_data.py:127:37 - error: "url" is not a known attribute of "None" (reportOptionalMemberAccess)
  /altair/altair/datasets/_data.py:127:41 - error: Argument of type "str" cannot be assigned to parameter "name" of type "Dataset | LiteralString" in function "url"
    Type "str" is not assignable to type "Dataset | LiteralString"
      "str" is not assignable to "LiteralString"
      "str" is not assignable to type "Literal['airports']"
      "str" is not assignable to type "Literal['annual_precip']"
      "str" is not assignable to type "Literal['anscombe']"
      "str" is not assignable to type "Literal['barley']"
      "str" is not assignable to type "Literal['birdstrikes']"
      "str" is not assignable to type "Literal['budget']"
    ... (reportArgumentType)
  /altair/altair/datasets/_data.py:292:55 - error: Argument of type "str" cannot be assigned to parameter "default_backend" of type "_Backend" in function "__init__"
    Type "str" is not assignable to type "_Backend"
      "str" is not assignable to type "_Polars"
      "str" is not assignable to type "_Pandas"
      "str" is not assignable to type "Literal['pandas[pyarrow]']"
      "str" is not assignable to type "_PyArrow" (reportArgumentType)
  /altair/altair/datasets/_data.py:355:16 - error: Type "str" is not assignable to return type "_Backend"
    Type "str" is not assignable to type "_Backend"
      "str" is not assignable to type "_Polars"
      "str" is not assignable to type "_Pandas"
      "str" is not assignable to type "Literal['pandas[pyarrow]']"
      "str" is not assignable to type "_PyArrow" (reportReturnType)

/altair/tests/test_datasets.py
  /altair/tests/test_datasets.py:407:14 - error: Argument of type "str" cannot be assigned to parameter "name" of type "Dataset | LiteralString" in function "__call__"
    Type "str" is not assignable to type "Dataset | LiteralString"
      "str" is not assignable to "LiteralString"
      "str" is not assignable to type "Literal['airports']"
      "str" is not assignable to type "Literal['annual_precip']"
      "str" is not assignable to type "Literal['anscombe']"
      "str" is not assignable to type "Literal['barley']"
      "str" is not assignable to type "Literal['birdstrikes']"
      "str" is not assignable to type "Literal['budget']"
    ... (reportArgumentType)
  /altair/tests/test_datasets.py:407:14 - error: Argument of type "str" cannot be assigned to parameter "name" of type "Dataset | LiteralString" in function "__call__"
    Type "str" is not assignable to type "Dataset | LiteralString"
      "str" is not assignable to "LiteralString"
      "str" is not assignable to type "Literal['airports']"
      "str" is not assignable to type "Literal['annual_precip']"
      "str" is not assignable to type "Literal['anscombe']"
      "str" is not assignable to type "Literal['barley']"
      "str" is not assignable to type "Literal['birdstrikes']"
      "str" is not assignable to type "Literal['budget']" (reportArgumentType)

[dangotbanned]

Testing 2

is_url

Show before

altair/tests/test_datasets.py

Lines 111 to 114 in 28fb332

	def is_url(name: Dataset, fn_url: Callable[..., str], /) -> bool:
	pattern = rf".+/vega-datasets@.+/data/{name}\..+"
	url = fn_url(name)
	return re.match(pattern, url) is not None

altair/tests/test_datasets.py

Lines 256 to 279 in 28fb332

	def test_url_no_backend(monkeypatch: pytest.MonkeyPatch) -> None:
	from altair.datasets._cache import csv_cache
	from altair.datasets._reader import infer_backend
	priority: Any = ("fake_mod_1", "fake_mod_2", "fake_mod_3", "fake_mod_4")
	assert csv_cache._mapping == {}
	with pytest.raises(AltairDatasetsError):
	infer_backend(priority=priority)
	url = csv_cache.url
	assert is_url("jobs", url)
	assert csv_cache._mapping != {}
	assert is_url("cars", url)
	assert is_url("stocks", url)
	assert is_url("countries", url)
	assert is_url("crimea", url)
	assert is_url("disasters", url)
	assert is_url("driving", url)
	assert is_url("earthquakes", url)
	assert is_url("flare", url)
	assert is_url("flights-10k", url)
	assert is_url("flights-200k", url)
	if find_spec("vegafusion"):
	assert is_url("flights-3m", url)

Show after

altair/tests/test_datasets.py

Lines 110 to 126 in f6789e3

	def is_url(name: Dataset, fn_url: Callable[..., str], /) -> bool:
	"""Check if the URL function returns a valid vega-datasets URL."""
	# Get the actual file name from metadata to avoid hardcoding patterns
	import narwhals as nw
	from altair.datasets._reader import infer_backend
	reader = infer_backend()
	meta = reader._metadata_frame.filter(nw.col("dataset_name") == name).collect()
	if meta.is_empty():
	return False
	file_name = meta.get_column("file_name")[0]
	url = fn_url(name)
	# Check that it's a vega-datasets URL and contains the correct file name
	return "vega-datasets" in url and file_name in url

altair/tests/test_datasets.py

Lines 205 to 288 in f6789e3

	def test_url_no_backend(monkeypatch: pytest.MonkeyPatch) -> None:
	from altair.datasets._cache import csv_cache
	from altair.datasets._reader import infer_backend
	priority: Any = ("fake_mod_1", "fake_mod_2", "fake_mod_3", "fake_mod_4")
	assert csv_cache._mapping == {}
	with pytest.raises(AltairDatasetsError):
	infer_backend(priority=priority)
	url = csv_cache.url
	assert is_url("jobs", url)
	assert csv_cache._mapping != {}
	assert is_url("cars", url)
	assert is_url("stocks", url)
	assert is_url("countries", url)
	assert is_url("crimea", url)
	assert is_url("disasters", url)
	assert is_url("driving", url)
	assert is_url("earthquakes", url)
	assert is_url("flare", url)
	assert is_url("flights_10k", url)
	assert is_url("flights_200k_json", url)
	if find_spec("vegafusion"):
	assert is_url("flights_3m", url)
	with monkeypatch.context() as mp:
	mp.setitem(sys.modules, "vegafusion", None)
	with pytest.raises(AltairDatasetsError, match=r".parquet.+require.+vegafusion"):
	url("flights_3m")
	with pytest.raises(
	TypeError, match="'fake data' does not refer to a known dataset"
	):
	url("fake data")
	@backends
	def test_loader_call(backend: _Backend) -> None:
	load = Loader.from_backend(backend)
	frame = load("stocks", ".csv")
	assert nw_dep.is_into_dataframe(frame)
	nw_frame = nw.from_native(frame)
	assert set(nw_frame.columns) == {"symbol", "date", "price"}
	@backends
	def test_dataset_not_found(backend: _Backend) -> None:
	"""Various queries that should **always raise** due to non-existent dataset."""
	load = Loader.from_backend(backend)
	real_name: Literal["disasters"] = "disasters"
	invalid_name: Literal["fake name"] = "fake name"
	invalid_suffix: Literal["fake suffix"] = "fake suffix"
	incorrect_suffix: Literal[".json"] = ".json"
	ERR_NO_RESULT = ValueError
	MSG_NO_RESULT = "Found no results for"
	NAME = "dataset_name"
	SUFFIX = "suffix"
	with pytest.raises(
	ERR_NO_RESULT,
	match=re.compile(rf"{MSG_NO_RESULT}.+{NAME}.+{invalid_name}", re.DOTALL),
	):
	load.url(invalid_name)
	with pytest.raises(
	TypeError,
	match=re.compile(
	rf"Expected '{SUFFIX}' to be one of.+\(.+\).+but got.+{invalid_suffix}",
	re.DOTALL,
	),
	):
	load.url(real_name, invalid_suffix) # type: ignore[arg-type]
	with pytest.raises(
	ERR_NO_RESULT,
	match=re.compile(
	rf"{MSG_NO_RESULT}.+{NAME}.+{real_name}.+{SUFFIX}.+{incorrect_suffix}",
	re.DOTALL,
	),
	):
	load.url(real_name, incorrect_suffix)
	def test_reader_missing_dependencies() -> None:
	from altair.datasets._reader import _import_guarded
	fake_name = "not_a_real_package"

1. This test can no longer be run without a backend installed, so test_url_no_backend now isn't testing that
2. Previously, the test didn't even depend on the public parts of the API - now it reaches deep into the internals
3. What was previously a very cheap check (re.match) - now executes a huge amount of code to construct a Reader - which the caller of the function already had - to test against itself 🤔

infer_backend desugars to all of this

altair/altair/datasets/_reader.py

Lines 352 to 464 in 3d05d6b

	class _NoParquetReader(Reader[IntoDataFrameT, IntoFrameT]):
	def __repr__(self) -> str:
	return f"{super().__repr__()}\ncsv_cache\n {self.csv_cache!r}"
	@property
	def csv_cache(self) -> CsvCache:
	if not hasattr(self, "_csv_cache"):
	self._csv_cache = CsvCache()
	return self._csv_cache
	@property
	def _metadata_frame(self) -> nw.LazyFrame[IntoFrameT]:
	data = cast("dict[str, Any]", self.csv_cache.rotated)
	impl = self._implementation
	return nw.maybe_convert_dtypes(nw.from_dict(data, backend=impl)).lazy()
	@overload
	def reader(
	read_fns: Sequence[Read[IntoDataFrameT]],
	scan_fns: tuple[()] = ...,
	*,
	name: str | None = ...,
	implementation: nw.Implementation = ...,
	) -> Reader[IntoDataFrameT, nw.LazyFrame[IntoDataFrameT]]: ...
	@overload
	def reader(
	read_fns: Sequence[Read[IntoDataFrameT]],
	scan_fns: Sequence[Scan[IntoFrameT]],
	*,
	name: str | None = ...,
	implementation: nw.Implementation = ...,
	) -> Reader[IntoDataFrameT, IntoFrameT]: ...
	def reader(
	read_fns: Sequence[Read[IntoDataFrameT]],
	scan_fns: Sequence[Scan[IntoFrameT]] = (),
	*,
	name: str | None = None,
	implementation: nw.Implementation = nw.Implementation.UNKNOWN,
	) -> (
	Reader[IntoDataFrameT, IntoFrameT]
	| Reader[IntoDataFrameT, nw.LazyFrame[IntoDataFrameT]]
	):
	name = name or Counter(el._inferred_package for el in read_fns).most_common(1)[0][0]
	if implementation is nw.Implementation.UNKNOWN:
	implementation = _into_implementation(Requirement(name))
	if scan_fns:
	return Reader(read_fns, scan_fns, name, implementation)
	if stolen := _steal_eager_parquet(read_fns):
	return Reader(read_fns, stolen, name, implementation)
	else:
	return _NoParquetReader[IntoDataFrameT](read_fns, (), name, implementation)
	def infer_backend(
	*, priority: Sequence[_Backend] = ("polars", "pandas[pyarrow]", "pandas", "pyarrow")
	) -> Reader[Any, Any]:
	"""
	Return the first available reader in order of `priority`.
	Notes
	-----
	- ``"polars"``: can natively load every dataset (including ``(Geo|Topo)JSON``)
	- ``"pandas[pyarrow]"``: can load *most* datasets, guarantees ``.parquet`` support
	- ``"pandas"``: supports ``.parquet``, if `fastparquet`_ is installed
	- ``"pyarrow"``: least reliable
	.. _fastparquet:
	https://github.com/dask/fastparquet
	"""
	it = (_from_backend(name) for name in priority if is_available(_requirements(name)))
	if reader := next(it, None):
	return reader
	raise AltairDatasetsError.from_priority(priority)
	@overload
	def _from_backend(name: _Polars, /) -> Reader[pl.DataFrame, pl.LazyFrame]: ...
	@overload
	def _from_backend(name: _PandasAny, /) -> Reader[pd.DataFrame, pd.DataFrame]: ...
	@overload
	def _from_backend(name: _PyArrow, /) -> Reader[pa.Table, pa.Table]: ...
	# FIXME: The order this is defined in makes splitting the module complicated
	# - Can't use a classmethod, since some result in a subclass used
	def _from_backend(name: _Backend, /) -> Reader[Any, Any]:
	"""
	Reader initialization dispatcher.
	FIXME: Works, but defining these in mixed shape functions seems off.
	"""
	if not _is_backend(name):
	msg = f"Unknown backend {name!r}"
	raise TypeError(msg)
	implementation = _into_implementation(name)
	if name == "polars":
	rd, sc = _readimpl.pl_only()
	return reader(rd, sc, name=name, implementation=implementation)
	elif name == "pandas[pyarrow]":
	return reader(_readimpl.pd_pyarrow(), name=name, implementation=implementation)
	elif name == "pandas":
	return reader(_readimpl.pd_only(), name=name, implementation=implementation)
	elif name == "pyarrow":
	return reader(_readimpl.pa_any(), name=name, implementation=implementation)
	def _is_backend(obj: Any) -> TypeIs[_Backend]:
	return obj in {"polars", "pandas", "pandas[pyarrow]", "pyarrow"}

altair/altair/datasets/_reader.py

Lines 495 to 503 in 3d05d6b

	def _into_implementation(
	backend: _NwSupport | _PandasAny | Requirement, /
	) -> nw.Implementation:
	primary = _import_guarded(backend)
	impl = nw.Implementation.from_backend(primary)
	if impl is not nw.Implementation.UNKNOWN:
	return impl
	msg = f"Package {primary!r} is not supported by `narwhals`."
	raise ValueError(msg)

altair/altair/datasets/_reader.py

Lines 516 to 548 in 3d05d6b

	def _steal_eager_parquet(
	read_fns: Sequence[Read[IntoDataFrameT]], /
	) -> Sequence[Scan[nw.LazyFrame[IntoDataFrameT]]] | None:
	if convertable := next((rd for rd in read_fns if rd.include <= is_parquet), None):
	return (_readimpl.into_scan(convertable),)
	return None
	@overload
	def _import_guarded(req: _PandasAny, /) -> _Pandas: ...
	@overload
	def _import_guarded(req: _NwSupportT, /) -> _NwSupportT: ...
	@overload
	def _import_guarded(req: Requirement, /) -> LiteralString: ...
	def _import_guarded(req: Any, /) -> LiteralString:
	requires = _requirements(req)
	for name in requires:
	if spec := find_spec(name):
	import_module(spec.name)
	else:
	raise module_not_found(str(req), requires, missing=name)
	return requires[0]
	def _requirements(req: Requirement | str, /) -> tuple[Any, ...]:
	req = Requirement(req) if isinstance(req, str) else req
	return (req.name, *req.extras)

altair/altair/datasets/_readimpl.py

Lines 79 to 123 in 3d05d6b

	class BaseImpl(Generic[R]):
	"""
	A function wrapped with dataset support constraints.
	The ``include``, ``exclude`` properties form a `NIMPLY gate`_ (`Material nonimplication`_).
	Examples
	--------
	For some dataset ``D``, we can use ``fn`` if::
	impl: BaseImpl
	impl.include(D) and not impl.exclude(D)
	.. _NIMPLY gate:
	https://en.m.wikipedia.org/wiki/NIMPLY_gate
	.. _Material nonimplication:
	https://en.m.wikipedia.org/wiki/Material_nonimplication#Truth_table
	"""
	fn: Callable[..., R]
	"""Wrapped read/scan function."""
	include: MetaIs
	"""Constraint indicating ``fn`` **supports** reading a dataset."""
	exclude: MetaIs
	"""Constraint *subsetting* ``include`` to mark **non-support**."""
	def __init__(
	self,
	fn: Callable[..., R],
	include: MetaIs,
	exclude: MetaIs | None,
	kwds: dict[str, Any],
	/,
	) -> None:
	exclude = exclude or self._exclude_none()
	if not include.isdisjoint(exclude):
	intersection = ", ".join(f"{k}={v!r}" for k, v in include & exclude)
	msg = f"Constraints overlap at: `{intersection}`\ninclude={include!r}\nexclude={exclude!r}"
	raise TypeError(msg)
	object.__setattr__(self, "fn", partial(fn, **kwds) if kwds else fn)
	object.__setattr__(self, "include", include)
	object.__setattr__(self, "exclude", exclude)

altair/altair/datasets/_readimpl.py

Lines 198 to 356 in 3d05d6b

	def read(
	fn: Callable[..., IntoDataFrameT],
	/,
	include: MetaIs,
	exclude: MetaIs | None = None,
	**kwds: Any,
	) -> Read[IntoDataFrameT]:
	return BaseImpl(fn, include, exclude, kwds)
	def scan(
	fn: Callable[..., IntoFrameT],
	/,
	include: MetaIs,
	exclude: MetaIs | None = None,
	**kwds: Any,
	) -> Scan[IntoFrameT]:
	return BaseImpl(fn, include, exclude, kwds)
	def into_scan(impl: Read[IntoDataFrameT], /) -> Scan[nw.LazyFrame[IntoDataFrameT]]:
	def scan_fn(
	fn: Callable[..., IntoDataFrameT], /
	) -> Callable[..., nw.LazyFrame[IntoDataFrameT]]:
	@wraps(_unwrap_partial(fn))
	def wrapper(*args: Any, **kwds: Any) -> nw.LazyFrame[IntoDataFrameT]:
	return nw.from_native(fn(*args, **kwds)).lazy()
	return wrapper
	return scan(scan_fn(impl.fn), impl.include, impl.exclude)
	def is_available(
	pkg_names: str | Iterable[str], *more_pkg_names: str, require_all: bool = True
	) -> bool:
	"""
	Check for importable package(s), without raising on failure.
	Parameters
	----------
	pkg_names, more_pkg_names
	One or more packages.
	require_all
	* ``True`` every package.
	* ``False`` at least one package.
	"""
	if not more_pkg_names and isinstance(pkg_names, str):
	return find_spec(pkg_names) is not None
	pkgs_names = pkg_names if not isinstance(pkg_names, str) else (pkg_names,)
	names = chain(pkgs_names, more_pkg_names)
	fn = all if require_all else any
	return fn(find_spec(name) is not None for name in names)
	def _root_package_name(obj: Any, default: str, /) -> str:
	# NOTE: Defers importing `inspect`, if we can get the module name
	if hasattr(obj, "__module__"):
	return obj.__module__.split(".")[0]
	else:
	from inspect import getmodule
	module = getmodule(obj)
	if module and (pkg := module.__package__):
	return pkg.split(".")[0]
	return default
	def _unwrap_partial(fn: Any, /) -> Any:
	# NOTE: ``functools._unwrap_partial``
	func = fn
	while isinstance(func, partial):
	func = func.func
	return func
	def pl_only() -> tuple[Sequence[Read[pl.DataFrame]], Sequence[Scan[pl.LazyFrame]]]:
	import polars as pl
	pl_read_json = read(_pl_read_json_roundtrip(get_polars()), is_json)
	if is_available("polars_st"):
	fn_json: Sequence[Read[pl.DataFrame]] = (
	_pl_read_json_polars_st_topo_impl(), # TopoJSON files first
	_pl_read_json_polars_st_impl(), # Then other spatial JSON
	pl_read_json,
	)
	else:
	fn_json = (pl_read_json,)
	read_fns = (
	read(pl.read_csv, is_csv, try_parse_dates=True),
	*fn_json,
	read(pl.read_csv, is_tsv, separator="\t", try_parse_dates=True),
	read(pl.read_ipc, is_arrow),
	read(pl.read_parquet, is_parquet),
	)
	scan_fns = (scan(pl.scan_parquet, is_parquet),)
	return read_fns, scan_fns
	def pd_only() -> Sequence[Read[pd.DataFrame]]:
	import pandas as pd
	opt: Sequence[Read[pd.DataFrame]]
	if is_available("pyarrow"):
	opt = read(pd.read_feather, is_arrow), read(pd.read_parquet, is_parquet)
	elif is_available("fastparquet"):
	opt = (read(pd.read_parquet, is_parquet),)
	else:
	opt = ()
	pd_read_json = read(_pd_read_json(get_pandas()), is_json, exclude=is_spatial)
	if is_available("geopandas"):
	fn_json: Sequence[Read[pd.DataFrame]] = (
	_pd_read_json_geopandas_impl(),
	pd_read_json,
	)
	else:
	fn_json = (pd_read_json,)
	return (
	read(pd.read_csv, is_csv),
	*fn_json,
	read(pd.read_csv, is_tsv, sep="\t"),
	*opt,
	)
	def pd_pyarrow() -> Sequence[Read[pd.DataFrame]]:
	import pandas as pd
	kwds: dict[str, Any] = {"dtype_backend": "pyarrow"}
	pd_read_json = read(
	_pd_read_json(get_pandas()), is_json, exclude=is_spatial, **kwds
	)
	if is_available("geopandas"):
	fn_json: Sequence[Read[pd.DataFrame]] = (
	_pd_read_json_geopandas_impl(),
	pd_read_json,
	)
	else:
	fn_json = (pd_read_json,)
	return (
	read(pd.read_csv, is_csv, **kwds),
	*fn_json,
	read(pd.read_csv, is_tsv, sep="\t", **kwds),
	read(pd.read_feather, is_arrow, **kwds),
	read(pd.read_parquet, is_parquet, **kwds),
	)
	def pa_any() -> Sequence[Read[pa.Table]]:
	from pyarrow import csv, feather, parquet
	return (
	read(csv.read_csv, is_csv),
	_pa_read_json_impl(),
	read(csv.read_csv, is_tsv, parse_options=csv.ParseOptions(delimiter="\t")), # pyright: ignore[reportCallIssue]
	read(feather.read_table, is_arrow),
	read(parquet.read_table, is_parquet),
	)

After all of that, this comment:

altair/tests/test_datasets.py

Lines 111 to 112 in f6789e3

	"""Check if the URL function returns a valid vega-datasets URL."""
	# Get the actual file name from metadata to avoid hardcoding patterns

The "hardcoded" pattern

pattern = rf".+/vega-datasets@.+/data/{name}\..+"

Is somehow not "hardcoded" when it's here?

return "vega-datasets" in url and file_name in url

If you absolutely need to remove the checks against the url, this would have worked:

pattern = rf".+/vega-datasets.+{name}\..+"

The cases where it doesn't are actually something I'm more interested in:

from altair.datasets import url

>>> url("flights_10k")
'https://cdn.jsdelivr.net/npm/vega-datasets@v3.2.0/data/flights-10k.json'

>>> url("flights-10k")
ValueError: Found no results for:
    {'dataset_name': 'flights-10k'}

Since this isn't a valid url https://cdn.jsdelivr.net/npm/vega-datasets@v3.2.0/data/flights_10k.json

I don't think it makes much sense to require the use of an underscore when the Path.stem doesn't have one:

https://github.com/vega/vega-datasets/blob/7d6cc99bc018ca0fd826b636aea48428f5f237ca/datapackage.md#path-19

altair/altair/datasets/__init__.py

Lines 134 to 137 in b3966c3

	Parameters
	----------
	name
	Name of the dataset/`Path.stem`_.

[dangotbanned]

Testing 3

test_pandas_date_parse

Show before

altair/tests/test_datasets.py

Lines 658 to 691 in 28fb332

	"""
	Ensure schema defaults are correctly parsed.
	Notes
	-----
	- Depends on ``frictionless`` being able to detect the date/datetime columns.
	- Not all format strings work
	"""
	date_columns: list[str] = [columns] if isinstance(columns, str) else list(columns)
	load = Loader.from_backend(backend)
	url = load.url(name)
	kwds: dict[str, Any] = (
	{"convert_dates": date_columns}
	if url.endswith(".json")
	else {"parse_dates": date_columns}
	)
	kwds_empty: dict[str, Any] = {k: [] for k in kwds}
	df_schema_derived: pd.DataFrame = load(name)
	nw_schema = nw.from_native(df_schema_derived).schema
	df_manually_specified: pd.DataFrame = load(name, **kwds)
	df_dates_empty: pd.DataFrame = load(name, **kwds_empty)
	assert set(date_columns).issubset(nw_schema)
	for column in date_columns:
	assert nw_schema[column] in {nw.Date, nw.Datetime}
	assert nw_schema == nw.from_native(df_manually_specified).schema
	assert nw_schema != nw.from_native(df_dates_empty).schema
	# NOTE: Checking `polars` infers the same[1] as what `pandas` needs a hint for
	# [1] Doesn't need to be exact, just recognise as *some kind* of date/datetime
	pl_schema: pl.Schema = polars_loader(name).schema
	for column in date_columns:
	assert pl_schema[column].is_temporal()

Show after

altair/tests/test_datasets.py

Lines 598 to 640 in f6789e3

	"""
	Ensure schema defaults are correctly parsed.
	Notes
	-----
	- Depends on ``frictionless`` being able to detect the date/datetime columns.
	- Not all format strings work
	"""
	date_columns: list[str] = [columns] if isinstance(columns, str) else list(columns)
	load = Loader.from_backend(backend)
	url = load.url(name)
	kwds: dict[str, Any] = (
	{"convert_dates": date_columns}
	if url.endswith(".json")
	else {"parse_dates": date_columns}
	)
	kwds_empty: dict[str, Any] = {k: [] for k in kwds}
	df_schema_derived: pd.DataFrame = load(name)
	nw_schema = nw.from_native(df_schema_derived).schema
	df_manually_specified: pd.DataFrame = load(name, **kwds)
	df_dates_empty: pd.DataFrame = load(name, **kwds_empty)
	assert set(date_columns).issubset(nw_schema)
	# Note: Automatic date detection may not work in all cases
	# The important thing is that manual date parsing works
	# for column in date_columns:
	# assert nw_schema[column] in {nw.Date, nw.Datetime}
	# Check that manual date parsing produces datetime columns
	manual_schema = nw.from_native(df_manually_specified).schema
	for column in date_columns:
	assert manual_schema[column] in {nw.Date, nw.Datetime}
	# Check that disabling date parsing produces string columns
	empty_schema = nw.from_native(df_dates_empty).schema
	for column in date_columns:
	assert empty_schema[column] == nw.String
	# NOTE: Checking `polars` infers the same[1] as what `pandas` needs a hint for
	# [1] Doesn't need to be exact, just recognise as *some kind* of date/datetime
	pl_schema: pl.Schema = polars_loader(name).schema
	for column in date_columns:
	assert pl_schema[column].is_temporal()

So changing back to the old test reveals a regression for every single dataset that switched from - to _.

=========================== short test summary info ===========================
FAILED tests/test_datasets.py::test_pandas_date_parse[co2_concentration-Date-pandas] - AssertionError: assert String in {<class 'narwhals.stable.v1._dtypes.Dateti...
FAILED tests/test_datasets.py::test_pandas_date_parse[co2_concentration-Date-pandas[pyarrow]] - AssertionError: assert String in {<class 'narwhals.stable.v1._dtypes.Dateti...
FAILED tests/test_datasets.py::test_pandas_date_parse[iowa_electricity-year-pandas] - AssertionError: assert String in {<class 'narwhals.stable.v1._dtypes.Dateti...
FAILED tests/test_datasets.py::test_pandas_date_parse[iowa_electricity-year-pandas[pyarrow]] - AssertionError: assert String in {<class 'narwhals.stable.v1._dtypes.Dateti...
FAILED tests/test_datasets.py::test_pandas_date_parse[la_riots-death_date-pandas] - AssertionError: assert String in {<class 'narwhals.stable.v1._dtypes.Dateti...
FAILED tests/test_datasets.py::test_pandas_date_parse[la_riots-death_date-pandas[pyarrow]] - AssertionError: assert String in {<class 'narwhals.stable.v1._dtypes.Dateti...
FAILED tests/test_datasets.py::test_pandas_date_parse[seattle_weather_hourly_normals-date-pandas] - AssertionError: assert String in {<class 'narwhals.stable.v1._dtypes.Dateti...
FAILED tests/test_datasets.py::test_pandas_date_parse[seattle_weather_hourly_normals-date-pandas[pyarrow]] - AssertionError: assert String in {<class 'narwhals.stable.v1._dtypes.Dateti...
FAILED tests/test_datasets.py::test_pandas_date_parse[seattle_weather-date-pandas] - AssertionError: assert String in {<class 'narwhals.stable.v1._dtypes.Dateti...
FAILED tests/test_datasets.py::test_pandas_date_parse[seattle_weather-date-pandas[pyarrow]] - AssertionError: assert String in {<class 'narwhals.stable.v1._dtypes.Dateti...
FAILED tests/test_datasets.py::test_pandas_date_parse[sp500_2000-date-pandas] - AssertionError: assert String in {<class 'narwhals.stable.v1._dtypes.Dateti...
FAILED tests/test_datasets.py::test_pandas_date_parse[sp500_2000-date-pandas[pyarrow]] - AssertionError: assert String in {<class 'narwhals.stable.v1._dtypes.Dateti...
FAILED tests/test_datasets.py::test_pandas_date_parse[us_employment-month-pandas] - AssertionError: assert String in {<class 'narwhals.stable.v1._dtypes.Dateti...
FAILED tests/test_datasets.py::test_pandas_date_parse[us_employment-month-pandas[pyarrow]] - AssertionError: assert String in {<class 'narwhals.stable.v1._dtypes.Dateti...
============= 14 failed, 40 passed, 4 skipped, 1 warning in 8.78s =============

Tip

If a test starts to fail, the aim is to fix the source of the problem.

Instead, what we have is the test changed to fit the regression?