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

Update core/lightning.py to core/lightning_module.py #10499

Closed
rohitgr7 opened this issue Nov 12, 2021 · 11 comments · Fixed by #12740
Closed

Update core/lightning.py to core/lightning_module.py #10499

rohitgr7 opened this issue Nov 12, 2021 · 11 comments · Fixed by #12740
Assignees
Labels
good first issue Good for newcomers lightningmodule pl.LightningModule refactor
Milestone

Comments

@rohitgr7
Copy link
Contributor

rohitgr7 commented Nov 12, 2021

Proposed refactoring or deprecation

Update core/lightning.py to core/lightning_module.py
Possibly also update core/datamodule.py to core/lightning_datamodule.py

Motivation

Update it since core/lightning.py only contains LightningModule so the filename doesn't signify what it contains.

Pitch

It's a simple update. We can deprecate the old filename. Need approvals if it looks good.

Additional context


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 @justusschock @awaelchli @akihironitta @rohitgr7 @carmocca @Borda @ananthsub @ninginthecloud @jjenniferdai

@tchaton
Copy link
Contributor

tchaton commented Nov 15, 2021

Hey @rohitgr7,

I am in favour of this. It would make discoverability of the code simpler.

@ananthsub @awaelchli @carmocca Any thoughts ?

@carmocca
Copy link
Contributor

carmocca commented Nov 15, 2021

Agree with the renaming.

However, it should be core/lightning.py -> core/module.py

because if we choose core/lighting_module.py, the following should also be changed
core/datamodule.py -> core/lightning_datamodule.py (already pointed out in the top post)
lite/lite.py -> lite/lightning_lite.py
utilities/cli.py -> utilities/lightning_cli.py

@rohitgr7
Copy link
Contributor Author

@Borda thoughts?

@Borda
Copy link
Member

Borda commented Nov 15, 2021

Agree with the renaming.

I would say that the lite/lite.py is not very good as well as trainer/trainer.py

thoughts?

I agree with the name, just has module.py only ay be confusing as the same name is in PT, so could be included somehow that this module is for models or its base? aka core/lightning.py -> core/base_module.py

@carmocca
Copy link
Contributor

carmocca commented Nov 15, 2021

I agree with the name, just has module.py only ay be confusing as the same name is in PT, so could be included somehow that this module is for models or its base? aka core/lightning.py -> core/base_module.py

But that's a separate topic, which would be if LightningModule is a good name.
It should only be base_module if LightningModule was renamed to BaseModule.

This issue is just about consistency of the filenames. The current format is class LightningX -> X.py

So either (1):

  • lightning.py is adapted to module.py for consistency with the format (:tada: )
  • the format is changed to class LightningX -> lightning_X.py (:+1: )

@stale
Copy link

stale bot commented Dec 17, 2021

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!

@stale stale bot added the won't fix This will not be worked on label Dec 17, 2021
@rohitgr7 rohitgr7 added the lightningmodule pl.LightningModule label Dec 17, 2021
@stale stale bot removed the won't fix This will not be worked on label Dec 17, 2021
@stale
Copy link

stale bot commented Jan 17, 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!

@stale stale bot added the won't fix This will not be worked on label Jan 17, 2022
@rohitgr7 rohitgr7 removed the won't fix This will not be worked on label Jan 17, 2022
@carmocca carmocca added the good first issue Good for newcomers label Jan 17, 2022
@stale
Copy link

stale bot commented Feb 17, 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!

@stale stale bot added the won't fix This will not be worked on label Feb 17, 2022
@carmocca carmocca added this to the future milestone Feb 17, 2022
@stale stale bot removed the won't fix This will not be worked on label Feb 17, 2022
@tanmoyio
Copy link
Contributor

@rohitgr7 I want to work on it,

@akihironitta
Copy link
Contributor

Hi @tanmoyio! That'll be great! Assigned this issue to you :)

@tanmoyio
Copy link
Contributor

tanmoyio commented Apr 12, 2022

@akihironitta so core/lightning.py -> core/module.py ?
won't it affect the tests ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers lightningmodule pl.LightningModule refactor
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants