Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update dependency inference for Python & JVM to have visibility by resolves for first-party code #14293

Closed
Eric-Arellano opened this issue Jan 28, 2022 · 3 comments
Assignees

Comments

@Eric-Arellano
Copy link
Contributor

#13621 but for first-party targets this time. If you have a python_source target that uses Resolve A, but you use Resolve B, then we should not infer a dep from B to A because the resolves are incompatible.

Key point! See the design doc for #13882 at https://docs.google.com/document/d/1mzZWnXiE6OkgMH_Dm7WeA3ck9veusQmr_c6GWPb_tsQ/edit#. What this means is that if you have a python_source target that works with both resolves A and B, really you will parametrize two distinct targets, one per resolve. Dependency inference will then infer whether to sue the A variant or the B variant, based on what the dependee uses itself. Without this change, we would have ambiguity that there are two providers of that file.

We can implement this change before implementing parametrization.

@Eric-Arellano
Copy link
Contributor Author

To implement this, we'll take a similar approach to how third-party dependency inference works:

@dataclass(frozen=True)
class ThirdPartyPackageToArtifactMapping:
mapping_roots: FrozenDict[_ResolveName, FrozenTrieNode]
def addresses_for_symbol(self, symbol: str, resolve: str) -> FrozenOrderedSet[Address]:

That is, have a distinct "universe" of dependencies per resolve name. Our objective is to update this code so that we pass the resolve to both first-party mapping and third-party mapping:

for typ in types:
first_party_matches = first_party_dep_map.symbols.addresses_for_symbol(typ)
third_party_matches = (
third_party_artifact_mapping.addresses_for_symbol(typ, resolve)
if java_infer_subsystem.third_party_imports
else FrozenOrderedSet()
)

That means we will need to change how symbol_mapper.py works, to key by resolve. I suspect we will want each FirstPartyMappingRequest to handle this, i.e. the Java specific and Scala specific implementations. Then the core code will probably merge on a per-resolve basis - not certain, Python doesn't have this code yet.

--

A question I had is if the key should include the scala version - you can only infer a dep on something that shares the same scala version. Precisely, the key would be tuple[resolve_name, scala_version]. It sounds like that isn't necessary because we already associate each resolve with a particular scala version, so using the same resolve implies using the same Scala version.

That would be great to keep this code path simpler.

tdyas pushed a commit that referenced this issue Feb 15, 2022
As described in #14293, dependency inference does not currently work properly for first-party sources when multiple resolves are configured. This PR fixes the issue for JVM (Java/Scala) first-party sources.

The solution is similar to what was already done for third-party dependency inference: make the resolve part of the key in the symbol map. With this change, dependency inference looks up symbols by both name and resolve.
alonsodomin pushed a commit to alonsodomin/pants that referenced this issue Feb 25, 2022
…4491)

As described in pantsbuild#14293, dependency inference does not currently work properly for first-party sources when multiple resolves are configured. This PR fixes the issue for JVM (Java/Scala) first-party sources.

The solution is similar to what was already done for third-party dependency inference: make the resolve part of the key in the symbol map. With this change, dependency inference looks up symbols by both name and resolve.
@Eric-Arellano
Copy link
Contributor Author

Tom already fixed JVM in #14491.

I've done most of the update for Python in #14486, only have to figure out the algorithm to merge all the mappings + update tests. Help appreciated if anyone is looking to contribute :)

@Eric-Arellano Eric-Arellano self-assigned this Mar 8, 2022
@Eric-Arellano
Copy link
Contributor Author

Closed by #14486

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

No branches or pull requests

1 participant