Use 'from' to explicitly chain exceptions

[jameslamb]

Current behavior

I wanted to spend a little time contributing this weekend, so I ran pylint over this repo. I see this project has a .pylintrc but it looks like pylint isn't run in CI, so some things do show up.

I found one class of warning from pylint that I think is worth addressing: explicitly re-raising exceptions in try-catches with from.

The newest version of pylint added a check for this. You can see details in "Explicit Exception Chaining". That PEP describes a way to improve stack traces from try-catched exceptions.

without from

try:
    {}["a"]
except KeyError:
    raise RuntimeError("hey this does not exist")

Traceback (most recent call last):
  File "<stdin>", line 2, in <module>
KeyError: 'a'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 4, in <module>
RuntimeError: hey this does not exist

with from

try:
    {}["a"]
except KeyError  as err:
    raise RuntimeError("hey this does not exist") from err

Traceback (most recent call last):
  File "<stdin>", line 2, in <module>
KeyError: 'a'

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "<stdin>", line 4, in <module>
RuntimeError: hey this does not exist

I think this language of "was the direct cause of the following exception" is clearer, and helps you to understand exactly where things started going wrong.

Proposed behavior

Would you be open to a pull request that changes try-catched exceptions in prefect to use this pattern? I wanted to ask first because right now there are 1334 places where that happens so it would touch a lot of code.

you can see them all like this:

pylint src/prefect/ | grep raise-missing-from > pylint.txt

I think this change would improve prefect and help save users some debugging time. Especially users who are not very experienced in Python.

Thanks for your time and consideration.

Notes for Hacktoberfest Contributors

This issue has been left open for Hacktoberfest 2020 contributors. If that describes you, welcome!

1. Leave a comment below claiming a sub-module
2. Identify all cases of this issue in that submodule by running code like the following in a terminal:

   pylint src/prefect/engine/cloud | grep "raise-missing-from"

3. Fix all of these cases.
   • see use explicit exception chaining in client #3320 for an example
4. Open a pull request, following the steps in https://github.com/PrefectHQ/prefect/pulls
   • if there is already a changes/issue3306.yaml, add your name to the list of contributors
   • if there is not already a changes/issue3306.yaml, create one in your pull request, like in use explicit exception chaining in client #3320

sub-modules with this problem

