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

Add poetry lock --diff flag to show which packages were changed when locking #9259

Closed
sneakers-the-rat opened this issue Apr 1, 2024 · 14 comments
Labels
kind/feature Feature requests/implementations status/triage This issue needs to be triaged

Comments

@sneakers-the-rat
Copy link
Contributor

sneakers-the-rat commented Apr 1, 2024

Issue Kind

Brand new capability

Description

first of all THANKS to all y'all, really appreciate all the work that has gone into poetry, we use it all the time and love it.

We have made an action to open a pull request every week to keep our lockfile up to date in a more organized way, and as part of that we would like to include in the body of the PR the packages that were updated.

As far as I am aware there is no way to show that as the output of the poetry lock command. Previously we were using poetry install && poetry update which WOULD show the version diff, but after 1.8.0 the poetry install command throws an error anytime the action whose purpose is to update the lockfile would be needed because the lockfile needs to be updated! conundrum!

It would be pretty great to be able to do this and replicate the output of poetry update, which would also save us the CI time of needing to actually install the packages when we don't really need to

$ poetry lock --diff
Updating dependencies
Resolving dependencies... (5.0s)

Package operations: 0 installs, 3 updates, 0 removals

  - Updating filelock (3.13.2 -> 3.13.3)
  - Updating ipykernel (6.29.3 -> 6.29.4)
  - Updating linkml-runtime (1.7.4 -> 1.7.5)

Writing lock file

If the devs think this is an OK idea, i would be happy to write the PR with some guidance on what would be acceptable here.

Impact

Described a bit above, but it seems like a pretty intuitive thing to be able to expect from poetry lock - "when i relock, show me what changed." poetry update already behaves this way. This I think would be most relevant when using poetry in CI, but it also would be useful in general interactive usage, i frequently will lock and then go and check out the git diff to see what changed, which is pretty opaque behavior even if it's not that big of a deal.

Workarounds

We are currently doing this as a workaround:

BEFORE=$(echo "$(poetry show)" | awk '{print $1,$3}')
poetry lock
AFTER=$(echo "$(poetry show)" | awk '{print $1,$3}')
set +e
diff -y --suppress-common-lines -W 80 <(echo "$BEFORE") <(echo "$AFTER")

to get

filelock 3.13.2			      |	filelock 3.13.3
ipykernel 6.29.3		      |	ipykernel 6.29.4
linkml-runtime 1.7.4		      |	linkml-runtime 1.7.5
@sneakers-the-rat sneakers-the-rat added kind/feature Feature requests/implementations status/triage This issue needs to be triaged labels Apr 1, 2024
@dimbleby
Copy link
Contributor

dimbleby commented Apr 1, 2024

after #8737 the poetry install command throws an error anytime the action whose purpose is to update the lockfile would be needed because the lockfile needs to be updated!

I think you have misunderstood something, though I cannot tell what. poetry update does not require you first to run poetry lock eg here is what we get if we do it today on poetry itself:

$ ~/.local/bin/poetry update
Updating dependencies
Resolving dependencies... (5.8s)

Package operations: 0 installs, 22 updates, 0 removals

  - Updating pycparser (2.21 -> 2.22)
  - Updating cryptography (42.0.3 -> 42.0.5)
  - Updating filelock (3.13.1 -> 3.13.3)
  - Updating packaging (23.2 -> 24.0)
  - Updating setuptools (69.1.0 -> 69.2.0)
  - Updating urllib3 (2.2.0 -> 2.2.1)
  - Updating zipp (3.17.0 -> 3.18.1)
  - Updating coverage (7.4.1 -> 7.4.4)
  - Updating identify (2.5.34 -> 2.5.35)
  - Updating importlib-metadata (7.0.1 -> 7.1.0)
  - Updating jaraco-classes (3.3.1 -> 3.4.0)
  - Updating msgpack (1.0.7 -> 1.0.8)
  - Updating pytest (8.0.1 -> 8.1.1)
  - Updating rapidfuzz (3.6.1 -> 3.7.0)
  - Updating typing-extensions (4.9.0 -> 4.10.0)
  - Updating virtualenv (20.25.0 -> 20.25.1)
  - Updating build (1.1.1 -> 1.2.1)
  - Updating mypy (1.8.0 -> 1.9.0)
  - Updating pytest-mock (3.12.0 -> 3.14.0)
  - Updating tomlkit (0.12.3 -> 0.12.4)
  - Updating trove-classifiers (2024.1.31 -> 2024.3.25)
  - Updating types-requests (2.31.0.20240125 -> 2.31.0.20240311)

