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

App data seeder by default should use bundled only #1833

Closed
wants to merge 1 commit into from

Conversation

gaborbernat
Copy link
Contributor

@gaborbernat gaborbernat commented May 19, 2020

This means latest compatible version between embed wheels and the ones
found under the path provided by --extra-search-dir. Report at the
end the installed wheel versions.

Resolves #1821.

@pfmoore
Copy link
Member

pfmoore commented May 19, 2020

I'm now confused. I thought that using the bundled versions was the default behaviour at the moment? What is this PR changing, in end user terms?

@gaborbernat
Copy link
Contributor Author

I'm now confused. I thought that using the bundled versions was the default behaviour at the moment? What is this PR changing, in end user terms?

Beforehand it picked up the latest available wheel within our app data cache. The app data cache can be populated from three locations: embedded wheels, extra search directories passed in, and images pulled in due to a --download run. So in theory beforehand either a --extra-search-dir or a --download run could silently upgrade in the background subsequent runs seeded wheels. This PR removes this subtle behaviour and instead always forces to pick the latest version between embedded and stuff in the current --extra-search-dir.

@pfmoore
Copy link
Member

pfmoore commented May 19, 2020

Hmm, I think I see the confusion. From the discussion on Discourse the current behaviour was described as:

20 <= virtualenv tried to strike a middle ground: install the latest version available offline (by default this is the embedded, however, if someone pulls in a new version e.g. via a manual run that has --download flag then in subsequent creations use the newer version).

But the poll in that thread didn't distinguish between "always use embedded" and the behaviour described here as the default. There was actually no option to choose "leave the behaviour as it is"! I see you've just posted here to clarify this. Would it be worth also mentioning it on Discourse?

I didn't spot this, and assumed that a vote for "always install the embedded wheel" meant leave the behaviour unchanged. I;m somewhat concerned now that the discussion as a result didn't reflect the real picture 🙁

I agree that the current behaviour is fairly subtle, but as it offers a way for users to update the default to the latest version (by doing --download on an environment - is that right?) I'm not sure the approach here (which means you have no way to upgrade the default until virtualenv is upgraded) is an improvement - and I'm sure it will be worse in the opinion of people like @dstufft who were arguing for "latest always".

I don't have the time to get sucked into an extended discussion on this, unfortunately. Ultimately I don't have a particularly strong opinion personally, other than preferring fast (i.e., no download) by default.

@gaborbernat
Copy link
Contributor Author

But the poll in that thread didn't distinguish between "always use embedded" and the behaviour described here as the default. There was actually no option to choose "leave the behaviour as it is"! I see you've just posted here to clarify this. Would it be worth also mentioning it on Discourse?

I think we agreed with @dstufft that the current behaviour probably picks the worst of both worlds. It's not reproducible and is very subtle when things change.

I agree that the current behaviour is fairly subtle, but as it offers a way for users to update the default to the latest version (by doing --download on an environment - is that right?)

They probably can do this by doing pip install -U virtualenv... and IMHO if we want to offer the user to upgrade the embedded wheels out of bound, I personally would be more comfortable by going the fully explicit way of providing a --upgrade flag that triggers an update run. This way the user can upgrade embedded wheels but is explicit and not the side effect of another run.

@gaborbernat gaborbernat force-pushed the bundle branch 3 times, most recently from 767a52e to 1004390 Compare May 19, 2020 10:18
This means latest compatible version between embed wheels and the ones
found under the path provided by ``--extra-search-dir``. Report at the
end the installed wheel versions.

Signed-off-by: Bernat Gabor <bgabor8@bloomberg.net>
@gaborbernat gaborbernat force-pushed the bundle branch 2 times, most recently from 176f0cc to 6f0bc05 Compare May 19, 2020 11:38
@pradyunsg
Copy link
Member

I still feel that always using the embedded version is going to make evolving pip/Python Packaging more difficult in the future, as more and more users get the older versions of pip "out of the box" and don't actually do an upgrade (possibly because they won't even see an upgrade notice that pip prints, coz their distro patched that code out 🙂).

Whether being slightly faster is more valuable than that, I'll let y'all decide/pick. I've spent more time on this today than I should've have already.

@pfmoore
Copy link
Member

pfmoore commented May 19, 2020

I have also spent way more time on this than I have available (I am literally typing this while waiting for an app to sign in).

