Only write lock file when installation is success

[wagnerluis1982]

Pull Request Check List

Resolves: #395

• [x] Added tests for changed code.
• [ ] Updated documentation for changed code.

How it appear to users

The user experience is almost the same, but "Writing lock file" only appears in the end of the process, and only if the installation process succeeds.

On success (message after installation)

$ poetry add jinja2      
Using version ^3.1.2 for jinja2

Updating dependencies
Resolving dependencies... (0.1s)

Package operations: 2 installs, 0 updates, 0 removals

  • Installing markupsafe (2.1.2)
  • Installing jinja2 (3.1.2)

Writing lock file

On failure (no message at all)

$ poetry add pyaudio
Using version ^0.2.13 for pyaudio

Updating dependencies
Resolving dependencies... (0.1s)

Package operations: 1 install, 0 updates, 0 removals

  • Installing pyaudio (0.2.13): Failed

  ChefBuildError

  Backend subprocess exited when trying to invoke build_wheel

  ...bdist logs omitted for brevity...

Note: This error originates from the build backend, and is likely not a problem with poetry but with pyaudio (0.2.13) not supporting PEP 517 builds. You can verify this by running 'pip wheel --use-pep517 "pyaudio (==0.2.13)"'.

[wagnerluis1982]

Summary

This fixes the leaking poetry.lock left changed incorrectly when poetry add installation fails (does not affect when using option --lock).

Code changes

The rollback happens in reaction to Installer.run status. When exit status is not good (i.e. different of zero), it removes the dependencies being added from the relevant objects (DependencyGroup and Locker data) and the installer runs again in lock only mode.

Comments

In order to accomplish, I had to make a few refactorings.

src/poetry/packages/locker.py

In the following, the locker object is "copied" using the constructor whatever the class is. This was done this way to leverage other implementations, e.g. TestLocker for the test cases. The problem here is TestLocker contains a few artificial state (such as _write and _locked) that is needed for my test case.

https://github.com/python-poetry/poetry/blob/7b40c844f523246738850b5ec9537dcf58ac7043/src/poetry/console/commands/add.py#L244-L247

So, I introduced to the Locker a method copy_with with same signature of constructor.

tests/helpers.py

I overrode the new copy_with in the TestLocker, but also copying the artificial state.

tests/console/commands/test_add.py

I added a call to "poetry lock" in the fixture poetry_with_up_to_date_lockfile, in order to the lock file uses the latest syntax of poetry.

[wagnerluis1982]

My investigation towards this bug also raised one question about the validity of one existing test in tests/console/commands/test_add.py.

test_add_keyboard_interrupt_restore_content

In the following, how can the lock be ever changed if Installer.run (which is the one method which changes the lock file) is never even called (it is mocked here). So there is nothing really to be restored.

https://github.com/python-poetry/poetry/blob/8558f0cb94c11d5d1f901900d4a6ea8a8167bd35/tests/console/commands/test_add.py#L2150-L2172

[dimbleby]

#395 was scoped only at not leaving the lock file inconsistent with the pyproject.toml. This goes further in also rolling back changes to the virtual environment.

I'm not sure whether this is a good idea. Though I can see how this might be desirable, it adds complexity and edge cases. (What if the rollback itself fails? What if the user had previously gone pip install some-tool in the environment and the dependencies being removed had overlap with some-tool's dependencies? in that second case, the environment after rollback won't be restored to its original state)

The failed install left the user with no hint that the lockfile might be inconsistent with pyproject.toml. No question that #395 identifies a bug here that wants fixing.

However, the failed install made plenty of logs saying "Installing package-foo" etc and so it should be obvious to a user that their environment has changed. Given the difficulty of reliably restoring exactly the previous environment, it's simpler and safer to let the user decide what to do about that.

Eg maybe they'd actually prefer to keep the 90% of the install that was successful while they fix up some missing libwhatever-dev dependency to allow the rest to succeed.

I'd suggest limiting the scope of this MR to leaving pyproject.toml and poetry.lock in a good state

[wagnerluis1982]

I'd suggest limiting the scope of this MR to leaving pyproject.toml and poetry.lock in a good state

I don't doubt this PR requires some polishing, but this is exactly what I'm doing here, no? In case of installation failure, I'm not removing any package, not really. I'm cleaning up the objects in the scope of poetry add and running installer in lock only mode to keep the lock consistent.

[wagnerluis1982]

Oh, now I realized the source of confusion. Here I'm not in no way making changes to environment.

I mean, AFAIK installer in lock only mode don't change the environment, so I'm relying on it.

[dimbleby]

Oh, perhaps I misread. Still I am surprised that this fix requires any re-locking: I'd expect to arrange things so that the file isn't written (or is explicitly rewritten with a saved copy of the old file) in case of failure - more like the handling of pyproject.toml.

Also maybe cf #7401 - it seems to be uncertain whether anyone likes that enough to merge it as a whole, but the locker.refresh() method there looks like it might be an improvement that could be useful in this one too.

[dimbleby]

What about poetry update? Should that update the lockfile, if installing updated packages fails?

Maybe you already looked into this, perhaps a better place to catch all such examples would be the Installer's _do_install() method. That could defer writing the lockfile until it knew that it had successfully executed operations.

[wagnerluis1982]

What about poetry update? Should that update the lockfile, if installing updated packages fails?

Maybe you already looked into this, perhaps a better place to catch all such examples would be the Installer's _do_install() method. That could defer writing the lockfile until it knew that it had successfully executed operations.

Good points. Excellent points, actually. This will require a further investigation, and probably a more complex change. At least I already have the test 😸

[dimbleby]

looking at your latest draft, I'd definitely suggest that simpler than backing up and restoring the lockfile would be to refactor _do_install() so that the write happens later, and only on success.

do_install() is certainly kinda messy today, if you can find a sensible way to extract something out of it so that it becomes a bit mroe readable that would probably be welcome too!

[wagnerluis1982]

looking at your latest draft, I'd definitely suggest that simpler than backing up and restoring the lockfile would be to refactor _do_install() so that the write happens later, and only on success.

Thank you very much for looking at the draft and for the suggestion.

Lazy as I am, I was avoiding to make any rewrite, but I was definitely starting to think along these lines 😸

do_install() is certainly kinda messy today, if you can find a sensible way to extract something out of it so that it becomes a bit mroe readable that would probably be welcome too!

I have a few ideas I'll try this week.

But do_install mess isn't the only problem here. To work with the Locker is a lot confusing.

[wagnerluis1982]

@dimbleby when you have a chance, check my latest update.

I am applying a delay in the write to lock file by creating an internal Locker wrapper that holds the lock data in memory is signalized that lock data is good to be dumped to file.

[wagnerluis1982]

Remaining to do:

• [x] Add test to test_installer
• [x] Add warning log informing the lock file changes were not applied
• [ ] ~~Think about the refactoring in _do_install~~

I need advice on the warning log as I am not sure what to write :thought_balloon:

[wagnerluis1982]

I was also thinking there is a need to check if lock file is writable. Before my changes, it's known in advance.

Now we install and only in the end we get aware of the issue...

• [x] EDIT: done :slightly_smiling_face:

[wagnerluis1982]

Okay, I feel this is ready for a review.

There are only 2 things I am uncertain:

1. The message "Writing lock file" is now a lie, since the lock is not anymore written at that place, but if moved to the end, it will change the existing output, where "Writing lock file" appears before install the dependencies. Since I am not sure this is desirable, I kept the output as is.
2. Related to the previous, I think we can keep the existing "Writing lock file" and add on the rollback a message "Rolling back lock file". Despite the fact the lock file was never written on installation file, I feel that is a good message to appear.

@dimbleby I am still thinking on a possible refactor on _do_install, but, if I do, I will do in context of a separate PR (that code is currently driving me nut).

[wagnerluis1982]

I just added a "Rolling back lock file" message. Check in the link below to see what you think.

In addition, this is a demo that shows the lock file was not changed :smile:

---

EDIT: updated the demo for an improved version showing poetry.lock content instead of hash.

[dimbleby]

I don't much like it! This adds a pile of code that I don't think ought to be needed.

• If the _TransactionalLocker is a useful API, let's just change the Locker rather than wrapping it up
• But is it needed? can't you just rearrange _do_install() so that the call to write the lockfile happens later (and only on success)?

[wagnerluis1982]

I don't much like it! This adds a pile of code that I don't think ought to be needed.

• If the _TransactionalLocker is a useful API, let's just change the Locker rather than wrapping it up
• But is it needed? can't you just rearrange _do_install() so that the call to write the lockfile happens later (and only on success)?

It is a good question. I tried to rearrange _do_install, but every attempt fails. Because the remaining operations in _do_install (and _execute I think) relies on the updated information provided by locker.lock_data.

Right now the Locker API has no way to write the _lock_data without writing to the file. So, I felt that I should keep the Locker intact (apart from the little refactoring) and created the wrapper.

All in all, I am open to discussions, here or in Discord :slightly_smiling_face:

[wagnerluis1982]

Update: I incorporated the transactional state into Locker.

All existing Locker operations was kept the same behavior. If start_transaction is called, the lock data is not written until commit.

I kept the implementation as simple as possible.

[wagnerluis1982]

I realized that indeed the lock file can be written after installation without any transaction mechanism. I let the failures in my initial investigations misled me, which turned to my current solution. Ashamed :no_mouth:

Now I have a working solution that do that, but there is a side effect in the output: "Writing lock file" message is shifted to the end (see below).