Writing lock file

I expect your problem is simply that you have allowed pyproject.toml and poetry.lock to drift apart: and your fix should just be to correct that (in a separate commit).

You might want to add apoetry check --lockstep to your pipeline to avoid this mistake in future.

Then I think that both this feature request and most of the work you have done in that other repository are unnecessary

@sneakers-the-rat
Copy link
Contributor Author

sneakers-the-rat commented Apr 1, 2024

poetry update does not require you first to run poetry lock eg here is what we get if we do it today on poetry itself

that is what poetry update shows after the package is already installed. If one calls poetry update before the package has been installed, it just shows all the packages being installed like:

expand/collapse
Creating virtualenv poetry-CqhcaHKN-py3.11 in /Users/jonny/Library/Caches/pypoetry/virtualenvs
Updating dependencies
Resolving dependencies... (2.1s)

Package operations: 59 installs, 1 update, 0 removals

  - Installing certifi (2024.2.2)
  - Installing charset-normalizer (3.3.2)
  - Installing distlib (0.3.8)
  - Installing filelock (3.13.3)
  - Installing idna (3.6)
  - Installing iniconfig (2.0.0)
  - Installing more-itertools (10.2.0)
  - Installing packaging (24.0)
  - Installing platformdirs (4.2.0)
  - Installing pluggy (1.4.0)
  - Installing pycparser (2.22)
  - Updating setuptools (69.1.1 -> 69.2.0)
  - Installing urllib3 (2.2.1)
  - Installing zipp (3.18.1)
  - Installing cffi (1.16.0)
  - Installing cfgv (3.4.0)
  - Installing coverage (7.4.4)
  - Installing crashtest (0.4.1)
  - Installing execnet (2.0.2)
  - Installing identify (2.5.35)
  - Installing importlib-metadata (7.1.0)
  - Installing jaraco-classes (3.4.0)
  - Installing msgpack (1.0.8)
  - Installing mypy-extensions (1.0.0)
  - Installing nodeenv (1.8.0)
  - Installing ordered-set (4.1.0)
  - Installing poetry-core (1.9.0)
  - Installing psutil (5.9.8)
  - Installing ptyprocess (0.7.0)
  - Installing pyproject-hooks (1.0.0)
  - Installing pytest (8.1.1)
  - Installing pyyaml (6.0.1)
  - Installing rapidfuzz (3.7.0)
  - Installing requests (2.31.0)
  - Installing typing-extensions (4.10.0)
  - Installing virtualenv (20.25.1)
  - Installing build (1.2.1)
  - Installing cachecontrol (0.14.0)
  - Installing cleo (2.1.0)
  - Installing deepdiff (6.7.1)
  - Installing dulwich (0.21.7)
  - Installing fastjsonschema (2.19.1)
  - Installing httpretty (1.1.4)
  - Installing installer (0.7.0)
  - Installing keyring (24.3.1)
  - Installing mypy (1.9.0)
  - Installing pexpect (4.9.0)
  - Installing pkginfo (1.10.0)
  - Installing poetry-plugin-export (1.7.1)
  - Installing pre-commit (3.5.0)
  - Installing pytest-cov (4.1.0)
  - Installing pytest-mock (3.14.0)
  - Installing pytest-randomly (3.15.0)
  - Installing pytest-xdist (3.5.0)
  - Installing requests-toolbelt (1.0.0)
  - Installing shellingham (1.5.4)
  - Installing tomlkit (0.12.4)
  - Installing types-requests (2.31.0.20240311)
  - Installing trove-classifiers (2024.3.25)
  - Installing xattr (1.1.0)