I agree that context is critical here and we don't have it. I will add a number of points, and then stop:

  1. My preference for speed is purely personal, not as a pip maintainer.
  2. As a compromise, this proposal is bound to make no-one happy. That should be expected, and hoping for people to approve of it is probably over-optimistic.
  3. It's possible that having older versions of pip "in the wild" will make it harder for people to adopt new standards. But that doesn't mean we can't develop new standards, or that we have to continue supporting old approaches. But that's a much wider debate that I don't intend to have here.
  4. People can upgrade their default pip at the moment by doing a --download run. This updates the cache, and the latest version in the cache is used from that point on. That behaviour is not (as far as I know) well known, nor is it very clearly described anywhere. We can (and should) publicise it more. Getting people on LTS distros to do the occasional virtualenv --download temp_env (or a dedicated virtualenv --update-seeds) shouldn't be impossible.
  5. At the moment, virtualenv is very actively maintained, and new pip releases are incorporated very quickly. That may not always be the case, and having a means where users can update their default is important. So I'm against App data seeder by default should use bundled only #1833 as it loses that option.

I will also say that the behaviour here, and the extra complexity, is very different from the pre-rewrite behaviour. It probably should have been publicised more as a change. But that's water under the bridge, and not worth arguing over. What I do think is important is that more publicity is done about how virtualenv works now, so that people can make informed decisions about how to configure the new behaviour to suit their needs.

App has logged in, so that's me done on this topic.

@gaborbernat
Copy link
Contributor Author

possibly because they won't even see an upgrade notice that pip prints, coz their distro patched that code out

If the distribution decides to patch out pips warning, it probably also going to take the effort to patch out pips auto-download feature.

Whether being slightly faster is more valuable than that, I'll let y'all decide/pick.

I feel like this is the attitude of everyone. At least that's what I concluded from no one actually taking the time to review my previous PR. If this is the case I'll choose the download off, embedded only. With a follow-up feature that allows the user explicitly request an upgrade of embedded wheels.

  1. My preference for speed is purely personal, not as a pip maintainer.

I've requested your and @pradyunsg opinion as a pip maintainer, not as a person, maybe I should have made it clearer. The fact that your both dismissed the request, I've taken as a sign of neither of you feel strong enough about it to cast an opinion. Paul voted in favour of use embedded, I've done so. Since I've maintained virtualenv only you two ever stepped up for any maintenance type job, so I only consider three of us as active maintainers. Considering this I feel like we the virtualenv active maintainer's group prefers embedded.

  • As a compromise, this proposal is bound to make no-one happy. That should be expected, and hoping for people to approve of it is probably over-optimistic.

I was hoping people can strike a compromise ground. Maybe I was too optimistic. But if people are not willing than I'll pick backed up by a public vote.

4. People can upgrade their default pip at the moment by doing a --download run.

There is a reason why this is not documented: because I find too cryptic to really on. Looking back was a really bad idea from myself of mine to go for allowing --download to upgrade the cache.

4. Getting people on LTS distros to do the occasional virtualenv --download temp_env (or a dedicated virtualenv --update-seeds) shouldn't be impossible.

I consider this a new feature and probably something that would be a follow-up PR for this PR. I'm on board with a virtualenv --upgrade-bundled-distributions.

What I do think is important is that more publicity is done about how virtualenv works now, so that people can make informed decisions about how to configure the new behaviour to suit their needs.

Agreed we should improve this section https://virtualenv.pypa.io/en/latest/user_guide.html#seeders; I consider this a slight extension of what this PR should do.

@pradyunsg
Copy link
Member

patch out pips auto-download

Experience with Ubuntu, Arch Linux and Fedora with virtualenv < 16 suggests otherwise. ;)

request an upgrade of the embedded wheels

I'm a bit less wary of use embedded wheels if we have this ability - at least it'll allow folks locked into older versions to upgrade. I do think it'd be important to work to ensure that distros don't break this functionality.

@dstufft
Copy link
Member

dstufft commented May 19, 2020

I still think that attempting to pin pip version by using virtualenv proxy as a crude is till a nonsense way to pin. This change makes it like, 5% less nonsense, but it's still on the whole nonsense.

Since I've maintained virtualenv only you two ever stepped up for any maintenance type job, so I only consider three of us as active maintainers. Considering this I feel like we the virtualenv active maintainer's group prefers embedded.

