search

python / importlib_metadata

Library to access metadata for Python packages
https://importlib-metadata.readthedocs.io
Apache License 2.0
128 stars 82 forks source link

Consider adding back a warning instead of KeyError #497

Closed hmaarrfk closed 3 months ago

hmaarrfk commented 4 months ago

I'm not sure where best to post this, but I hit the KeyError in the wild on some projects that are quite good at keeping up with python best practices.

I think it is because the previous warning was not something that would be easily triggered by the CIs:

# Do not remove prior to 2024-01-01 or Python 3.14
_warn = functools.partial(
    warnings.warn,
    "Implicit None on return values is deprecated and will raise KeyErrors.",
    DeprecationWarning,
    stacklevel=pypy_partial(2),
)

https://github.com/python/importlib_metadata/commit/a970a491b56a3bf529821ce21867af4573cf2e0d

Ultimately, I believe this warning gets triggered when somebody uses importlib_metadata to crawl through the user's installed directories. This means that it is prone to producing more errors on the users installation and not the "perfectly crafted CI environments". Whats worse (I think) is that this might catch some namespace packages that are left-over from bad installation procedures.

If I would have caught this warning (as an advanced user) I would have reported it to the impacted project https://github.com/cupy/cupy/issues/8440 however, since as a user I would have very little chance of seeing this since it is hidden behind a DeprecationWarning, I was unable to warn the project downstream to importlib_metadata

Would it be acceptable to add back the warning but to elevate it to a UserWarning?

jaraco commented 3 months ago

I can see what you're saying, how the deprecation is invisible except in CI environments or when running under python -W default. Unfortunately, what you're asking is to reset the clock, to go back two years and carry the undesirable behavior for two years and then once again enact the effort to remove the undesirable behavior. Moreover, this undesirable behavior (null result for missing keys) was never intentionally introduced, but was an incidental behavior of the email.message.Message class.

Then again, maybe it doesn't have to be a two-year ordeal. Maybe the warning can be re-introduced as a UserWarning as a bug fix (for the warning not being visible).

Still, what you're proposing is a lot of work, so I don't want to consider it unless it's going to have substantial benefit for the downstream users. Given that users already get:

This leads me to think that although the planned approach failed to fully surface the transition to users in 2023, the presence of the change in importlib_metadata does provide ample control for downstream consumers to manage the transition in these cases where the warnings were not visible.

Moreover, the fact that this project is synced to importlib.metadata in the stdlib magnifies the cost of any change.

My instinct (hope) is that there are a small number of users affected by the issue that fundamentally need to adjust their expectations anyway that there's not a lot of value in adding the UserWarnings. After all, a UserWarning may itself be dangerous as it may surface for users and not developers, still requiring rapid intervention for the affected projects/libraries.

Furthermore, this proposal suggests that DeprecationWarning is not a suitable warning for deprecating undesirable behavior. Unless there's a methodology that this project should have followed to avoid this pitfall, my understanding is that the best practice is to use DeprecationWarning to signal the deprecated behavior. Given the substantial cost of these transitions, it cannot be the case that one can only discover that a UserWarning was needed where a DeprecationWarning was chosen and reset the clock after the fact.

I realize these errors came at a surprise, requiring essentially emergency intervention. That was not the intention, but I'm reluctant to roll back the transition unless it's going to avoid that surprise for a substantial number of consumers. Do you have reason to think the issue encountered in cupy is likely to occur in a more widespread way?

hmaarrfk commented 3 months ago

What surprised me the most was that cupy in the offending line was using python 3.10 and importlib.metadata and not importlib_metadata

Do you have reason to think the issue encountered in cupy is likely to occur in a more widespread way?

I’m not sure I have anything to add beyond my original statement. With that. Let’s close this and if somebody else comes across it they can comment here for you to reconsider.

Thank you for your time!!!