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

pkg_resources is deprecated #119

Open
marcinwrochna opened this issue Mar 7, 2023 · 5 comments
Open

pkg_resources is deprecated #119

marcinwrochna opened this issue Mar 7, 2023 · 5 comments
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed priority: 1 Medium priority task

Comments

@marcinwrochna
Copy link

Bug

src/lightning_utilities/core/imports.py imports pkg_resources, which is deprecated (since at least 2020) in favor of importlib.resources, importlib.metadata, and their backports (importlib_resources, importlib_metadata). See here.

To Reproduce

PYTHONWARNINGS=default::DeprecationWarning python -c 'import torchmetrics'

(or import lightning_utilities)

@marcinwrochna marcinwrochna added bug Something isn't working help wanted Extra attention is needed labels Mar 7, 2023
@akihironitta
Copy link
Contributor

@marcinwrochna Thanks for reporting this issue! Feel free to send a PR if you're interested :)

@Borda
Copy link
Member

Borda commented Mar 16, 2023

@marcinwrochna Thanks for reporting this issue! Feel free to send a PR if you're interested :)

Yeas that would be a nice and valuable contribution if you have a time to do it :)

@Borda Borda added the good first issue Good for newcomers label Mar 16, 2023
@marcinwrochna
Copy link
Author

marcinwrochna commented Mar 25, 2023

Sorry, this requires some understanding of all the requirements in lightning projects and of python packaging in general – I have neither, so I'll just say what I found by looking into it.

Three files currently use pkg_resources. In all cases, they cannot be directly replaced with importlib, because there is no parse_requirements, Requirements, nor require() there. I believe the intention here is that one shouldn't try to reimplement distribution-package requirement checks outside of pip.

  1. Your setup.py uses it, as far as I understand, only to check that requirement strings would be parsed correctly (or maybe to normalize them?). It can be left as is, because setup.py won't be imported by users. But it would probably be better to avoid arbitrary code in package requirement definitions, and instead just test whether the plaintext requirements work as expected (at least with pip install). This makes requirements discoverability possible, and doesn't assume that pip parses it exactly like your version of the deprecated pkg_resources package does. Alternatively, you could probably use packaging.requirements instead, as you already do in other places.

  2. You use it in load_requirements in src/lightning_utilities/install/requirements.py. But a) I don't know why all these workarounds are needed, maybe the requirement strings could be fixed instead, maybe it's because you use pkg_resources.parse_requirements() in the first place and maybe it doesn't match with pip's behaviour (see 1.)? Moreover, b) you never use that definition? Instead you use another implementation in https://github.com/Lightning-AI/lightning/blob/master/src/lightning/app/utilities/packaging/build_config.py and yet another in https://github.com/Lightning-AI/lightning/blob/master/.actions/assistant.py . I'm confused.

  3. You use it in src/lightning_utilities/core/imports.py for compare_version and RequirementCache. The usage in compare_version can easily be replaced with importlib. RequirementCache however uses require(), which isn't available elsewhere. But do you really need it? It's unclear even whether RequirementCache is supposed to check distribution packages or import packages. It seems you only use it to check simple name>=version requirements (not the various complicated requirement strings) and also only in situations where you'd import the module anyway. Maybe you could just try-import the appropriate module and check it's __version__? It seems you wanted to give a hint like "Use pip install foo", but you could just provide that hint as an additional argument in situations where the distribution package name isn't obvious? By the way, maybe instead of storing RequirementCache instances as global variables you should make it a function decorated with @lru_cache.

@Borda Borda changed the title pkg_resources is deprecated pkg_resources is deprecated Mar 28, 2023
@Borda
Copy link
Member

Borda commented Mar 28, 2023

I believe the intention here is that one shouldn't try to reimplement distribution-package requirement checks outside of pip

I found this quite calling as setup.py is called when installing on a blank environment, so it may need to somehow wrap the build class and so have local import of the package inside 🐿️

I would see two steps:

  1. replace pkg_resources within this package
  2. find a pattern/way how to use these functions in another setup on blank env.

@Borda
Copy link
Member

Borda commented Oct 7, 2023

Other listed issues - Lightning-AI/pytorch-lightning#16756

@Borda Borda added the priority: 1 Medium priority task label Oct 7, 2023
@stale stale bot added the won't fix label Mar 17, 2024
@Lightning-AI Lightning-AI deleted a comment from stale bot Mar 17, 2024
@stale stale bot removed the won't fix label Mar 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed priority: 1 Medium priority task
Projects
None yet
Development

No branches or pull requests

3 participants