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

Define null values for enums #6095

Closed
ifsheldon opened this issue Feb 20, 2021 · 3 comments
Closed

Define null values for enums #6095

ifsheldon opened this issue Feb 20, 2021 · 3 comments
Labels
feature Is an improvement or enhancement good first issue Good for newcomers help wanted Open to be worked on let's do it! approved to implement refactor won't fix This will not be worked on

Comments

@ifsheldon
Copy link
Contributor

🚀 Feature

Create null values for enums instead of using None. For example, utilities.enums.DistributedType should have a DistributedTypes.None enum value in addition to existing enum values.

Motivation

Python builtin None is hard to track, and I think it is always a good practice to have default null values for enums in compensation for the lack of null safety of Python. It is especially beneficial when a variable holding a type of enum will be None, which is the case for DistributedType in trainer.connectors.accelerator_connector.

Pitch

Define null values for enums, we can:

  • keep track of when a variable of a enum type will be None, facilitating code analysis
  • remind code collaborators of the possibility of being None when using an enum type
  • (If we practice this broadly, we can at least) ensure code collaborators that a variable holding a value of a enum type will always valid(i.e. not literally None)
  • "prevent" Null pointer exception/ None does not have method/attribute error.

Additional context

When fixing #5966 in #5970, I have to take special care of None of self._distrib_type, which is a bit annoying.

@ifsheldon ifsheldon added feature Is an improvement or enhancement help wanted Open to be worked on labels Feb 20, 2021
@Borda Borda added let's do it! approved to implement good first issue Good for newcomers labels Feb 20, 2021
@Borda Borda added this to the 1.3 milestone Feb 20, 2021
@carmocca
Copy link
Contributor

I personally don't think this is worth it. It's just moving the problem from one place to another. Also makes the Optional type unintuitive.

Can you showcase some pieces of code who would greatly benefit from this?

@ifsheldon
Copy link
Contributor Author

ifsheldon commented Feb 21, 2021

It is not just about Optional. You can see some code like(e.g. in accelerator_connector.py)

def __init__(self):
    self.attribute = None #initialize
    if condition:
        self.attribute = Enum.A
    else:
        self.attribute = Enum.B
    # many code 
    if condition:
        self.attribute = None
    # many code
    if self.attribute is not None:
         # do something

In this situation, Optional will not help at all. The points really matter are those I mentioned in the pitch, especially the first and the second points.
For the first point, if you have defined SomeEnum.None, you can, through static code analysis, find out where it is used while you cannot just find None since any variable can be None without any context. SomeEnum.None can give more context when code contributors implement the enum and use the enum.
And it's cheap if you are not intended to fully review the legacy code. You just add another value to an enum. That's it.

@stale
Copy link

stale bot commented Jun 8, 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 Jun 8, 2021
@stale stale bot closed this as completed Jun 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature Is an improvement or enhancement good first issue Good for newcomers help wanted Open to be worked on let's do it! approved to implement refactor won't fix This will not be worked on
Projects
None yet
Development

No branches or pull requests

4 participants