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

extend Enum api #5478

Closed
3 tasks done
Borda opened this issue Jan 12, 2021 · 6 comments
Closed
3 tasks done

extend Enum api #5478

Borda opened this issue Jan 12, 2021 · 6 comments
Labels
discussion In a discussion stage good first issue Good for newcomers help wanted Open to be worked on refactor
Milestone

Comments

@Borda
Copy link
Member

Borda commented Jan 12, 2021

🚀 Feature

it may be nice to have some general calls over our enums such as Distributed mentioned by @carmocca in #5300 (comment)

Any logic to compare different DistributedTypes should be encapsulated by the enum itself.

Motivation

easier handling some cumulative behaviours, such is_ddp which cover DDP and DDP_SPAWN

Pitch

class DistributedType(LightningEnum):
    DP = 'dp'
    ...

    def is_distributed(self):
        return self in (DistributedType.DDP, DistributedType.DDP_SPAWN, DistributedType.DDP2)
@Borda Borda added feature Is an improvement or enhancement help wanted Open to be worked on design Includes a design discussion labels Jan 12, 2021
@Borda Borda added this to the 1.2 milestone Jan 12, 2021
@edenlightning edenlightning modified the milestones: 1.2, 1.3 Feb 8, 2021
@edenlightning edenlightning added refactor and removed design Includes a design discussion labels Feb 22, 2021
@edenlightning edenlightning removed this from the 1.3 milestone Feb 22, 2021
@stale stale bot added won't fix This will not be worked on and removed won't fix This will not be worked on labels Mar 25, 2021
@Lightning-AI Lightning-AI deleted a comment from stale bot Mar 25, 2021
@carmocca carmocca added good first issue Good for newcomers won't fix This will not be worked on and removed feature Is an improvement or enhancement help wanted Open to be worked on labels Mar 25, 2021
@stale stale bot removed the won't fix This will not be worked on label Mar 25, 2021
@Borda Borda added this to the 1.3 milestone Mar 25, 2021
@carmocca carmocca added the help wanted Open to be worked on label Mar 25, 2021
@awaelchli
Copy link
Contributor

I would like to do a comment/counter-pitch here.
If we had a strict, finite set of training types the enum with the is_distributed etc. functionality would be a great solution.
However, users can pass in their own plugins and they will not fall into the fixed categories
(DistributedType.DDP, DistributedType.DDP_SPAWN, DistributedType.DDP2)

I suggest that we keep these enum types strictly internal and only use them for argument parsing part where the options are strictly limited.

@carmocca
Copy link
Contributor

Maybe I am missing something but isn't that already a problem? see (random example):

https://github.com/PyTorchLightning/pytorch-lightning/blob/6b990f3fa5646cfbf2e2945daf568a27921b8362/pytorch_lightning/trainer/connectors/accelerator_connector.py#L518

This issue is just about moving those checks from one place to the other (connector to Enum), not if those checks should exist at all

@awaelchli
Copy link
Contributor

Yes, should be fine to move them.
Just wanted to bring awareness of the current limitation

@edenlightning edenlightning removed this from the v1.3 milestone Apr 27, 2021
@edenlightning edenlightning added the discussion In a discussion stage label May 9, 2021
@stale stale bot added the won't fix This will not be worked on label Jun 8, 2021
@carmocca
Copy link
Contributor

carmocca commented Jun 9, 2021

This might not be relevant after #6090, wdyt @kaushikb11 ?

@stale stale bot removed the won't fix This will not be worked on label Jun 9, 2021
@kaushikb11
Copy link
Contributor

Interesting, this is something that could be looked into. Like the idea of having it separate from the Accelerator Connector.

@Lightning-AI Lightning-AI deleted a comment from stale bot Jun 9, 2021
@Borda Borda added this to the v1.4 milestone Jun 9, 2021
@edenlightning edenlightning modified the milestones: v1.4, v1.5 Jul 1, 2021
@awaelchli awaelchli modified the milestones: v1.5, v1.6 Nov 4, 2021
@carmocca
Copy link
Contributor

Closing as we are trying to reduce our dependence on Enums, since they limit customization: #10422

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion In a discussion stage good first issue Good for newcomers help wanted Open to be worked on refactor
Projects
None yet
Development

No branches or pull requests

5 participants