Make default module mapping logic consistent and easy to understand #20513

purajit · 2024-02-09T00:55:24Z

This was motivated by trying to add some additional module and stub-related mappings.

While doing that, I realized the patterns only applied to implementation modules only, and realized that a
refactor would be the easiest way forward, while also making the logic easier to step through.

I haven't added any of those module mappings in this PR since I wanted this to only be the logic change.

Changes

Make the replacement functions more generic (allow injecting any separator)
Better type hinting - more complete types for generics, also made return values more specific (return values and variables should be hinted as specifically as possible; functions can choose to accept more generic types as arguments)
Made impl and type package mappings behave identically
The previous logic seemed to have potential for bugs - it does a fallback on each call, has some hackiness around checking for a stubs package, nested conditions, etc. Simplified the module lookup logic. Now it's just:
- check if it's in the standard module mapping
- else check if it's in the type stub module mapping
- else check if the patterns for type stub modules match - this needs to be done first because a stub module might match an implementation module pattern (example, python-pkg-types)
- else check if the patterns for impl modules match
- else use the fallback
Update tests to ensure symmetry between standard modules and type stub modules
Added new test for test_generate_type_stub_mappings_from_pattern_matches_para, which is the main new logic here
All other changes created by pants fmt

Testing

Added and modified doctest examples, and python src/python/pants/backend/python/dependency_inference/default_module_mapping.py passes
pants test src/python/pants/backend/python/dependency_inference/:: passes
Ran this build on our monorepo, which uses quite a tangle of every feature here
CI

src/python/pants/backend/python/dependency_inference/default_module_mapping.py

purajit · 2024-02-09T21:46:10Z

src/python/pants/backend/python/dependency_inference/default_module_mapping.py

+    re.compile(r"""^stubs_(.+)"""): [first_group_hyphen_to_underscore],
+    re.compile(r"""^types_(.+)"""): [first_group_hyphen_to_underscore],
+    re.compile(r"""^stubs-(.+)"""): [first_group_hyphen_to_underscore],
+    re.compile(r"""^types-(.+)"""): [first_group_hyphen_to_underscore],
+    re.compile(r"""^(.+)_stubs"""): [first_group_hyphen_to_underscore],
+    re.compile(r"""^(.+)_types"""): [first_group_hyphen_to_underscore],
+    re.compile(r"""^(.+)-stubs"""): [first_group_hyphen_to_underscore],
+    re.compile(r"""^(.+)-types"""): [first_group_hyphen_to_underscore],


these do the work of the fallback[6:]/fallback[:6] before, without needing to check for those, or assuming prefix length

purajit · 2024-02-09T21:47:07Z

src/python/pants/backend/python/dependency_inference/module_mapper.py

-
-
-@functools.cache
-def generate_mappings(proj_name: str, fallback_value: str) -> Iterable[str]:


Not needed anymore; essentially, the only thing that should care about the fallback logic should be the main loop. The rest should just deal with their own domains.

purajit · 2024-02-09T21:47:38Z

src/python/pants/backend/python/dependency_inference/module_mapper.py

@@ -355,23 +349,23 @@ async def map_third_party_modules_to_addresses(
    for tgt in all_python_targets.third_party:
        resolve = tgt[PythonRequirementResolveField].normalized_value(python_setup)

-        def add_modules(modules: Iterable[str], *, type_stub: bool = False) -> None:
+        def add_modules(modules: Iterable[str], *, is_type_stub: bool) -> None:


Removed default to ensure explicitness of something critical

src/python/pants/backend/python/dependency_inference/module_mapper_test.py

purajit · 2024-02-09T21:57:49Z

src/python/pants/backend/python/dependency_inference/module_mapper_test.py

+    assert generate_mappings_from_pattern(proj_name, is_type_stub=False) == expected_modules
+
+
+@pytest.mark.parametrize(


this is the new test

purajit · 2024-02-09T21:58:42Z

src/python/pants/backend/python/dependency_inference/module_mapper_test.py

+    for pattern, replacements in [
+        *DEFAULT_MODULE_PATTERN_MAPPING.items(),
+        *DEFAULT_TYPE_STUB_MODULE_PATTERN_MAPPING.items(),
+    ]:


this and the above add symmetry between the two. From here till the last comment in this file are just pants fmt changes

purajit · 2024-02-09T22:21:32Z

src/python/pants/backend/python/dependency_inference/default_module_mapping.py

+    re.compile(r"""^stubs[_-](.+)"""): [first_group_hyphen_to_underscore],
+    re.compile(r"""^types[_-](.+)"""): [first_group_hyphen_to_underscore],
+    re.compile(r"""^(.+)[_-]stubs"""): [first_group_hyphen_to_underscore],
+    re.compile(r"""^(.+)[_-]types"""): [first_group_hyphen_to_underscore],


these do the work of fallback[6:]/fallback[:6] before

cognifloyd

Looks sane.

kaos

Thanks!

huonw · 2024-02-22T01:39:47Z

There's a few approvals here, is this waiting on something before merge? (thanks all for contributing and reviewing!)

kaos · 2024-02-22T08:00:20Z

Nothing but a little time for us to find if we overlooked something during review, I think. Thanks for bumping.

purajit changed the title ~~20240207 module mapping logic~~ make default module mapping logic consistent and easy to understand Feb 9, 2024

purajit changed the title ~~make default module mapping logic consistent and easy to understand~~ [WIP] make default module mapping logic consistent and easy to understand Feb 9, 2024

purajit force-pushed the 20240207-module-mapping-logic branch from c103685 to 6a1e8ad Compare February 9, 2024 01:57

make default module mapping logic consistent and easy to understand

6d3c78a

purajit force-pushed the 20240207-module-mapping-logic branch from 6a1e8ad to 6d3c78a Compare February 9, 2024 21:42

added test for stub pattern matching mapping

7f60f74

purajit force-pushed the 20240207-module-mapping-logic branch from 5f10a36 to 7f60f74 Compare February 9, 2024 21:54

purajit commented Feb 9, 2024

View reviewed changes

purajit changed the title ~~[WIP] make default module mapping logic consistent and easy to understand~~ Make default module mapping logic consistent and easy to understand Feb 9, 2024

purajit marked this pull request as ready for review February 9, 2024 22:15

squish regexes together

5bd6543

purajit commented Feb 9, 2024

View reviewed changes

cognifloyd approved these changes Feb 9, 2024

View reviewed changes

cognifloyd added category:internal CI, fixes for not-yet-released features, etc. backend: Python Python backend-related issues labels Feb 9, 2024

switch order of impl and stub pattern checks

d983ef3

kaos approved these changes Feb 12, 2024

View reviewed changes

kaos merged commit 42ff1dd into pantsbuild:main Feb 22, 2024
24 checks passed

purajit deleted the 20240207-module-mapping-logic branch March 21, 2024 17:17

purajit mentioned this pull request Jun 26, 2024

Bump default terraform version to 1.9.0, add all known terraform versions inbetween #21110

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make default module mapping logic consistent and easy to understand #20513

Make default module mapping logic consistent and easy to understand #20513

purajit commented Feb 9, 2024 •

edited

Loading

purajit Feb 9, 2024 •

edited

Loading

purajit Feb 9, 2024

purajit Feb 9, 2024

purajit Feb 9, 2024

purajit Feb 9, 2024

purajit Feb 9, 2024

cognifloyd left a comment

kaos left a comment

huonw commented Feb 22, 2024

kaos commented Feb 22, 2024



		@functools.cache
		def generate_mappings(proj_name: str, fallback_value: str) -> Iterable[str]:

		assert generate_mappings_from_pattern(proj_name, is_type_stub=False) == expected_modules


		@pytest.mark.parametrize(

Make default module mapping logic consistent and easy to understand #20513

Make default module mapping logic consistent and easy to understand #20513

Conversation

purajit commented Feb 9, 2024 • edited Loading

Changes

Testing

purajit Feb 9, 2024 • edited Loading

Choose a reason for hiding this comment

purajit Feb 9, 2024

Choose a reason for hiding this comment

purajit Feb 9, 2024

Choose a reason for hiding this comment

purajit Feb 9, 2024

Choose a reason for hiding this comment

purajit Feb 9, 2024

Choose a reason for hiding this comment

purajit Feb 9, 2024

Choose a reason for hiding this comment

cognifloyd left a comment

Choose a reason for hiding this comment

kaos left a comment

Choose a reason for hiding this comment

huonw commented Feb 22, 2024

kaos commented Feb 22, 2024

purajit commented Feb 9, 2024 •

edited

Loading

purajit Feb 9, 2024 •

edited

Loading