Skip to content
This repository has been archived by the owner on Oct 9, 2023. It is now read-only.

Integrate lightning_utilities #1452

Closed
carmocca opened this issue Sep 6, 2022 · 10 comments · Fixed by #1457
Closed

Integrate lightning_utilities #1452

carmocca opened this issue Sep 6, 2022 · 10 comments · Fixed by #1457
Labels
good first issue Good for newcomers help wanted Extra attention is needed refactors & code health

Comments

@carmocca
Copy link
Contributor

carmocca commented Sep 6, 2022

🚀 Feature

We have released https://github.com/Lightning-AI/utilities/ which contains multiple utilities that are shared across the codebase.

Motivation

Avoid protected imports
Avoid duplicated code

Pitch

Go through the Flash codebase and see which duplications or protected imports we have

@carmocca carmocca added enhancement New feature or request help wanted Extra attention is needed labels Sep 6, 2022
@carmocca
Copy link
Contributor Author

carmocca commented Sep 6, 2022

@carmocca carmocca added refactors & code health and removed enhancement New feature or request labels Sep 6, 2022
@krshrimali
Copy link
Contributor

Hi, @carmocca - Thank you! I am up for it, and it's only going to help us stay stable with respect to these utilities.

Is it possible that we can create a list of the sections of code that we can change, and take some help from our awesome community?

@carmocca carmocca added the good first issue Good for newcomers label Sep 8, 2022
@carmocca
Copy link
Contributor Author

carmocca commented Sep 8, 2022

The two I pointed out in my previous message are the only ones I could find. These are the ones we currently have in the utilities: https://github.com/Lightning-AI/utilities/tree/main/src/lightning_utilities/core

Should be a very easy PR, up for grabs!

@krshrimali
Copy link
Contributor

The two I pointed out in my previous message are the only ones I could find. These are the ones we currently have in the utilities: https://github.com/Lightning-AI/utilities/tree/main/src/lightning_utilities/core

Should be a very easy PR, up for grabs!

Thanks! @uakarsh - Maybe you'll like to pick these up? Please let us know 🎉

@carmocca
Copy link
Contributor Author

carmocca commented Sep 8, 2022

For examples of the PR in lightning: Lightning-AI/pytorch-lightning#14475, and Lightning-AI/pytorch-lightning#14537

@uakarsh
Copy link
Contributor

uakarsh commented Sep 8, 2022

Hi @carmocca @krshrimali, this definitely looks interesting. Can you give me some time, to explore about lightning_utility, and the existing codebase? I would surely get back to you.

@krshrimali
Copy link
Contributor

Hi @carmocca @krshrimali, this definitely looks interesting. Can you give me some time, to explore about lightning_utility, and the existing codebase? I would surely get back to you.

Oh definitely, please take your time and let us know if you need any help! Thank you for your help! ❤️

@uakarsh
Copy link
Contributor

uakarsh commented Sep 15, 2022

HI, @krshrimali @carmocca, I would now start working on this task. So, far I have been going through the lightning_utilities repo, and what I could understand is that the repo's main idea is to centralize the steps involved before starting the code, i.e module availability, the checking the data structures required for different methods, and converting them to some other data structure. Although there were many other steps involved, these were something, I was able to understand.

So, my current plan is to g through the entire codebase of lightning flash, and replace all the methods that come under lightning_utilities with them.

Anything, which I missed or should be included while modifying the codebase?

@carmocca
Copy link
Contributor Author

Nothing missed. That's perfect :)

@uakarsh
Copy link
Contributor

uakarsh commented Sep 15, 2022

Cool, would be starting today

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
good first issue Good for newcomers help wanted Extra attention is needed refactors & code health
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants