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

Breaking change introduced in the legacy Poetry installer #6314

Closed
1 of 3 tasks
adzeeman opened this issue Sep 1, 2022 · 26 comments · Fixed by #6378
Closed
1 of 3 tasks

Breaking change introduced in the legacy Poetry installer #6314

adzeeman opened this issue Sep 1, 2022 · 26 comments · Fixed by #6378
Labels
kind/bug Something isn't working as expected

Comments

@adzeeman
Copy link

adzeeman commented Sep 1, 2022

  • I am on the latest Poetry version.
  • I have searched the issues of this repo and believe that this is not a duplicate.
  • If an exception occurs when executing a command, I executed it again in debug mode (-vvv option).
  • OS version and name:
  • Poetry version:
  • Link of a Gist with the contents of your pyproject.toml file:

Issue

Hi Poetry team. With Poetry 1.2.0, the legacy installer no longer works the same way it had.

We use Poetry in production as part of our CI/CD process, and the breaking change resulted in multiple pipelines failing.

On 31 August, the legacy installer stopped working. The fix was to append the --version 1.1.15 to the installer. In our own time, we will migrate over to 1.2.0 but will need to evaluate that release before doing so.

On 1 September, the installer stopped working again, even with the fix that worked the day before. GET_POETRY_IGNORE_DEPRECATION=1 is now required. We had to update all our pipelines again.

The bug is created as a request that breaking changes are not introduced in the future when attempting to phase out legacy procedures. Instead, please use alternative measures, such as deprecation warnings.

I created this bug since we plan to continue using Poetry but wanted to raise this concern with the development team.

@adzeeman adzeeman added kind/bug Something isn't working as expected status/triage This issue needs to be triaged labels Sep 1, 2022
@neersighted
Copy link
Member

neersighted commented Sep 1, 2022

Hi @adzeeman, sorry about the headache. To give you a little background, get-poetry.py was deprecated with a warning message for over a year (and the replacement was available for 6 months before the deprecation) -- however usage of the script in production with no pinned version was much more common than we thought.

Thus, when 1.2.0 was released, the old installer would break as it would select the latest stable version for install, and then realize that 1.2.0 wasn't installable. As a compromise it was suggested that the installer fall back to 1.1.x with loud warnings in that situation -- we did so, but the with addition of a random failure percentage to try and force investigation and migration, since clearly warning messages, blog posts, and release notes had not made those downloading the script from our repo sufficiently aware.

I am open to the idea of considering an explicit --version to be an opt-out of the failure chance, but I hesitate because the eventual (total) removal of this script in several months will likely cause similar heartache. If we get it out of the way now, that may be for the best, even if it causes more pain and confusion now.

This was not meant to happen at the same time as 1.2.0 -- honestly the plan was to get rid of get-poetry.py before the 1.2.0 release, but we simply didn't have the bandwidth and the issue forced itself with the new stable release.

Since the cat is already out of the bag, do you think allowing for --version to silently continue is really any better since we still need to remove this installer? Note that the new installer has long been able to install Poetry 1.1 -- the release branches and installers have no intrinsic ties, beyond 1.2 lacking the build machinery to produce get-poetry.py-compatible archives.

Finally, sorry again about the pain here -- deprecating the get-poetry.py script was thought to have been a solved problem a year ago, though it is clear that the message the script printed for the last 12 months could have been more clear on the nature of the upcoming breakage. Future deprecations of similar tools (e.g. install-poetry.py in this repo) will be informed by this experience.

@ghost

This comment was marked as off-topic.

@neersighted

This comment was marked as off-topic.

@ghost

This comment was marked as off-topic.

@neersighted

This comment was marked as off-topic.

@steve-mavens
Copy link

steve-mavens commented Sep 1, 2022

I think the suggestion was that incompatibility with get-poetry.py constitutes a breaking change in 1.2, and therefore it should have had a major version bump to 2.0. But you're going to say that incompatibility with get-poetry is not a breaking change since get-poetry has been deprecated for over a year! And even so it's not clear to me whether renumbering 1.2 as 2.0 would help at all with get-poetry breaking, since can't it still select "2.0" as the latest version?

