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

Improve Exception Handling #2266

Closed
awaelchli opened this issue Jun 19, 2020 · 6 comments · Fixed by #4859
Closed

Improve Exception Handling #2266

awaelchli opened this issue Jun 19, 2020 · 6 comments · Fixed by #4859
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 priority: 2 Low priority task
Milestone

Comments

@awaelchli
Copy link
Contributor

awaelchli commented Jun 19, 2020

🚀 Code Quality Improvement

I came across this a few times already:

try:
   # something
except Exception:
   # something

It is the worst possible way to handle Exceptions. It is better to catch the specific exception or at least log a message.

Alternatives

None. Sooner or later someone has to deal with this anyway :)

@awaelchli awaelchli added feature Is an improvement or enhancement help wanted Open to be worked on labels Jun 19, 2020
@awaelchli awaelchli self-assigned this Jun 19, 2020
@Borda
Copy link
Member

Borda commented Jun 19, 2020

@awaelchli so what do you propose to have more exact Exceptions like ImportError?

@awaelchli
Copy link
Contributor Author

Yes exactly, the specific exception we try to catch.
Also, sometimes we don't need exception handling at all:

try: 
    x = myobject.attribute
except Exception
    x = 0

can be replaced with
x = getattr(myobject, "attribute", 0)

@awaelchli
Copy link
Contributor Author

awaelchli commented Aug 8, 2020

Another code smell we frequently see (in many places in the code):

try:
    from some_package import some_module
except ImportError:
    SOMEPACKAGE_AVAILABLE = False
else:
    SOMEPACKAGE_AVAILABLE = True

Better:

catch ModuleNotFoundError instead

Even better (and shorter):

import importlib
SOMEPACKAGE_AVAILABLE = importlib.util.find_spec("some_package") is not None

@Borda
Copy link
Member

Borda commented Aug 8, 2020

About the import error, I remember that thare is something twisted about python version and avalanche exceptions, I guess that py3.6 doesn't have the one suggest (need to check)

Also is there a simple way how to import jist a function from a package that you are not sure if it is installed?

@awaelchli
Copy link
Contributor Author

awaelchli commented Aug 9, 2020

Also is there a simple way how to import jist a function from a package that you are not sure if it is installed?

I think the simplest is this:

if importlib.util.find_spec("some_package") is not None:
    from some_package import my_function

because when we import the function in Python it always has to import the whole package anyway.

@stale
Copy link

stale bot commented Oct 25, 2020

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 Oct 25, 2020
@stale stale bot removed the won't fix This will not be worked on label Oct 25, 2020
@Borda Borda added the priority: 2 Low priority task label Nov 19, 2020
@edenlightning edenlightning modified the milestones: 1.1, 1.1.x Nov 30, 2020
@Borda Borda modified the milestones: 1.1.x, 1.1 Dec 4, 2020
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 priority: 2 Low priority task
Projects
None yet
3 participants