The problem is that we can't install the package because of #8737 -- so we have to run poetry lock, but poetry lock doesn't show the diff of the packages that were updated during the lock, hence this issue!

I expect your problem is simply that you have allowed pyproject.toml and poetry.lock to drift apart: and your fix should just be to correct that (in a separate commit).

so, yes. what i'm talking about is how we can't see what was updated in a poetry lock command in a CI action which keeps our lockfile up to date without needing to eg. pollute every PR with a lockfile update, do it manually, etc.

Then I think that both this feature request and most of the work you have done in that other repository are unnecessary

well that's pretty rude! would love to hear how you would do it better. we are trying to consolidate lockfile updates into specific, coordinated PRs from a known user to keep our PRs and git history uncluttered, so if the answer is to relock in every PR that's pretty user hostile behavior when the alternative is add an optional print command for information that seems pretty natural to want when one relocks.

@dimbleby
Copy link
Contributor

dimbleby commented Apr 2, 2024

The problem is that we can't install the package because of #8737

The problem is that you have allowed your pyproject.toml and poetry.lock to drift apart.

The fix is to make a one-time correction - after which the whole premise for this feature request and that other pull request collapses. There is no "conundrum", future updates will work exactly as they always have.

Pointing this out is intended to be helpful rather than rude. You can take a horse to water...

@sneakers-the-rat
Copy link
Contributor Author

The problem is that you have allowed your pyproject.toml and poetry.lock to drift apart.

the weird thing is that they didn't, and the divergence was caused by a combination of a problem that is being discussed elsewhere and other extremely normal things that happen in the course of collaborative development. The merge and lock happened out of order, and it was masked in CI by the cache. We got that. Already resolved. Fully 100% crystal clear on how manually locking resolves that problem this time, as I was when i opened the issue.

That's actually not really relevant at all to the content of this issue though -

after which the whole premise for this feature request and that other pull request collapses

The premise for this feature request is extremely simple: "when poetry does something, can it tell me what it did?"
The premise for my workaround is also quite simple: "we need to work around poetry's technically correct but occasionally brittle behavior" even if that's not all that relevant to the feature request of an expected print statement.

Even in the normal situation, we still want to be able to see what packages were changed when we automatically refresh the lockfile as the docs suggest doing. It would be nice if we didn't have to install the packages every time in order to be able to use the output from poetry update (or error and need to manually relock, or do the workaround that I did) - which was something that seemed like a weird missing feature even before this current bug gave reason to ask why it wasn't implemented.

You can take a horse to water...

are there any other devs that aren't going to talk down to me for asking and offering to write something both simple and expected? I'd even happily take a "no, we don't want to display changes when updating a lockfile because..."

@dimbleby
Copy link
Contributor

dimbleby commented Apr 2, 2024

the weird thing is that they didn't

yes they did, as you can confirm by running poetry check --lock in the linkml repository

@sneakers-the-rat
Copy link
Contributor Author

please read the rest of that sentence. i'm just about done with poetry unless there are any other devs that aren't intent on missing the purpose of the issue, this isn't the first issue where someone is being pointlessly combative.

look allow me to recenter the purpose of the issue:

"When I run the poetry lock command, would it be possible for it to show me what was changed in the lockfile, as is in a similar, but distinct command." A no is fine too.

@dimbleby
Copy link
Contributor

dimbleby commented Apr 2, 2024

I really am trying to help you by getting to the root of your use case

What you wrote when you opened this was that you saw a problem running poetry install and you seemed to believe that this was always going to fail whenever an update was needed ("the poetry install command throws an error anytime the action whose purpose is to update the lockfile would be needed because the lockfile needs to be updated! conundrum!")

This is wrong, and a far simpler way to address the problem that you saw would be to keep your pyproject.toml and poetry.lock consistent all along eg as I previously suggested by adding poetry check --lock to your pipeline

You seem now to be shifting to "this is a good idea anyway". I'm not sure whether I agree or not, if you prefer to fix your pipelines by this more complicated approach then so be it.

