pip exception parsing `last_check` datetime

[belm0]

Description

pip wheel fails when parsing last_check due to lack of Z suffix support by datetime.fromisoformat().

It looks like the fromisoformat() call was added recently, and indeed there was concern in the PR about handling Z. @pfmoore @pgan

Expected behavior

pip install does not having an error parsing last_check

pip version

23.3.dev0

Python version

3.8

OS

linux

How to Reproduce

1. git clone https://github.com/pypa/pip.git
2. cd pip
3. python3.8 -m venv venv
4. source venv/bin/activate
5. pip install -e .
6. pip wheel -vv scipy==1.10.1

Output

WARNING: There was an error checking the latest version of pip.
See below for error
Traceback (most recent call last):
  File "/Users/john/tmp/pip/src/pip/_internal/self_outdated_check.py", line 227, in pip_self_version_check
    upgrade_prompt = _self_version_check_logic(
  File "/Users/john/tmp/pip/src/pip/_internal/self_outdated_check.py", line 188, in _self_version_check_logic
    remote_version_str = state.get(current_time)
  File "/Users/john/tmp/pip/src/pip/_internal/self_outdated_check.py", line 76, in get
    last_check = datetime.datetime.fromisoformat(self._state["last_check"])
ValueError: Invalid isoformat string: '2023-10-11T17:07:57Z'

(aside: an error should be logged as ERROR, not WARNING)

Code of Conduct

• I agree to follow the PSF Code of Conduct.

[belm0]

my workaround

last_check = datetime.datetime.fromisoformat(self._state["last_check"].replace('Z', '+00:00'))

[pfmoore]

@pganssle any thoughts?

[pganssle]

The problem to me seems to be that somehow this is bubbling up to become a user-visible exception? In the original PR, it seems I was under the impression that if it failed to parse the old state file, it would raise a warning to that effect but otherwise it wouldn't effect anything (I don't remember what these state files do, but I'm guessing they are some kind of cache, and this was just going to cause one cache miss the first time you run pip after upgrading from the version without .isoformat() to the version with .isoformat(), which seems like a very small cost to pay in exchange for not indefinitely keeping around code to parse both formats.

It seemed like we all agreed that that was the case before the merge, so why is the assumption invalid? Is it that pip wheel isn't properly catching errors somewhere?

[sbidoul]

@pganssle per #12357 it seems the problem surfaces as a warning. So I understand it is not blocking but noisy. Handling parse errors as suggested in #12006 (comment) sounds like a an easy solution (I've not investigated in detail).

[belm0]

it seems the problem surfaces as a warning. So I understand it is not blocking but noisy.

It's an exception that stops the build. See repro steps where pip wheel ... fails.

[sbidoul]

Ah, ok, thanks @belm0. Then we'll need a patch release.

[pganssle]

If it's an exception, I think the issue is that you are not catching it, because it's supposed to be a warning.

If it's a warning, then I don't have any strong opinion about this. If you consider it valuable information for users to have that the state information is being discarded because it was created with a version of pip that is too old, then I would say keep warning about it, otherwise just silently swallow the parse error and consider it a cache miss or whatever.

[notatallshaw]

Also I'm fairly sure that a secondary issue with the current state, is users needing to downgrade Pip are stuck figuring out how to delete this version cache, with it probably not being obvious to most users that would fix it.

Today I upgraded Pip in my work environment, discovered an error, and needed to downgrade Pip to understand if Pip was the issue or not. Then I got the error message about Pip not being able to check for new versions.

Would this not also affect pip wheel?

[pfmoore]

I think that this comment where I said "So we can simply return None (saying that we need to refresh the state) if we hit a parse error" was never addressed, because it looks like we never did change the code to return None in that case.

Although honestly, I don't see why we shouldn't replace Z with +00:00 as suggested above. That seems perfectly reasonable to me.