• src/prefect/client - @jameslamb (use explicit exception chaining in client #3320)
• src/prefect/core - @brainless (use explicit exception chaining in core and engine #3383)
• src/prefect/engine - @brainless (use explicit exception chaining in core and engine #3383)
• src/prefect/environments/execution/dask
• src/prefect/environments/execution/k8s
• src/prefect/environments/storage - @shalika10
• src/prefect/tasks/airtable
• src/prefect/tasks/aws - @heyitskevin
• src/prefect/tasks/azure
• src/prefect/tasks/azureml
• src/prefect/tasks/databricks - @rikturr (explicit exception chaining for databricks hook #3448)
• src/prefect/tasks/dbt
• src/prefect/tasks/dropbox
• src/prefect/tasks/gcp - @ericlundy87 (Corrected use from explicitly #3326)
• src/prefect/tasks/great_expectations
• src/prefect/tasks/gsheets
• src/prefect/tasks/kubernetes
• src/prefect/tasks/mysql - @luthrap (luthrap|#3306|Fix mysql module for explicit chaining #3428)
• src/prefect/tasks/postgres - @brunocasarotti
• src/prefect/tasks/redis
• src/prefect/tasks/rss
• src/prefect/tasks/snowflake
• src/prefect/tasks/spacy - @sp1thas
• src/prefect/tasks/template
• src/prefect/tasks/twitter
• src/prefect/utilities - @gabrielcalderon (Added FROM to explicitly chain exceptions in src/prefect/utilities #3429)

NOTE: This issue is not urgent. It's ok to claim a sub-module now and not contribute until Hacktoberfest begins on October 1st.

[joshmeek]

Hey @jameslamb I don't see an issue with making this change! If you want to update all 1334 places go for it haha

[jameslamb]

Thanks! I just had a thought...this is a perfect thing for Hacktoberfest.

What do you think about structuring this issue like I set up microsoft/LightGBM#3390, where Hacktoberfest contributors could come and "claim" this fix for a specific submodule.

This is the the type of change that I think is perfect for Hacktoberfest contributors, some of whom are making their first or fifth or 10th open source contribution.

• it's very self-contained
• it makes the library a tiny bit better, but it's ok to delay it so there's no pressure

The tradeoff in doing that, though, is that if it goes well it would be a lot of PRs that have to be reviewed, which increases the burden on maintainers.

What do you think?

[joshmeek]

That sounds great @jameslamb!

[jameslamb]

ok thanks! I just updated it. I'll also add a PR that hits one of them so people coming here have an example to go off of.

[jameslamb]

ok I just added a sample PR. Thanks for being willing to support this. I know it'll be some extra maintenance work, but the PRs should be quick to review and I think a few people will get their first open source contribution ever from this issue, which is sweet.

Also, could you please add labels good first issue and (if you're open to it) hacktoberfest to this issue? That'll help it get discovered when Hacktoberfest starts.

[joshmeek]

Done! Do you want me to merge that PR that we can link to or are we leaving it open as an active example?

[jameslamb]

yeah I think it should be merged (if it passes tests and you agree with it, of course). I just added a link to it in the description so people will always be able to click over and see the diff, even if it's merged.

[joshmeek]

Awesome sounds good 👍

[rikturr]

I'll take the next one on the list! (and more if others don't volunteer after Hacktoberfest)

• src/prefect/contrib/tasks/databricks

[ericlundy87]

hey can I take src/prefect/tasks/gcp?

[gaby]

I can take src/prefect/utilities 👍🏻

I also can take several others if there's no other volunteers after Hacktoberfest.

[jcrist]

Thanks for opening this @jameslamb! One pattern I've seen a few places that should be special cased is the following:

try:
    do_something()
except Exception as exc:
   self.logger.warning("An exception occurred: %s", exc)
   raise exc

I'm not sure if this is flagged by pylint the same as the above, but try-except blocks where the initial error is re-raised should just use an empty raise rather than calling raise exc.

try:
    do_something()
except Exception as exc:
   self.logger.warning("An exception occurred: %s", exc)
   raise  # <- no need to specify the exception here

[jameslamb]

oh good point! You don't need to worry about "chaining" anything since you're just raising the exact exception that got you into that except block to start with.

I tested and can confirm pylint doesn't flag these. I think it's smart enough to know you're raising a new exception.

mkdir some_nonsense
touch some_nonsense/__init__.py

some_nonsense/trying_stuff.py

"""
some_nonsense/trying_stuff.py
"""

try:
    raise RuntimeError("hello")
except RuntimeError as exc:
    print("you failed this time but keep trying")
    raise

I tried running pylint some_nonsense/ and no warning was raised

------------------------------------
Your code has been rated at 10.00/10

[luthrap]

i can take src/prefect/contrib/tasks/mysql

[joshmeek]

@luthrap Go for it! As of yesterday that file was actually moved under src/prefect/tasks/mysql 🙂 (I just updated this issue to reflect that change for future reference)

[brainless]

Hello @jameslamb and team, thanks for sharing this super well documented issue. I want to take up src/prefect/core and src/prefect/engine. Thanks

[brainless]

Just want to add one info if it helps anyone.

If you want to run pylint from inside your IDE or just without the pipe to grep, you may want to use python -m pylint <directory> --disable=all --enable=W0707. The error code we are looking for is W0707, as listed in their docs.

[brainless]

Hi everyone, thanks for merging the earlier PR. I would like to take up the following:

• src/prefect/environments/execution/dask
• src/prefect/environments/execution/k8s
• src/prefect/environments/storage
• src/prefect/tasks/airtable
• src/prefect/tasks/aws
• src/prefect/tasks/azure
• src/prefect/tasks/azureml

There are not too many cases in these directories, so it seems fine to me to get them fixed in one batch.

[jameslamb]

@brainless thanks! But could you please hold off for now? This issue was broken up into small pieces so that multiple people who are just getting started with open source could contribute on it, especially for Hacktoberfest 2020.

^ this is my opinion, but I am not a maintainer here. The maintainers may disagree with me and it's ultimately their decision.

[brainless]

@brainless thanks! But could you please hold off for now? This issue was broken up into small pieces so that multiple people who are just getting started with open source could contribute on it, especially for Hacktoberfest 2020.

^ this is my opinion, but I am not a maintainer here. The maintainers may disagree with me and it's ultimately their decision.

Hey @jameslamb, I see your point. That makes sense. I can hold off on this and find some other issues for Hacktober 2020 for Prefect, if they are labelled. Thanks.

[gaby]

@brainless Also, they don't get counted until October 1st.

[brainless]

@brainless Also, they don't get counted until October 1st.

I was not aware of the counting, just saw it today as the registrations opened. I am looking for a project to start contributing to and continue doing so long term. Hacktoberfest 2020 was just a nudge to start side-work since many projects label the "easy to start" issues and offer more help to project-newcomers.

[joshmeek]

Yeah I'm in favor of holding off and tackling in smaller pieces once October starts next Thursday 👻

[joshmeek]

Hmm what else do we need to do to enable it? I have the hacktoberfest label on issues and it has been available for use for a while 🤔

[jameslamb]

See that PR I linked. To combat spam, digitalocean is now only counting PRs from repos that have the topic hacktoberfest.

No pressure! I'm sure people would come help on this issue even without Hacktoberfest.

[jcrist]

We needed to add hacktoberfest as a repository topic. Should be resolved now.

[joshmeek]

Ah I see, misread the comment, good stuff 👍

[shalika10]

hey this is my first contribution !! please assign me src/prefect/environments/storage

[shalika10]

@jameslamb @joshmeek yayy @jcrist my first PR got merged :)

[brunocasarotti]

Hi guys, could "src/prefect/tasks/postgres" be asigned to me? It would be my first contribution too.

[brunocasarotti]

In this case:

        except (Exception, pg.DatabaseError) as error:
            conn.close()
            raise error

Should I make this change:

        except (Exception, pg.DatabaseError) as error:
            conn.close()
            raise

Or should I raise a RuntimeError from error?

[jcrist]

You should do a bare raise, we want the original error to be raised.

[sp1thas]

Hi, I could take up spacy tasks.

[heyitskevin]

Can I take src/prefect/tasks/aws ?

[gaby]

I just submitted a PR for src/prefect/tasks/twitter since today is the last day for Hacktoberfest. I can take several of the ones left is no one else is doing them.

[abidahmadq]

@jameslamb should i fix the remaining ones considering hacktoberfest is over haha?

[jameslamb]

@Abid1998 yes probably! I'm not a maintainer in this project though, so I can't say whether the maintainers here would prefer to receive one pull request which resolves the remaining cases or whether they'd rather see multiple smaller PRs which might be easier to review.

[abidahmadq]

@jameslamb will work on 1 for now and raise a PR let's see what the maintainers say after that.

[abidahmadq]

Raised a PR can someone please review it ?

[jcrist]

Fixed by #4255. Closing.