@sneakers-the-rat
Copy link
Contributor Author

sneakers-the-rat commented Apr 2, 2024

I'm not in charge of that repo. I don't approve the pull requests. I don't ensure that everyone did everything in the right order. I am just trying to help out. I wanted to write something that would be robust to potential future errors like this. I will pull a check like this. I will also keep the PR that I made because i a) didn't want to have to install the packages every time in the first place because it would waste cycles or cache space, and b) need it any way to get a presentable PR from that action. In order to not have to do that workaround, i proposed something that I thought would be an obvious, near-zero labor, improvement to the lock command.

When a tool requires people to do awkward things to work around its limitations, it could be 100% my fault, user error, I should have obviously known the Correct thing to do before the issue ever happened and it's fundamentally bad for poetry to offer informative feedback on what it's doing. it also could be that there is some shortcoming in the docs or featureset of the tool. it's usually both, and that's why people raise issues - 'hey this isn't working for me can it do something different.' So yes - I may have written the initial issue imprecisely when I said "it will throw an error every time" when i meant "it will throw an error every time this particular situation happens." mea culpa. Please take the note that there is definitely something else wrong here too.

If it's the position of the devs that there being no way to tell what changed during a lock aside from diffing it is intended behavior, great. that's about all i came here to ask and offer help if wanted about.

@dimbleby
Copy link
Contributor

dimbleby commented Apr 2, 2024

the position of the devs

I represent only myself

fwiw I think that the change you propose to make might not be so easy as you expect - try it and prove me wrong! So if I were you I would absolutely be looking for an easier way to solve the problem that I had in front of me.

Another simple approach, if linkml are unwilling or unable to keep the files in their repository consistent, would be to preface the existing action with poetry lock --no-update

@sneakers-the-rat
Copy link
Contributor Author

--- a/poetry/console/commands/lock.py
+++ b/poetry/console/commands/lock.py
@@ -20,6 +20,10 @@ class LockCommand(InstallerCommand):
             " version of <comment>pyproject.toml</>. (<warning>Deprecated</>) Use"
             " <comment>poetry check --lock</> instead.",
         ),
