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

Move _*_AVAILABLE checks to their respective files. #11826

Closed
carmocca opened this issue Feb 9, 2022 · 10 comments · Fixed by #18575
Closed

Move _*_AVAILABLE checks to their respective files. #11826

carmocca opened this issue Feb 9, 2022 · 10 comments · Fixed by #18575
Labels
Milestone

Comments

@carmocca
Copy link
Contributor

carmocca commented Feb 9, 2022

Proposed refactor

Move all the variables defined here that are only used in one file
https://github.com/PyTorchLightning/pytorch-lightning/blob/1e36cffbca3f92855d82907370544fade1656da6/pytorch_lightning/utilities/imports.py#L98-L134
to their respective files.

Motivation

This encapsulates code and avoids clutter from the utilities.

Pitch

Move:

https://github.com/Lightning-AI/lightning/blob/50fd12f841bc10ec093e80d0acf3e3b51cd927fe/src/pytorch_lightning/utilities/imports.py#L28-L51

To the files that use them.

The rest I don't think should be moved.


If you enjoy Lightning, check out our other projects! ⚡

  • Metrics: Machine learning metrics for distributed, scalable PyTorch applications.

  • Lite: enables pure PyTorch users to scale their existing code on any kind of device while retaining full control over their own loops and optimization logic.

  • Flash: The fastest way to get a Lightning baseline! A collection of tasks for fast prototyping, baselining, fine-tuning, and solving problems with deep learning.

  • Bolts: Pretrained SOTA Deep Learning models, callbacks, and more for research and production with PyTorch Lightning and PyTorch.

  • Lightning Transformers: Flexible interface for high-performance research using SOTA Transformers leveraging Pytorch Lightning, Transformers, and Hydra.

cc @Borda

@Borda
Copy link
Member

Borda commented Feb 9, 2022

@carmocca could you elaborate more on why this is needed?
I would say having import change in one place makes it easier to find and this imports is a primitive without any dependency so there is no risk of cyclic imports compared to your proposal...

@carmocca
Copy link
Contributor Author

My point is that if one of these primitives is used in only one file, then it should be defined there. For example _JSONARGPARSE_AVAILABLE is only used the CLI:

$ grep -iIRn "_JSONARGPARSE_AVAILABLE" pytorch_lightning/
pytorch_lightning//utilities/__init__.py:44:    _JSONARGPARSE_AVAILABLE,
pytorch_lightning//utilities/imports.py:108:_JSONARGPARSE_AVAILABLE = _package_available("jsonargparse") and _compare_version("jsonargparse", operator.ge, "4.0.0")
pytorch_lightning//utilities/cli.py:32:from pytorch_lightning.utilities.imports import _JSONARGPARSE_AVAILABLE
pytorch_lightning//utilities/cli.py:37:if _JSONARGPARSE_AVAILABLE:
pytorch_lightning//utilities/cli.py:131:        if not _JSONARGPARSE_AVAILABLE

Moving it means we contain all the CLI-related logic into a single place instead of leaving "breadcrumbs" in our utilities. The utilities module is slowly becoming a "dumping ground" for random pieces of code when some of those pieces actually have a logical place to be.

Also, as a naive user, I would first look for this in the LightningCLI code than in a generic utility module.

The same argument can be applied to the other variables mentioned in the top post.

@stale
Copy link

stale bot commented Mar 13, 2022

This issue has been automatically marked as stale because it hasn't had any recent activity. This issue will be closed in 7 days if no further activity occurs. Thank you for your contributions, Pytorch Lightning Team!

@mikcnt
Copy link

mikcnt commented Dec 11, 2022

Hey there! Can I work on this?

@carmocca
Copy link
Contributor Author

Yes of course. You can pick any of these and start chipping away. The linked "Merged" PRs here can be examples if you'd like some guidance

@shenoynikhil
Copy link
Contributor

@carmocca You can strike off _TPU_AVAILABLE I guess from this list. And _XLA_AVAILABLE seems to be fixed too.

@carmocca
Copy link
Contributor Author

@tanaymeh
Copy link
Contributor

Hi @carmocca, can I work on the leftovers?

@carmocca
Copy link
Contributor Author

Yes of course

@tanaymeh tanaymeh removed their assignment Mar 17, 2023
@tanaymeh
Copy link
Contributor

Hi @carmocca, due to some other commitments, I am unfortunately not able to do this at the moment and hence I have unassigned myself.
Apologies!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants