Pip: Can we flatten `pip._internal` into something else?

[pradyunsg]

Can we flatten pip._internal into something else?

_Originally posted by @pradyunsg in https://github.com/pypa/pip/pull/8466#issuecomment-646159513_

I'm thinking of basically doing str.replace("internal.", "") on our imports:

• pip._internal.network -> pip._network
• pip._internal.cache -> pip._cache
• (you get the idea)
• pip._vendor stays as is.

It'll make them a bit... nicer. It'll be easier to write these imports, to read them and so on.

Before someone suggests a pip -> _pip, that transition would eat into a lot more of our churn budget and we'll need to keep the pip package anyway, if we don't want a repeat of #5599. (did you forget that's a thing? Ubuntu users haven't.)

[pradyunsg]

We do have 1 thing to lose here -- GitHub's History feature does not follow renames out-of-the-box.

[McSinyx]

Generally I'd be happy if we have this. We can also move the packages without the underscore and tell people not to use any of pip's internal libraries like we're doing currently, because TBH because _spam looks ugly and communication is key.

GitHub's History feature does not follow renames out-of-the-box.

Does this mean that blaming wouldn't work on web UI and we need to follow the rename's parent commit and blame there? I've just tried locally and git blame would still work though.

Another inconvenience is recursive grep/ack/rp: we'd need to filter out _vendor somehow and it's not immediately obvious for me to do it with only ack for example (well I can just pipe it through e.g. grep -v _vendor but :disappointed:). If possible, I suggest having a pip_vendor package instead.

[dstufft]

We used to not have any prefix at all and it caused a lot of confusion where people wouldn’t realize pip doesn’t have an api because it was following a convention that says “hey these names are public”. I don’t think we should lose some sort of prefix.

I think the _internal name is clearer and makes it far more obvious to people not to touch it. If we want imports to be nicer we could use relative imports instead.

Sent from my iPhone

On Jun 24, 2020, at 12:33 AM, Nguyễn Gia Phong [e-mail] wrote:


Generally I'd be happy if we have this. We can also move the packages without the underscore and tell people not to use any of pip's internal libraries like we're doing currently, because TBH because _spam looks ugly and communication is key.

GitHub's History feature does not follow renames out-of-the-box.

Does this mean that blaming wouldn't work on web UI and we need to follow the rename's parent commit and blame there? I've just tried locally and git blame would still work though.

Another inconvenience is recursive grep/ack/rp: we'd need to filter out _vendor somehow and it's not immediately obvious for me to do it with only ack for example (well I can just pipe it through e.g. grep -v _vendor but 😞). If possible, I suggest having a pip_vendor package instead.

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or unsubscribe.

[uranusjr]

Personally I feel this is little merit. The import names are cumbersome, but removing _internal isn’t helping much. OTOH this would break literally everything using pip internals, again. I know pip doesn’t really have an API and you’re at your own risk doing it etc, but it is still not a nice thing to do simply because pip maintainers think it looks better. (And also note that this change would not really help prevent #8466.)

[pfmoore]

I agree, I don't think this has sufficient benefit to justify the impact. And the whole point of the move to _internal was to send the signal to end users. Removing that for the maintainers' convenience seems like a change in priority, as well as yet more churn that gratuitously breaks users (yes, users that are using undocumented features, but we could say the same about half of the command line behaviour of pip...)

[pradyunsg]

Well, I remember the _internal move very clearly. :)

Okay, there seems to be some sort of mis-understanding that we're dropping the entire prefix _internal. To be abundantly clear, these names are still _ prefixed. They'll still follows the "this is an implementation detail" naming convention for Python, but without an extra namespace level.

Here's a bigger example:

from pip._internal.distributions import AbstractDistribution
from pip._internal.index.package_finder import PackageFinder
from pip._internal.models.link import Link
from pip._internal.network.download import Downloader
from pip._internal.req.req_install import InstallRequirement
from pip._internal.req.req_tracker import RequirementTracker
from pip._internal.utils.hashes import Hashes

from pip._distributions import AbstractDistribution
from pip._index.package_finder import PackageFinder
from pip._models.link import Link
from pip._network.download import Downloader
from pip._req.req_install import InstallRequirement
from pip._req.req_tracker import RequirementTracker
from pip._utils.hashes import Hashes

Please note that I'm not saying that we switch pip._internal.utils to pip.utils but rather, to pip._utils.

OTOH this would break literally everything using pip internals, again.
users that are using undocumented features, but we could say the same about half of the command line behaviour of pip

I really don't think "this will break users who do unsupported things" is a strong line of reasoning here.

Unlike the CLI commands, the expectations around this area (using things under import pip) are clearly set in https://pip.pypa.io/en/stable/user_guide/#using-pip-from-your-program -- that there's no public API and no guarantees that things will work.

I understand your concern, but I don't think we should not make improvements to pip's codebase because someone is doing things that are explicitly documented as unsupported (which is different from not-documented behaviors).