This is a concerning attitude to me. We've never attempted to exclude a maintainer's opinion because we felt they weren't "active" enough. Even the times we've removed a previous committers access we've always stressed that it was merely a technical means to reduce attack surface and they're still considered maintainers and only have to ask to get it back.

@gaborbernat
Copy link
Contributor Author

This is a concerning attitude to me. We've never attempted to exclude a maintainer's opinion because we felt they weren't "active" enough.

I've not excluded anyone opinion on this. If I we'd not have a long issue discussion, a long discuss topic and I'd not request a review from all three of you in my PR filled in yesterday.

This change makes it like, 5% less nonsense, but it's still on the whole nonsense.

This tone of yours pushes me very close to burnout on maintaining virtualenv. And starting to deeply regret getting involved. You've routinely dismissed as nonsense both my and @asottile opinion now, which is not very constructive or inclusive if you ask me.

@dstufft
Copy link
Member

dstufft commented May 19, 2020

I've not excluded anyone opinion on this. If I we'd not have a long issue discussion, a long discuss topic and I'd not request a review from all three of you in my PR filled in yesterday.

Then what is the point of that statement? I can't find anyway to read it other than as an implicit way to discount my commit bit on this project and to remove my ability to weigh in on changes. What's the point of saying "well this is what active maintainers want", except to attempt to downplay what people not in your definition of active maintainer want?

This tone of yours pushes me very close to burnout on maintaining virtualenv. And starting to deeply regret getting involved. You've routinely dismissed as nonsense both my and @asottile opinion now, which is not very constructive or inclusive if you ask me.

Honestly I feel I've been pretty constructive during this entire discussion up until that last post which I do apologize for. I've repeatedly attempted to describe, in great detail, why I think always installing from embedded is the wrong approach to take using both in terms of an ecosystem health perspective and in terms of a solving the underlying problem of pinning the versions that is the stated reason for preferring to always install from embedded versions. Along the way, I've shifted from "Restore the previous behavior to always install the absolute latest" to "let's find a way to ensure timely updates so that someone stuck on an old version of virtualenv isn't installing a 5+ year old version of pip" as a middle ground, due to the concerns that have been raised by both you and @asottile about the previous behavior.

If you feel as if any of my statements were arguments against you personally rather than reasoned arguments against some specific technical decision then please point them out to me so I can better refine further discussion to avoid that.

@gaborbernat
Copy link
Contributor Author

gaborbernat commented May 19, 2020

I've burned out discussing this topic. I'm going to step back from this project as a whole for the time being. Feel free to proceed as you feel it adequate. I'd rather spend my free time on stuff that's not tagged as nonsense.

@pfmoore
Copy link
Member

pfmoore commented May 19, 2020

The tone of this conversation has definitely become too confrontational. I'd suggest that we all drop the subject for a while. I'd lock the topic, but as the participants are maintainers (and hence could comment even on a locked topic) that wouldn't help - so I'll ask everyone to respect my intent and step away for a while.

@dstufft
Copy link
Member

dstufft commented May 21, 2020

I've stepped away from this issue for a few days, talked to some folks about it, and thought about it some.

First-- I'd like to apologize to @gaborbernat. The tone, and language I used was not appropriate and not fair. I allowed my personal frustration in both the change of behavior, and the discussion itself bleed through and in that frustration I behaved poorly, and for that I apologize.

Second-- Given my lack of time to dedicate to virtualenv and the fact that virtualenv itself is not a project I'm particularly interested in other than where it intersects with packaging work (my maintainership of it was largely coincidental because it came as part of a pip core developer), I'm going to be dropping my commit bit and unfollowing the repository. Regardless of my feelings on the change at hand's impact on the ecosystem, virtualenv having an active maintainer is important too, and I do not want to be the thing that prevents that.

Third-- I still do believe that ensuring timely updates is the correct thing to do, and I think the polling shows the majority of people agree with that (between middle ground and always update, 56% of the poll respondents were for some form of implicit upgrading versus 44% who wanted it fixed to the embedded version). That being said, as part of the above, I am now disengaging from this conversation as well. Presumably that means that the new status quo remains unless the remaining team decides to pick it up and change it, however I'll accept whatever the remaining team decides to do.

Sorry again for the trouble and pain I've caused. I hope that it does not prevent anyone, particularly @gaborbernat from continuing to contribute to this project.

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

Successfully merging this pull request may close these issues.

Ensure bundled pip does not get too out of date
4 participants