Our team only got around to making a general switch from get-poetry to install.python-poetry.org last week, and we still have older code that cannot be installed by poetry 1.1.* (because it doesn't support local dependencies so well), and therefore stuck on poetry 1.0.10, and therefore stuck on get-poetry.py. I think the python ecosystem in general, doesn't support neglect: if you don't actively maintain all your code then it will tend to break for some reason or other, even with pinned dependencies. And once you start making those changes, you need your build to still work, which means more maintenance. poetry is one part of this, but by no means unusual in the context of Python tooling. We don't like it, but we don't have much choice but to do it.

Presumably when get-poetry disappears, to get poetry 1.0.10 we'd have to either fork the installer, use an older version from some other URL, or else pip install poetry?

@jonapich
Copy link
Contributor

jonapich commented Sep 1, 2022

I still believe that my original solution was the proper fix for the "planet":

#6287

I have also spent some hours yesterday morning pinning some missing --version 1.1.15 to the legacy script in order to extinguish the fires. This morning, it started failing again even though I did pin the version (that 5% thing). So the "fix" that was introduced basically made our workaround useless in favor of a new workaround.

It would be better if the script would install the last compatible version (1.1.15) and issue a warning that to obtain 1.2+ you need to use the new installer, and even crash if you pass in --version 1.2.0 saying it's just not the way to do it. That would be the best thing to do, because now, the worst that will happen is that people are going to bypass your deprecation with an env var, which has a good potential to create additional support cases about plugins not working. The worst that should happen, is that you successfully install and use an old version you're used to 🤷🏻‍♂️

Train of thoughts:

  • Deprecations and forced migrations are sometimes necessary, but it's not in this case since the new installer is a new file with a shiny new url. There's nothing to do to stay "backward compatible", except preventing 1.2+ from being installed.
  • The legacy installer could live at its documented URL forever and never be removed from the repository. You can probably even protect the file against changes. This is what we would expect from a "curl installable" script. There's no rush to have everyone move to 1.2, since the 1.1.15 version works amazingly well and will most likely continue to do so for years. Feeling the urge to delete this file feels a bit OCD considering the harm it would cause.
  • The 1.2 announcement mentioned "years of work" to come up to this. This sort of wording makes me think that this should have been released as 2.0. A major bump like that absolutely opens the door for people to have to migrate to the new installer.
  • User experience is always the driving factor / priority. Some people have very complex build setups that takes hours, and also logging isn't always easy to retrieve. It's not very courteous to force a migration when the old version is still working just fine.

@finswimmer
Copy link
Member

Hello @jonapich,

I have also spent some hours yesterday morning pinning some missing --version 1.1.15 to the legacy script in order to extinguish the fires.

may I asked you, why you didn't take the chance to replace the old installer by the new one? You can use the new installer to install 1.1.15 as well.

It would be better if the script would install the last compatible version (1.1.15) and issue a warning that to obtain 1.2+ you need to use the new installer, and even crash if you pass in --version 1.2.0 saying it's just not the way to do it.

As @neersighted wrote we are open for this idea:

I am open to the idea of considering an explicit --version to be an opt-out of the failure chance, but I hesitate because the eventual (total) removal of this script in several months will likely cause similar heartache. If we get it out of the way now, that may be for the best, even if it causes more pain and confusion now.

This may save you some time now, but if we drop the older installer script from the repo in some month, there will be other people, telling us we should have informed them earlier.

The legacy installer could live at its documented URL forever and never be removed from the repository.

We don't want to blow up our code base for legacy reasons over the time. So it's necessary to make a clean up from time to time. You can always use a permalink to the file in a specific commit, e.g. https://github.com/python-poetry/poetry/blob/a613347bf0a9fd7b7ec782cd45222b3a0f0e7401/get-poetry.py , or even put this file in a local repo if you heavily rely on it.

There's no rush to have everyone move to 1.2, since the 1.1.15 version works amazingly well and will most likely continue to do so for years.

As said above, you can use the new installer to install 1.1.15.

The 1.2 announcement mentioned "years of work" to come up to this. This sort of wording makes me think that this should have been released as 2.0.

The version number based on SemVer doesn't tell you anything about the amount of code changes, it is telling you something about the changes in the API. At Poetry we consider CLI as the API and nothing more. The installer script is a stand alone project.

It's not very courteous to force a migration when the old version is still working just fine.

Please let me repeat it once more: You are not forced to a new version of Poetry. You are forced to a new version of the installer only.

fin swimmer

@remram44
Copy link
Contributor

remram44 commented Sep 1, 2022

This is a bit useless after the fact, but I would like to point out that the way you advertised the deprecation of the old installer was not ideal. This is the warning that would be shown up the the day you made it fail on purpose:

This installer is deprecated, and cannot install Poetry 1.2.0a1 or newer.

What I read there is that "this installer is deprecated, what that means is it can't install Poetry 1.2.0". What you meant is that "this installer is deprecated and going away any minute, also incidentally it cannot install Poetry 1.2.0".

Probably a lot of people, like me, were planning to upgrade to the new installer at the same time they upgraded to 1.2.0. All those people now have their pipelines broken because there was not a single day between the release of 1.2.0 and the day the "deprecated" installer was disabled (even if using it to install an older version with the --version flag).

Your "brownout" strategy is not tripping up stragglers who haven't updated the installer despite many warnings. I am a very excited Poetry user who was going to update as fast as possible following 1.2.0's release and was still trip up by this. Please consider revising your deprecation strategy (or having a public strategy...) for future breaking changes, it would be greatly appreciated.

@ssnover
Copy link

ssnover commented Sep 1, 2022

may I asked you, why you didn't take the chance to replace the old installer by the new one? You can use the new installer to install 1.1.15 as well.

I can't speak for others in this thread, but the looming problem on my team is that the install script is used in a docker container build pipeline. We maintain branches for old release versions so that we can build them if we need a patch. All of those branches contain a Docker build which will be invalidated by the removal of the installer. So this move doesn't just force updating our current pipeline, it requires us to update the installer for every single release branch we maintain.

@dimbleby
Copy link
Contributor

dimbleby commented Sep 1, 2022

I offer no view on whether the poetry project could have handled this better, but I want to encourage those of you who are blaming poetry for your CI problems to take at least a glance in the mirror.

Many of us have learned this the hard way so I have some sympathy, but the fact is: if you write your pipelines in such a way that strangers on the internet can break them - then sooner or later that will happen.

If you were relying on get-poetry.py never changing - you should have downloaded it and used your own copy. Count youselves lucky that no-one went rogue and uploaded a version that steals all your secrets and deletes all your code.

(But you shouldn't use a local copy of the script either: because you're getting all your dependencies from a repository that you own and not from public PyPI, right? So the script is useless to you anyway.)

Here endeth the lesson, please resume the complaining...

@remram44
Copy link
Contributor

remram44 commented Sep 1, 2022

There is a world between "you can't expect to be up forever" and "you can't criticize the people taking it down". This doesn't even make sense.

I am not saying that my company or my system is dying because of this. But I don't think that nothing deserves criticism until something or someone actually dies either.

Let's find a way to lighten the impact and make sure future changes are handled better, please.

@dimbleby
Copy link
Contributor

dimbleby commented Sep 1, 2022

You use quote marks very freely. I did not express any of the views that you are disagreeing with.

@jonapich
Copy link
Contributor

jonapich commented Sep 1, 2022

The reason why I did not switch to the new installer is to reduce the scope of changes. When I will be ready to use the new installer, I will take the time to make sure all the ends of our CI/CD pipeline can work with it, then I will be able to roll it out globally.

When I tried the new installer, I was immediately faced with a SSL issue because of an old public image. When I fixed that, I realized GitHub fails at poetry install with 1.2.0 but not with 1.1.15. Why? I don't know; cache maybe? I don't have time to test it at the moment. The old one works perfectly fine for my needs. There's a "chain" there that must work with proofs and we're talking multiple repositories and deployments. It's trivial to do, but very time consuming. This process is usually reserved to clear vulnerabilities, so it's pretty annoying when it's just someone who codes an "if" to block some functionality for no critical reason.

You are right, I was foolish to curl @ master. I would have never done that if it wasn't how the documentation was written in the first place (let's be fair; the documentation recommends the --version, it never talks about not using @master). So I'll just pin the commit before the brownout and call it a day. But now I'm scared about the new installer, and who knows what you're going to do with it in 2 years. I mean, it's already the 3rd installer... I think that proves that there's a problem with the way you deal with backward compatibility.


