resolver.py: Add CustomPackageProvider class #541
Conversation
prarit
force-pushed
the
RHELAI-3189
branch
2 times, most recently
from
6b8e855 to
6a37714
Compare
There was a problem hiding this comment.
This looks good! Just a couple of suggestions:
- Can you add some test cases for
find_best_match()function, if possible? - I think (from past experience) we don't want to mention JIRA reference in description since this is upstream and we don't want to share about RHELAI.
I will leave it to @dhellmann for final approval
src/fromager/resolver.py
Outdated
| self, | ||
| req: Requirement, | ||
| constraints: Constraints, | ||
| versions: dict[Version, str], |
There was a problem hiding this comment.
We have #489 describing creating a class that maps versions to arbitrary values and offering a way to resolve to the "right" value. That would invert the direction of this PR, since the dict-like object would be the main interface. I wonder if that would provide a more natural API for consumers for this use case. We could hide a GenericProvider inside of the VersionMap type, for example.
There was a problem hiding this comment.
I added VersionsMap to see if it made a big difference -- it doesn't AFAICT so I've submitted a v2 with that change.
There was a problem hiding this comment.
I'll leave it up to you to resolve this comment @dhellmann -- I'm not sure if I captured your concern or not.
prarit
force-pushed
the
RHELAI-3189
branch
5 times, most recently
from
7edf844 to
656a8c0
Compare
src/fromager/resolver.py
Outdated
| for version, _ in versions: | ||
| # Candidates require a URL for initialization: Use localhost as a place holder | ||
| # Candidates must match the requirement name in is_satisfied_call below | ||
| candidate = Candidate(self.req.name, version, "https://localhost") |
There was a problem hiding this comment.
versionis type str, butCandidateexpects typeVersionhere.localhostcan resolve to a HTTPS server if a systems runs a web server, too. RFC 2606 reserves TLD.invalidas place holder. I suggest to usehost.invalidhere to indicate that the candidate does not have a valid URL.
| candidate = Candidate(self.req.name, version, "https://localhost") | |
| candidate = Candidate(self.req.name, version, "https://host.invalid") |
Alternative: Change Candidate.url to support None. It's going to require more refactoring.
There was a problem hiding this comment.
Ah thanks. I would never have figured that one out :). I've applied the host.invalid change in the new push. I think modifying Candidate.url is a little out of scope for me atm.
src/fromager/resolver.py
Outdated
| @@ -200,6 +200,7 @@ def get_project_from_pypi( | |||
| [str, RequirementsMap, CandidatesMap], | |||
| typing.Iterable[tuple[str, str | Version]], | |||
| ] | |||
| VersionsMap: typing.TypeAlias = typing.Mapping[str, typing.Iterable[Version]] | |||
There was a problem hiding this comment.
I think my explanation of VersionMap wasn't clear.
The idea was to have VersionMap be an association from a Version to an arbitrary value. Then the user of the map could query the map with a Requirement specifier and Constraint, and have the map give back the value associated with the version that matches that rule.
Our plugin could then instantiate a VersionMap, populate it, and query it without having to set up and use a Provider on its own.
I've put up an example in #542
JIRA: https://issues.redhat.com/browse/RHELAI-3189 The builder project contains code that attempts to satisfy constraints and package requirements. This code is effectively replicating is_satsified_by() of the GenericProvider class and should be removed from builder in favor of a new class in fromager. The new class has find_best_match(), which unlike find_matches() returns the best match in a provided list of versions. v2: sort versions, minor style changes, add VersionsMap v3: Fix https://host.invalid in Candidate creation, add None return to init(), remove VersionMap implementation Suggested-by: Doug Hellmann <[email protected]> Signed-off-by: Prarit Bhargava <[email protected]>
is_satisfied_by() returns false if a requirement is defined without a specifier. For example using 'aotriton' (no other arguments) results in a failure regardless of any constraint specified. In this case it is safe to return True and return a tested value for the constraint. Fix is_satisfied_by() to handle requirements without specifiers. Signed-off-by: Prarit Bhargava <[email protected]>
JIRA: https://issues.redhat.com/browse/RHELAI-3189
The builder project contains code that attempts to satisfy constraints and package requirements. This code is effectively replicating is_satsified_by() of the GenericProvider class and should be removed from builder in favor of a new class in fromager.
The new class has find_best_match(), which unlike find_matches() returns the best match in a provided list of versions.
Suggested-by: Doug Hellmann [email protected]
Signed-off-by: Prarit Bhargava [email protected]