Checks for proper email configuration before sending error emails

[sklarsa]

Description

Currently, if a user did not configure email in the client.cfg or isn't pointing luigi to a config file at all, whenever a task runs that throws an error, a warning about a None parameter is shown:
/Users/steven/source/github/luigi/luigi/parameter.py:261: UserWarning: Parameter value None is not of type string. warnings.warn("Parameter value {0} is not of type string.".format(str(x)))

I was able to track down this warning to the email config task in luigi.notifications. Since the prefix and receiver tasks default to None, this warning is thrown when the email is instantiated and the parameter values are serialized.

Instead of changing the warning or email task directly, I decided to add code in luigi.notifications.send_error_email which checks to make sure that email is configured properly before attempting to send an error email in any context.

Motivation and Context

I believe that given a default use-case with no custom configs, luigi should not be throwing ambiguous warnings. This can confuse users, potentially creating the misconception that there is something wrong with a user's task or pipeline, when in fact it's an internal task that is throwing the warning.

Have you tested this? If so, how?

I created a task that always throws an error:

class TaskC(luigi.Task):

    def run(self):
        raise Exception("TEST")

    def complete(self):
        return False

Then I ran luigi TaskA --local-scheduler and saw the new warnings regarding the invalid configuration instead of an ambiguous parameter warning.

[mention-bot]

@sklarsa, thanks for your PR! By analyzing the history of the files in this pull request, we identified @daveFNbuck, @neilisaac and @jslovan to be potential reviewers.

[Tarrasch]

Actually. We just recently deliberately removed the warning you get when not having configured [email], see #1908 (comment). Can you instead make PR that just get rids of the UserWarning: Parameter value None is not of type string annoyance?

[sklarsa]

Done

[Tarrasch]

I actually meant to find what Parameter is set to None and instead set it to some other working default value like '' or whatever is suitable. :)

I don't think we should remove that best-practices warning. Although it would be so much better if it would point out where the Parameter was first defined ...

[sklarsa]

How's this?

[sklarsa]

I can look into that a little more. I don't think the parameter class
itself has any knowledge of what task it is in though... Perhaps we can
make the error message a little more detailed instead so it can be more
useful when running larger pipelines

On Nov 17, 2016 10:55 PM, "Arash Rouhani" notifications@github.com wrote:

I actually meant to find what Parameter is set to None and instead set it
to some other working default value like '' or whatever is suitable. :)

I don't think we should remove that best-practices warning. Although it
would be so much better if it would point out where the Parameter was first
defined ...

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1923 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AB1xRfZCZ63sw9QPKJjQgAvCBGMVWNBmks5q_SG-gaJpZM4K1u2B
.

[Tarrasch]

Very good job! Thanks for fixing according to my feedback. :)

---

I don't think the parameter class itself has any knowledge of what task it is in though

You're thinking like a C++-programmer ^^. I wouldn't be surprised if there's something like my_object.__line_where_object_was_defined__ in python. Have you googled it? :)

[sklarsa]

I'll take a look at it but save it for a separate diff since it might take a while for me to come up with a relatively elegant solution

[Tarrasch]

@sklarsa, Indeed, please save it for a future PR. However there seem to be some test error related to this change. Can you see if it can be fixed? :)

[sklarsa]

Tests passing

[Tarrasch]

Sorry. Just 1 small change and I promise this is ready to be merged! :)

[Tarrasch]

Thanks. Though these two lines can be simplified to simply:

if email().prefix:

This is awesome. As we also get rid of the is not None antipattern in python!

[Tarrasch]

Finally. Thanks @sklarsa, if you could find a way to generally better point out where the non-string parameter comes from, that would be great. :)

[Tarrasch]

@sklarsa, it would be so good if all parameters who "complain" also printed out where they were implemented. It seems be reasonably easy to implement. http://stackoverflow.com/a/22148022/621449, do you want to make a PR or at least create an issue mentioning this as a TODO?

[sklarsa]

Sure thing..I can make a PR for that

…

On Mon, Nov 28, 2016 at 11:14 PM, Arash Rouhani ***@***.***> wrote: @sklarsa <https://github.com/sklarsa>, it would be so good if all parameters who "complain" also printed out where they were implemented. It seems be reasonably easy to implement. http://stackoverflow.com/a/ 22148022/621449, do you want to make a PR or at least create an issue mentioning this as a TODO? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1923 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AB1xRYQIytGBVSvibNVzL1roI0Ocm_Ezks5rC6akgaJpZM4K1u2B> .