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

Renaming InstallationCandidate and friends #7031

Open
pradyunsg opened this issue Sep 18, 2019 · 8 comments
Open

Renaming InstallationCandidate and friends #7031

pradyunsg opened this issue Sep 18, 2019 · 8 comments
Labels
C: finder PackageFinder and index related code state: needs discussion This needs some more discussion type: maintenance Related to Development and Maintenance Processes

Comments

@pradyunsg
Copy link
Member

pradyunsg commented Sep 18, 2019

This is an offshoot from a discussion around #6607 and the future resolver work as part of #988. Also, naming things is difficult.


In this issue, I'm trying to jot down a few naming related concerns that I have w.r.t. the package index and resolver code in pip right now, mostly to get inputs from other folks and figure out actionable items. I want to try to make sure that no one has blocking concerns with whatever names and verbs we end up choosing here. That said, this can easily turn into bikeshedding so please don't block suggestions unless they're in the "no this can not work" territory.

I'll also quickly note that I do wish that I would've flagged these earlier, but well, I didn't (or I did and I don't recall why I didn't pursue them then). Sorry if this causes annoyance for anyone and know that that's not the intent here.


I think we should rename InstallationCandidate to something that does not conflict with the "candidate" from resolvelib's terminology. The two classes do not model the same concept and since the resolver and package-finder do pass around information, they should utilize a shared, non-conflicting vocabulary in them.

The InstallationCandidate name makes sense given pip's current resolution strategy for something from the index is: see all candidates from the package index and choose a "best candidate". Once chosen, this is the candidate that would be installed, ignoring any future conflicts.

However, in the resolvelib model, a "candidate" may be deselected if a conflict is found, which means a once-"selected" package index link does not always correspond to something that would be installed. It would if it is still selected at the end of the resolution process, but there are no guarantees during resolution.

If we agree that InstallationCandidate needs to be renamed, we'd also want to change some of the phrases we're using:

  • evaluate_link() needs a better description.
    • Current: returns whether a link is a candidate for install.
    • Suggestion: Evaluate if link is compatible with the current environment.
  • candidate for installation -> candidate (information?) from index

@techalchemy and I tried to come up with a better name to avoid this ambiguity, and after a lot of mental effort, we landed on IndexCandidateInfo. Personally, I think it's better than InstallationCandidate. It is clearer that the object is, information from the index about a "candidate" available from the index. However, we're still using the noun "candidate" as a part of this, which I'd ideally want us to not be using here.

None the less, to structure the discussion more linearly, there's basically 2 or 3 things I'd want to hear from interested folks on:

  • Does IndexCandidateInfo sound like a better-enough name for now to do a switch "now" and avoid renaming almost every class in index.py?
  • Does anyone else think there's enough value to be had, by completely avoiding the verb "candidate" in index.py?
    • If yes, do you have any suggestions for a better noun/verb combination -- for PackageFinder, Link, InstallationCandidate, "evaluate" link, "evaluate" candidate -- such that we avoid using "candidate"?
@pradyunsg pradyunsg added state: needs discussion This needs some more discussion type: maintenance Related to Development and Maintenance Processes C: finder PackageFinder and index related code labels Sep 18, 2019
@pradyunsg
Copy link
Member Author

/cc @cjerdonek @uranusjr @chrahunt

@cjerdonek
Copy link
Member

cjerdonek commented Sep 18, 2019

What's the usage of the word "candidate" in resolvelib that we're trying not to conflict with? E.g. can you give examples of class names, doc usage, etc?

It seems like one way to avoid conflicts would be to use "candidate" as an adjective (modifier of the noun after it) in index.py rather than as a noun (it's not currently being used as a verb). And as long as the noun part is different from what's used in resolvelib, it seems like things should be okay. Is it being used as a noun or adjective (modifier) in resolvelib?

Right now, InstallationCandidate is just a wrapped Link object (it's used to wrap those Link objects that are considered "candidates", or still in the running), where the Link object is coming from an index. How about CandidateIndexLink? Then the "noun" part is IndexLink (a type of Link object, since it's a wrapped Link), and the "adjective" (modifier) part is Candidate.

Then e.g. get_install_candidate() could be renamed to get_candidate_index_link().

I do think we should try to avoid class names with the word Info at the end because Info doesn't add anything as it's pretty abstract and non-descriptive..

(Technical grammar note: Actually, it turns out that "candidate" in this sense is still technically a noun, but it's a noun used as an adjective since it's modifying the noun after it. This usage has the name noun adjunct or attributive noun, like the "chicken" in "chicken soup," as chicken is a noun saying what kind of soup it is.)

@cjerdonek
Copy link
Member

I think it could even work to call the class IndexLink since we aren't currently using IndexLink for anything. Even with this name, though, I think we might still want to use the word "candidate" in some method names like get_candidate_index_link() to communicate that these IndexLink objects are the ones that were found to be candidates for install.

@cjerdonek
Copy link
Member

Another possibility is PackageLink. This is even better I think than IndexLink because (1) these objects won't necessarly come from package indexes (they can also come from e.g. command-line options like --find-links), and (2) each of these objects will be the combination of a Link object, a project name, and version number (the things needed for a package).

Also, the more I think about it, the more I think "candidate" shouldn't be part of the class name. The reason is that whether an object is a candidate isn't so much a property of the instance itself, but rather where in the process of filtering things out you are. For example, a Link object can start out as a candidate, but after you apply some criteria, it becomes no longer a candidate. But knowing that fact doesn't change anything about the underlying object. It just changes whether and how the object should be used.

Another way of viewing this is that "candidate-ness" is just a boolean flag or piece of information. And we don't need a whole class just to represent objects for which the flag is true. You can do that by filtering collections and using variable names to distinguish the candidates from the non-candidates (e.g. package_links vs. candidate_links, etc).

@pradyunsg
Copy link
Member Author

pradyunsg commented Sep 19, 2019

What's the usage of the word "candidate" in resolvelib that we're trying not to conflict with? E.g. can you give examples of class names, doc usage, etc?

Whoops. I'd meant to link the first instance of that reference to https://github.com/sarugaku/resolvelib#candidate. It's basically a choice that the resolver can make -- something that can be in the resolved set.

To be abundantly clear, the Link in InstallationCandidate today does point to a concrete candidate that we could download and interact with. However, it's not the actual Python object we'd be using in the resolver as candidates which is where I think the confusion could/would/does arise.

I think it'll help if we avoid using "candidate" in our index interaction code, so that is unambiguously a resolver-only word/concept. It could well be that I'm drawing the line in the wrong place here.

candidates for install

I have a minor concern with this phrasing -- I think we'd be better off not referring to the actions may we take later, in the index interaction code.

As an example, pip would not be installing when using pip download but it's still interacting with the index. Similarly, we could potentially not download an asset behind a link, in a world where we could get metadata about that asset separately.


CandidateIndexLink

My brain bundles this as candidate-index link, which isn't what we're going for here. :(

IndexLink

I'm OK with this. I'm also not a fan though, since "IndexLink" and "Link" sound the same but are different. Also, I imagine the variable naming could get mixed up very quickly. ;)

Technical grammar note

^ thanks for this. :)

@pradyunsg
Copy link
Member Author

Also, the more I think about it, the more I think "candidate" shouldn't be part of the class name.

Yay! We're all on the same page. :)

PackageLink

I agree, this is definitely the nicest name yet. :)

@cjerdonek
Copy link
Member

Great! I'm also okay with the idea of phasing out usages of the word "candidate" in collector.py and index.py to the extent possible, though maybe that can happen more gradually instead of trying to do it at the same time as the class rename (as there are more choices to be made).

@pradyunsg
Copy link
Member Author

maybe that can happen more gradually instead of trying to do it at the same time as the class rename (as there are more choices to be made).

Makes sense. I don't imagine we'd be doing all this in a single PR anyway. :P

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: finder PackageFinder and index related code state: needs discussion This needs some more discussion type: maintenance Related to Development and Maintenance Processes
Projects
None yet
Development

No branches or pull requests

2 participants