Only write lock file when installation is success

[wagnerluis1982]

Pull Request Check List

Resolves: #395

• 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 ("Writing lock file" displays 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 ("Writing lock file" is not displayed)

$ 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)"'.

[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]

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]

Reason: some tests were failing due to the lack of this property (that exists on parent Locker)

[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 🙂

[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 😶

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.

[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 #7498 (comment) comes from.

[wagnerluis1982]

Thank, @radoering, I got the idea now.

But this is getting weirder, I added the wheel json files as instructed and the test produced a very confused output.

Notice it was logged "Installing setuptools (39.2.0)", but I have a dependency on setuptools??? It makes no sense.

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 setuptools (>=40.8.0) which doesn't match any versions, version solving failed.

[wagnerluis1982]

I also made a different investigation by switching off my network and noticed this test is requiring network to work. See below.

Was that supposed to be???

Updating dependencies
Resolving dependencies...

Writing lock file

Package operations: 7 installs, 0 updates, 0 removals

  • Installing six (1.11.0)

  gaierror

  [Errno -3] Temporary failure in name resolution

  at /usr/lib/python3.8/socket.py:918 in getaddrinfo
      914│     """
      915│     # We override this function since we want to translate the numeric family
      916│     # and socket type values to enum constants.
      917│     addrlist = []
    → 918│     for res in _socket.getaddrinfo(host, port, family, type, proto, flags):
      919│         af, socktype, proto, canonname, sa = res
      920│         addrlist.append((_intenum_converter(af, AddressFamily),
      921│                          _intenum_converter(socktype, SocketKind),
      922│                          proto, canonname, sa))

The following error occurred when trying to handle this error:


  NewConnectionError

  <urllib3.connection.HTTPSConnection object at 0x7f973c75afd0>: Failed to establish a new connection: [Errno -3] Temporary failure in name resolution

  at ~/.cache/pypoetry/virtualenvs/poetry-Iqs9p7vq-py3.8/lib/python3.8/site-packages/urllib3/connection.py:186 in _new_conn
      182│                 % (self.host, self.timeout),
      183│             )
      184│ 
      185│         except SocketError as e:
    → 186│             raise NewConnectionError(
      187│                 self, "Failed to establish a new connection: %s" % e
      188│             )
      189│ 
      190│         return conn

The following error occurred when trying to handle this error:


  MaxRetryError

  HTTPSConnectionPool(host='files.pythonhosted.org', port=443): Max retries exceeded with url: /packages/67/4b/141a581104b1f6397bfa78ac9d43d8ad29a7ca43ea90a2d863fe3056e86a/six-1.11.0-py2.py3-none-any.whl (Caused by NewConnectionError('<urllib3.connection.HTTPSConnection object at 0x7f973c75afd0>: Failed to establish a new connection: [Errno -3] Temporary failure in name resolution'))

  at ~/.cache/pypoetry/virtualenvs/poetry-Iqs9p7vq-py3.8/lib/python3.8/site-packages/urllib3/util/retry.py:592 in increment
      588│             history=history,
      589│         )
      590│ 
      591│         if new_retry.is_exhausted():
    → 592│             raise MaxRetryError(_pool, url, error or ResponseError(cause))
      593│ 
      594│         log.debug("Incremented Retry for (url='%s'): %r", url, new_retry)
      595│ 
      596│         return new_retry

The following error occurred when trying to handle this error:


  ConnectionError

  HTTPSConnectionPool(host='files.pythonhosted.org', port=443): Max retries exceeded with url: /packages/67/4b/141a581104b1f6397bfa78ac9d43d8ad29a7ca43ea90a2d863fe3056e86a/six-1.11.0-py2.py3-none-any.whl (Caused by NewConnectionError('<urllib3.connection.HTTPSConnection object at 0x7f973c75afd0>: Failed to establish a new connection: [Errno -3] Temporary failure in name resolution'))

  at ~/.cache/pypoetry/virtualenvs/poetry-Iqs9p7vq-py3.8/lib/python3.8/site-packages/requests/adapters.py:565 in send
      561│             if isinstance(e.reason, _SSLError):
      562│                 # This branch is for urllib3 v1.22 and later.
      563│                 raise SSLError(e, request=request)
      564│ 
    → 565│             raise ConnectionError(e, request=request)
      566│ 
      567│         except ClosedPoolError as e:
      568│             raise ConnectionError(e, request=request)
      569│

[wagnerluis1982]

I didn't include the wheel files. They are in another branch that I am experimenting.

• https://github.com/wagnerluis1982/poetry/blob/613fda0a9f61c4d0260158077dd6a6fba0dc9f1d/tests/repositories/fixtures/pypi.org/json/wheel.json
• https://github.com/wagnerluis1982/poetry/blob/613fda0a9f61c4d0260158077dd6a6fba0dc9f1d/tests/repositories/fixtures/pypi.org/json/wheel/0.31.0.json

[dimbleby]

I have a dependency on setuptools??? It makes no sense.

like the earlier dependency on wheel, this will be a default build requirement for a package that needs building and that does not specify its build requirements.

certainly it's undesirable for these tests to rely on having an internet connection

[wagnerluis1982]

like the earlier dependency on wheel, this will be a default build requirement for a package that needs building and that does not specify its build requirements.

I will take your word.

Still, don't you think that's weird the solver to show to the user

  • Installing setuptools (39.2.0)

and in the end to display

  SolveFailure

  Because -root- depends on setuptools (>=40.8.0) which doesn't match any versions, version solving failed.

[dimbleby]

pretty sure it's just an unlucky coincidence that this testcase happens to pick up a dependency on setuptools 39.2.0, while some other dependency has (by default) a build-time dependency on setuptools 40.8.0.

Agree that it's not completely obvious that this is what is happening, if you haven't got enough to fix in this MR already feel free also to make the messages better!

[radoering]

For example, we should replace -root- with the actual name of the package we are trying to build if possible. Originally, I thought this comes from

poetry/src/poetry/installation/chef.py

Line 69 in 40061f9

package = ProjectPackage("__root__", "0.0.0")

but it's spelled differently so it may be unrelated...

[dimbleby]

it definitely comes from there, -root- is __root__ after normalisation

[dimbleby]

@wagnerluis1982 if you're still interested in progressing this then it's going to be worth rebasing on master, the testcase that was giving you trouble is fixed as at #7759

[wagnerluis1982]

@dimbleby thanks. I'm still interested. Will do the rebase ASAP.

[github-actions]

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.