note that this change would not really help prevent #8466

Oh, yea. This is an unrelated suggestion, that stemmed from the discussion/statements said there.

[pfmoore]

I do get that there's still an underscore. I just don't think it's as strong an indication.

I really don't think "this will break users who do unsupported things" is a strong line of reasoning here.

My argument is more "we already broke people doing unsupported things, it seems unfair to do it again" although I acknowledge that being able to do so is precisely why we put the API under _internal.

I'm not going to make a fight over this, but I don't think the imports are that bad, either.

[pradyunsg]

I don’t think we should lose some sort of prefix.

Agreed -- the question I'm asking here is, do we think pip._XYZ is better-enough a prefix to justify the effort (This PR would break all outstanding PRs at that point).

I think the _internal name is clearer and makes it far more obvious to people not to touch it.

I agree -- I do think just a plain _ is also sufficient, and I wanna know what others think here.

I do get that there's still an underscore. I just don't think it's as strong an indication.

That's fair. I wanna know how we all feel about such a change -- and I'm not in "we should do this" mode, but rather "should we do this?". :)

If we want imports to be nicer we could use relative imports instead.

Perhaps!

[uranusjr]

We can use relative imports, it’s just not very nice to use right now because the nesting level is quite uneven in many places. Maybe what we ought to do is to flatten the contents of pip._internal instead? So say in pip._internal.index I can do from .utils.packaging import dist_in_usersite.

[pradyunsg]

I don't really feel strongly about this (so if there's no concensus that X is better, status quo wins). That said, I do think that it'd be worthwhile to see if there's some other form (of code organization or import approach) that we feel would be better than status quo.

---

(example, from src/pip/_internal/index/package_finder.py)

Current:

from pip._internal.exceptions import (
    BestVersionAlreadyInstalled,
    DistributionNotFound,
    InvalidWheelFilename,
    UnsupportedWheel,
)
from pip._internal.index.collector import parse_links
from pip._internal.models.candidate import InstallationCandidate
from pip._internal.models.format_control import FormatControl
from pip._internal.models.link import Link
from pip._internal.models.selection_prefs import SelectionPreferences
from pip._internal.models.target_python import TargetPython
from pip._internal.models.wheel import Wheel
from pip._internal.utils.filetypes import WHEEL_EXTENSION
from pip._internal.utils.logging import indent_log
from pip._internal.utils.misc import build_netloc
from pip._internal.utils.packaging import check_requires_python
from pip._internal.utils.typing import MYPY_CHECK_RUNNING
from pip._internal.utils.unpacking import SUPPORTED_EXTENSIONS
from pip._internal.utils.urls import url_to_path

Relative imports:

from ..exceptions import (
    BestVersionAlreadyInstalled,
    DistributionNotFound,
    InvalidWheelFilename,
    UnsupportedWheel,
)
from ..models.candidate import InstallationCandidate
from ..models.format_control import FormatControl
from ..models.link import Link
from ..models.selection_prefs import SelectionPreferences
from ..models.target_python import TargetPython
from ..models.wheel import Wheel
from ..utils.filetypes import WHEEL_EXTENSION
from ..utils.logging import indent_log
from ..utils.misc import build_netloc
from ..utils.packaging import check_requires_python
from ..utils.typing import MYPY_CHECK_RUNNING
from ..utils.unpacking import SUPPORTED_EXTENSIONS
from ..utils.urls import url_to_path
from .collector import parse_links

str.replace("._internal.", "._"):

from pip._exceptions import (
    BestVersionAlreadyInstalled,
    DistributionNotFound,
    InvalidWheelFilename,
    UnsupportedWheel,
)
from pip._index.collector import parse_links
from pip._models.candidate import InstallationCandidate
from pip._models.format_control import FormatControl
from pip._models.link import Link
from pip._models.selection_prefs import SelectionPreferences
from pip._models.target_python import TargetPython
from pip._models.wheel import Wheel
from pip._utils.filetypes import WHEEL_EXTENSION
from pip._utils.logging import indent_log
from pip._utils.misc import build_netloc
from pip._utils.packaging import check_requires_python
from pip._utils.typing import MYPY_CHECK_RUNNING
from pip._utils.unpacking import SUPPORTED_EXTENSIONS
from pip._utils.urls import url_to_path

[pfmoore]

Purely personal views:

• Relative imports would leave me counting dots. I'm not over-keen on that.
• pip._utils form - nothing wrong with this
• pip._internal - I don't actually find it that bad

Basically, status quo is fine with me, _utils is OK if people want to change.

[xavfernandez]

FWIW pip._internal is also fine with me.

[chrahunt]

I'm also in favor of keeping pip._internal, since it keeps our internal files looking tidy with no prefix required.

I'm slightly against pip._utils because of the broken PRs and harder history tracking, as mentioned above.

[pradyunsg]

Sounds like the concensus is to stick with what we have! ^>^

Thanks all!