+        option(
+            "show-updates",
+            None,
+            "Show which packages were updated during lockfile generation.")
     ]

     help = """
@@ -50,6 +54,6 @@ file.
             )
             return 1

-        self.installer.lock(update=not self.option("no-update"))
+        self.installer.lock(update=not self.option("no-update"), show_updates=self.option('show-updates'))

         return self.installer.run()


--- a/poetry/installation/installer.py
+++ b/poetry/installation/installer.py
@@ -58,6 +58,7 @@ class Installer:
         self._groups: Iterable[str] | None = None
         self._skip_directory = False
         self._lock = False
+        self._show_updates = False

         self._whitelist: list[NormalizedName] = []

@@ -143,16 +144,24 @@ class Installer:

         return self

-    def lock(self, update: bool = True) -> Installer:
+    def lock(self, update: bool = True, show_updates: bool = False) -> Installer:
         """
         Prepare the installer for locking only.
         """
         self.update(update=update)
+        self.show_updates(show_updates=show_updates)
         self.execute_operations(False)
         self._lock = True

         return self

+    def show_updates(self, show_updates: bool = False):
+        """
+        When locking, show which packages were updated
+        """
+        self._show_updates = show_updates
+        return self
+
     def is_updating(self) -> bool:
         return self._update

@@ -267,8 +276,16 @@ class Installer:
         uninstalls = self._populate_lockfile_repo(lockfile_repo, ops)

         if not self.executor.enabled:
+            if self._show_updates:
+                pre_ops = self._get_operations_from_lock(self._locker.locked_repository())
+
             # If we are only in lock mode, no need to go any further
             self._write_lock_file(lockfile_repo)
+
+            if self._show_updates:
+                post_ops = self._get_operations_from_lock(self._locker.locked_repository())
+                self._do_show_updates(pre_ops, post_ops)
+
             return 0

         if self._groups is not None:
@@ -353,6 +370,28 @@ class Installer:
                 self._io.write_line("")
                 self._io.write_line("<info>Writing lock file</>")

+    def _do_show_updates(self, pre_ops: list[Operation], post_ops: list[Operation]):
+        pre_ops = {op.package.name: op.package for op in pre_ops}
+        post_ops = {op.package.name: op.package for op in post_ops}
+        changes = []
+        for pre_pkg_name, pre_pkg in pre_ops.items():
+            if pre_pkg.name not in post_ops:
+                changes.append(Uninstall(pre_pkg))
+            else:
+                post_pkg = post_ops.pop(pre_pkg.name)
+                if pre_pkg.version != post_pkg.version:
+                    changes.append(Update(initial=pre_pkg, target=post_pkg))
+        for post_pkg in post_ops.values():
+            changes.append(Install(post_pkg))
+
+
+        self._io.write_line("")
+        self.executor._display_summary(changes)
+        for op in changes:
+            message = f"  <fg=blue;options=bold>-</> {self.executor.get_operation_message(op, done=True)}"
+            self._io.write_line(message)
+
     def _execute(self, operations: list[Operation]) -> int:
         return self._executor.execute(operations)

@dimbleby
Copy link
Contributor

dimbleby commented Apr 2, 2024

aiui, _get_operations_from_lock() calculates operations against the installed environment.

So you are not showing changes to the lockfile - you are showing changes that would be made to the environment, if you were to install from the updated lockfile.

I think that is not what you want. (But if it is what you want, good news! we already have poetry install --dry-run)

@radoering
Copy link
Member

I think I understand what you want and I think it makes sense to me. However, there are some pitfalls:

  • poetry lock creates a new lockfile from scratch and ignores an existing lockfile so the information whether something was updated is not available so far. (Only poetry lock --no-update considers an existing lockfile.)
  • You have to consider cases where two versions of a package are locked (for different environments). Only one of both might be updated.

@sneakers-the-rat
Copy link
Contributor Author

sneakers-the-rat commented Apr 2, 2024

aiui, _get_operations_from_lock() calculates operations against the installed environment.

If you'll read the rest of the code, thats fine here - calculating operations before and after updating the lockfile and taking the diff gives you the changes to the lockfile. I tested it without anything installed, with an installation, and with a partial installation. There is probably a better way to do it, but I would rather delete this issue and stop using poetry altogether than have more input from you, thanks.


However, there are some pitfalls:

@radoering THANK YOU for your input.

poetry lock creates a new lockfile from scratch and ignores an existing lockfile so the information whether something was updated is not available so far.

I noticed! And thats totally reasonable. I only poked around a bit, but I think I was able to capture the diff by getting the operations from the lockfile before and after locking - its not very sophisticated and it feels a little expensive, but it was ofc much shorter than the solving step, so maybe thats ok for an optional flag? The operation system is super nice btw, nice work there, good to find a package where things work as expected :)

You have to consider cases where two versions of a package are locked (for different environments). Only one of both might be updated.

Good point!! I had missed that. Id be happy to update my example and take a closer look at the tests to see what other cases I have (certainly) missed and make a more organized PR if this is something yall would consider.

It seems like it might be a good thing in general to take a diff of operations to be performed - it looks like maybe the Transaction object (or another ?) holds a set of operations, so it might be good to generalize this out from just comparing lockfile States to eg. Be able to compare what would be installed on one environment vs another, etc. Ill take a closer read to see what already exists like that and otherwise try and implement.

Thanks for your courteous reply, happy to draft this so its as little work to yall as I can make it.

@sneakers-the-rat
Copy link
Contributor Author

I have decided that following the discussion on this issue I will not try and PR this and am instead transitioning my active projects away from poetry. Thanks to the maintainers who have not been extremely rude to me and others over the years for their work. It might be time to re-read the code of conduct and see if theres anything to be done to improve how these discussions go - as someone who I showed this issue to said: "communities fall to the level of horrible behavior they tolerate."

Thanks again for the work, maybe we'll link up in some kinder place ♥

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Feature requests/implementations status/triage This issue needs to be triaged
Projects
None yet
Development

No branches or pull requests

3 participants