What I would like to see here is the Poetry team acknowledging that this was poorly handled + mistakes were made, then offer a clean resolution that doesn't require anyone to change or rewrite anything. In a corporate environment, this kind of situation would warrant an internal post-mortem and I really hope you will go through this exercise, as a team.

And while we're at it; you should use SemVer too, and ensure that patches are low risk and security fixes, minors are new ADDED features and deprecation warnings, and majors come with deprecations.

@jonapich
Copy link
Contributor

jonapich commented Sep 1, 2022

The version number based on SemVer doesn't tell you anything about the amount of code changes, it is telling you something about the changes in the API. At Poetry we consider CLI as the API and nothing more.

Is it a problem if a hundred developers are using their own local poetry install while sharing repos? Anyone can lock and commit. Should everyone and all repos uses the same major/minor/patch or if I can expect some consistency? So far, it's working well, but it's all 1.1.x. I am asking because I originally thought that <2 was a reasonable pin, but now I think I need to do <1.2 instead for today, and eventually >1.1,<1.3... What do you recommend?

The installer script is a stand alone project.

To be totally honest with you, that's not something users want to have to deal with.

I don't feel like this kind of awareness is necessary in other CLI tools such as docker, kubectl, aws cli, etc. Either there are no rules, or they are very clearly typed out. Also, their old stuff pretty much always work forever when you follow their docs, it doesn't suddenly break a 6-months old branch. They don't assume that the reader knows about all of the best practices, so they take the time to type everything out if needed, and that's why they are so successful.