Is this desired???

Current output

Using version ^1.1.2 for foo

Updating dependencies
Resolving dependencies...

Writing lock file

Package operations: 1 install, 0 updates, 0 removals

  - Installing foo (1.1.2)

New output writing lock file in the end

Using version ^1.1.2 for foo

Updating dependencies
Resolving dependencies...

Package operations: 1 install, 0 updates, 0 removals

  - Installing foo (1.1.2)

Writing lock file

[radoering]

IMO the "writing lock file" message should be printed when the lock file is written. If it is written before installation it should be printed before. If it is written after installation it should be printed afterwards.

[wagnerluis1982]

In that case, I will publish the solution postponing lock file writing to the end.

[wagnerluis1982]

Solution published!

Once this gets approved, it will also need update in the website:

This would be here, right?

https://github.com/python-poetry/website/blob/9aab35bcc573f353c81d69e9d92412d0c1133669/layouts/home.html#L65-L80

[wagnerluis1982]

This proper lock file writing caught a test case with a wrong assumption.

I moved back to master and set output on the following (rather than NullIO) and found a solver issue. Since test case isn't checking output, it was missed!

https://github.com/python-poetry/poetry/blob/3e32a4f83e89b2b2bede68e0d375a70931b04a8e/tests/installation/test_installer.py#L1147-L1166

Updating dependencies
Resolving dependencies...

Writing lock file

Package operations: 7 installs, 0 updates, 0 removals

  • Installing six (1.11.0)
  • Installing attrs (17.4.0)
  • Installing more-itertools (4.1.0)
  • Installing pluggy (0.6.0)
  • Installing py (1.5.3)
  • Installing setuptools (39.2.0)

  SolveFailure

  Because -root- depends on wheel (*) which doesn't match any versions, version solving failed.

  at src/poetry/mixology/version_solver.py:349 in _resolve_conflict
      345│             )
      346│             self._log(f'! which is caused by "{most_recent_satisfier.cause}"')
      347│             self._log(f"! thus: {incompatibility}")
      348│ 
    → 349│         raise SolveFailure(incompatibility)
      350│ 
      351│     def _choose_package_version(self) -> str | None:
      352│         """
      353│         Tries to select a version of a required package.

The following error occurred when trying to handle this error:


  SolverProblemError

  Because -root- depends on wheel (*) which doesn't match any versions, version solving failed.

  at src/poetry/puzzle/solver.py:163 in _solve
      159│             packages = result.packages
      160│         except OverrideNeeded as e:
      161│             return self._solve_in_compatibility_mode(e.overrides)
      162│         except SolveFailure as e:
    → 163│             raise SolverProblemError(e)
      164│ 
      165│         combined_nodes = depth_first_search(PackageNode(self._package, packages))
      166│         results = dict(aggregate_package_nodes(nodes) for nodes in combined_nodes)
      167│ 

[wagnerluis1982]

I pasted on last comment, by mistake, an incorrect output (missing "Writing lock file" line), I edited it to be right.

[dimbleby]

sounds like the next step is to figure out why that testcase is hitting a solver failure. Maybe it's right to do so and there's just something about the test setup that's wrong, maybe it's a real bug.

[dimbleby]

pretty sure that -root- comes from https://github.com/python-poetry/poetry/blob/40061f9d1351a9f76d6bdc1db8b3d903864c1373/src/poetry/installation/chef.py#L69, and I expect the dependency on wheel is via the default build requirement for something that is being prepared by that testcase.

[wagnerluis1982]

I traced the failure of test_installer_with_pypi_repository to the PR #7560. The one commit before that PR is working well.

[radoering]

This is because the pool with the mock repository is not generated via create_pool() from the config, but created directly and passed to the installer. Before #7560 this pool wasn't used to create the isolated environment for building dependencies but a new pool was created from the config, which does not contain the mock repository. (In real-world scenarios both pools were created from the same config and thus were equal.) With #7560 the pool is passed from the installer to the chef.

Thus, it's just an issue of the test that #7560 plus your change revealed.

[wagnerluis1982]

Thus, it's just an issue of the test that #7560 plus your change revealed.

Thank you for the explanation.

Now I'm trying to figure out how to fix the test, but so far no relevant progress, I don't understand how the solver operates at all.

[radoering]

I assume you "just" have to add two json files for wheel in https://github.com/python-poetry/poetry/tree/master/tests/repositories/fixtures/pypi.org/json

See https://github.com/python-poetry/poetry/blob/master/tests/repositories/fixtures/pypi.org/json/setuptools.json and https://github.com/python-poetry/poetry/blob/master/tests/repositories/fixtures/pypi.org/json/setuptools/39.2.0.json for example.

That's where the ancient setuptools 39.2.0 in https://github.com/python-poetry/poetry/pull/7498#issuecomment-1450943161 comes from.