uranusjr commented on code in PR #76890:
URL: https://github.com/apache/airflow/pull/76890#discussion_r4547725546
##########
shared/module_loading/src/airflow_shared/module_loading/__init__.py:
##########
@@ -178,12 +173,18 @@ def _get_grouped_entry_points() -> dict[str, list[EPnD]]:
# A distribution is yielded once per sys.path entry it is found on
(e.g. RHEL/Fedora
# venvs list both lib64/ and lib/ site-packages), possibly in
different versions.
# Only the first one is importable, so it is the only one whose
entry points count.
- dist_metadata = dist.metadata
- if dist_metadata and (name := dist_metadata.get("Name")):
- normalized_name = _normalize_dist_name(name)
- if normalized_name in seen_dist_names:
+ # No public API returns the name without parsing METADATA.
_normalized_name is private,
+ # but Python's own importlib.metadata.entry_points() deduplicates
on it too, and it comes
+ # from the .dist-info directory name. AttributeError is not
caught, so a Python without
+ # it fails loudly here.
Review Comment:
Hmm, the AttributeError is caught by the outer except Exception (Line 192),
so every distribution is skipped with a warning. Airflow then starts with no
providers or plugins. Maybe we could catch it here too. if we doesn’t, this
comment needs to be fixed.
##########
shared/module_loading/tests/module_loading/test_entry_points.py:
##########
@@ -97,17 +99,44 @@ def
test_entry_points_with_dist_same_dist_in_two_versions_returns_first_only():
]
-# No metadata file at all, or metadata without a Name field (the key is
absent, not None).
[email protected]("dist_metadata", [None, {"Version": "1.0.0"}],
ids=["no-metadata", "no-name"])
-def
test_entry_points_with_dist_without_name_are_not_deduplicated(dist_metadata):
+def test_entry_points_with_dist_deduplicates_without_reading_metadata():
+ distributions = [
+ MockDistribution("apache-airflow-providers-foo",
[metadata.EntryPoint("a", "b", "group_x")]),
+ MockDistribution("apache_airflow_providers_foo",
[metadata.EntryPoint("c", "d", "group_x")]),
+ MockDistribution("apache-airflow-providers-bar",
[metadata.EntryPoint("e", "f", "group_x")]),
+ ]
+
+ with (
+ mock.patch("airflow_shared.module_loading.metadata",
MockMetadata(distributions)),
+ mock.patch.object(
+ MockDistribution, "metadata", create=True,
new_callable=mock.PropertyMock
+ ) as metadata_property,
+ ):
+ entries = list(entry_points_with_dist("group_x"))
+
+ assert [ep.name for ep, _ in entries] == ["a", "e"]
+ metadata_property.assert_not_called()
+
+
+# When the directory name gives no name, _normalized_name reads METADATA
instead: a missing file
+# raises TypeError, a file without a Name field raises KeyError.
Review Comment:
Which error you get actually depends on the version; `importlib-metadata`
changed this around 8.0, and stdlib has yet another different version.
(Investigated by Claude.)
Distribution metadata are parsed with `EmailMessage`, which actually never
raises KeyError but just silently returns an empty string. It’s
`importlib-metadata` that handles the situation differently.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