The docs really should type out exactly what to do, such as:

POETRY_VERSION=1.2.0  # this is of course generated for the current version of the docs
curl -sSL https://github.com/python-poetry/install/get-poetry.py@some-static-sha | python -

So that there is no ambiguity. (edit: the link in my example is obviously wrong, it's only to illustrate the concept).

This way you know that everyone actually pinned a static version, and can blame other people for not following the docs. That's much better than having to do support only to spell out that not using @master was for us to realize and manage. Currently, you don't know how people decided to pin their versions, and it causes great pain vs the backward compatibility of the installer, it would seem.

In light of the current events, I suggest that the documentation explains that the best thing to do in CI servers is to download the https://install.python-poetry.org script to a local repo and not to curl it like the documentation tells, because it's not possible to pin the new installation script anymore (and you could steal my keys whenever you want!).

@neersighted
Copy link
Member

@adzeeman Discussion has been largely freewheeling but devoid of anything actionable. Thus I'd like input from you on my original question -- do you think that making an explicit --version as an opt-out will keep this from happening again when the old installer is eventually removed (or breaks due to external factors)?

It seems like we've already ripped off the bandaid, however painful and ill-advised that may have been. I am incline to let things stand as they are, unless there's a more graceful path (where graceful includes the future removal of this code) from where we are now that I am missing.

@remram44
Copy link
Contributor

remram44 commented Sep 2, 2022

I think this should definitely be done and the sooner the better, and I don't see any detraction from anyone but project members. Please let us know what we can do to make this happen as soon as possible.

The idea that the deprecation warning that is already printed was ignored, and therefore you had to take this measure, doesn't seem substantiated. We noticed your warning and are taking steps. I don't understand why you imagined you had to take such drastic measures to avoid breaking things when the old installer is removed... especially if it causes just as much breakage sooner.

@neersighted
Copy link
Member

neersighted commented Sep 2, 2022

@remram44 It's still unclear what you mean/want. I'd still like input from @adzeeman, but in the meantime I'm going to suggest the following solution:

  • By default, 1.2.0 will be selected and the installer will fail.
  • Any explicit version can be selected, and the installer will succeed for versions it understands.
  • If GET_POETRY_IGNORE_DEPRECATION=1, the latest installable version will be used even when no explicit version is selected.
  • In all cases a deprecation message will be printed, and warnings will be issued when uninstallable versions of Poetry are overlooked.
  • A target removal date of January 1, 2023 will be printed, with a issue to track the removal, and a blog post linking to that issue.

A brief reminder of how we got here: no arbitrary removal or cut-off for 1.2 was picked, instead (over a year ago) get-poetry.py was declared deprecated in the changelog, script itself, and on the blog. A warning was added for when it tried to install a version it did not understand. When 1.2 was released, the script started selecting 1.2 when it was instructed to install the latest version (i.e. no explicit --version). It was expected that users of the existing script (who had not used it in a CI environment with a pinned/explicit version) would have long migrated to install.python-poetry.org as the deprecation was advertised over a year ago.

The current changes are an attempt to drive awareness of the need to migrate independent from 1.2. I see the above as the best way to move forward from where we are now (given that there is wide-scale awareness of this now, it seems hopeful that January 1 won't be another CI bloodbath).

@jonapich
Copy link
Contributor

jonapich commented Sep 2, 2022

This is reasonable, but we are not taking in account:

  • Old branches / hotfixes / mistakes. To support this, you should get the latest 1.1.x version when not specifying --version. To support it forever, rename master to main and leave master on that old commit forever as a relic of the past 🤷🏻‍♂️ this way you can delete the file from main and feel safe about it.

  • Reducing support calls. In order to force people to move to the new installer, forbid installing 1.2.0+ with the old installer. For instance, it doesn't take much for a developer to include the environment variable bypass, then leaves / moves to another project, and another developer comes in and has no clue that the deprecation variable actually means that 1.2.0's plugins are not working/etc.

@neersighted
Copy link
Member

neersighted commented Sep 2, 2022

I don't think you fully groked my suggestion -- maybe this will make more sense?

This is reasonable, but we are not taking in account:

  • Old branches / hotfixes / mistakes. To support this, you should get the latest 1.1.x version when not specifying --version. To support it forever, rename master to main and leave master on that old commit forever as a relic of the past 🤷🏻‍♂️

You will get the latest 1.1.x version (frozen in time as 1.1.15) when you install with GET_POETRY_IGNORE_DEPRECATION=1. If you don't set the env var, you will get 1.2.0 which will cause a failure.

  • Reducing support calls. In order to force people to move to the new installer, forbid installing 1.2.0+ with the old installer. For instance, it doesn't take much for a developer to include the environment variable bypass, then leaves / moves to another project, and another developer comes in and has no clue that the deprecation variable actually means that 1.2.0's plugins are not working/etc.

1.2.0 cannot be installed with the old installer. When I say "get 1.2.0" I really mean "the installer will select version 1.2.0 as it is the latest, realize it cannot install that version, and fail with a useful error message."

@neersighted
Copy link
Member

Regarding the eventual move of master to main, that is something we intend to do... However to do so cleanly will require doing forge support (aka using Github to set up the redirect), and leaving a 'fake' master around is going to cause issues. I could maybe see a new branch without shared history (which would prevent using the wrong branch for those currently tracking master) -- but the error messages given by the Git client when trying to pull that branch will be confusing.

Regardless, this is not the thread/time to discuss renaming the branch.

@jonapich
Copy link
Contributor

jonapich commented Sep 2, 2022

Hi @neersighted, I did "groke" that correctly, however I am under the impression that you did not fully evaluate the branch situation stated by a comment above: #6314 (comment)

These are old release branches which are sometimes used to provide hotfixes, and thus must contain minimal changes when the need arise. Having to systematically go and adjust an environment variable or pin a version or change the script to make it work is not something that is desired / is not something that plays in your favor for that kind of workflow.

The problem is specifically this part:

If you don't set the env var, you will get 1.2.0 which will cause a failure.

In fact, to be courteous to people who copy/pasted your docs back then, it would be better if they receive the latest 1.1.x when no env var and no --version is specified.

1.2.0 cannot be installed with the old installer.

We agree on this! :)

@neersighted
Copy link
Member

I am very much opposed to installing the latest 1.1.x when no --version is provided without some sort of intervention, as we're just going to repeat this come January 1 (or whatever target we set) -- since anyone who cares has had to intervene in their pipelines (or is tolerating a random but low failure rate), GET_POETRY_IGNORE_DEPRECATION should already be present for users who do not want to pin a version anyway. The cat is already out of the bag there.

@remram44
Copy link
Contributor

remram44 commented Sep 2, 2022

This seems reasonable to me, if you want the latest and the latest can't be installed, it breaks. The current deprecation message stated that it wouldn't be able to install 1.2.0. You will never be able to make it work for old branches of any age anyway.

@adzeeman
Copy link
Author

adzeeman commented Sep 5, 2022

Hi @neersighted. Apologies for only responding now. I did not follow the conversation over the weekend.

By default, 1.2.0 will be selected and the installer will fail.

Seems reasonable. Changing this now to install a different version will be confusing.

Any explicit version can be selected, and the installer will succeed for versions it understands.

The important bit is that it should succeed if a valid version is selected. This means that the GET_POETRY_IGNORE_DEPRECATION isn't required.

If GET_POETRY_IGNORE_DEPRECATION=1, the latest installable version will be used even when no explicit version is selected.

This makes sense. I.e., this flag won't serve any purpose.

In all cases a deprecation message will be printed, and warnings will be issued when uninstallable versions of Poetry are overlooked.
A target removal date of January 1, 2023 will be printed, with a issue to track the removal, and a blog post linking to that issue.

This is a good idea.

@neersighted, thank you for taking a look at this issue and addressing it.

@neersighted
Copy link
Member

neersighted commented Sep 5, 2022

It seems like this is a compromise that works out for everyone, which is encouraging to see -- the road here was bumpy, but I think this is the best we can do for existing users, and the project as a whole.

I am going to lock this issue as any discussion of this deprecation should happen in #6377 -- that is also where progress towards removal will be tracked.

@python-poetry python-poetry locked as resolved and limited conversation to collaborators Sep 5, 2022
@mkniewallner mkniewallner removed the status/triage This issue needs to be triaged label Sep 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
kind/bug Something isn't working as expected
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants