Print statements in builtin functions

[Mark Dokter <md...@know-center.at>]

Hi!

While reviewing PRs lately I have noticed quite a number of print
statements in the code. Imho that is ok while developing but should not
be there when merging it in. The internals of builtins don't need
documentary status output like "---=== Method X builtin function
starting now ===---". These wouldn't hurt as much as something that
prints out intermediates. A few more thoughts on it:

* For high frequency bulitins print() statements not only slow down
execution but also fill up logs unnecessarily.

* An "if verbose" toggle might be a half baked remedy as it increases
compile time if there's a lot of debugging code to remove.

* print() statements that are left in the code get in the way of
rewrites and operator fusion.

So I'm inclined to clean out all the print() statements (or at least
comment them out) while merging PRs.

Opinions?

Best regards,
Mark

Re: Print statements in builtin functions

[arnab phani <ph...@gmail.com>]

I agree with Sebastian. Users should be given a way to control verbosity --
that helps in debugging and also as per other libraries (e.g. Scikit learn).
We can have multiple levels of verbosity though, let say from 1 to 5.

Regards,
Arnab.

On Sat, Feb 27, 2021 at 1:49 PM Baunsgaard, Sebastian
<ba...@tugraz.at.invalid> wrote:

> I'm in favor of the if verbose option, since ideally the compile overhead
> is (and should be) negligible and if a user want then they can get
> information from the methods.
>
> ________________________________
> From: Mark Dokter <md...@know-center.at>
> Sent: Saturday, February 27, 2021 1:47:05 PM
> To: dev@systemds.apache.org
> Subject: Print statements in builtin functions
>
> Hi!
>
> While reviewing PRs lately I have noticed quite a number of print
> statements in the code. Imho that is ok while developing but should not
> be there when merging it in. The internals of builtins don't need
> documentary status output like "---=== Method X builtin function
> starting now ===---". These wouldn't hurt as much as something that
> prints out intermediates. A few more thoughts on it:
>
> * For high frequency bulitins print() statements not only slow down
> execution but also fill up logs unnecessarily.
>
> * An "if verbose" toggle might be a half baked remedy as it increases
> compile time if there's a lot of debugging code to remove.
>
> * print() statements that are left in the code get in the way of
> rewrites and operator fusion.
>
> So I'm inclined to clean out all the print() statements (or at least
> comment them out) while merging PRs.
>
> Opinions?
>
> Best regards,
> Mark
>
>

Re: Print statements in builtin functions

["Baunsgaard, Sebastian" <ba...@tugraz.at.INVALID>]

I'm in favor of the if verbose option, since ideally the compile overhead is (and should be) negligible and if a user want then they can get information from the methods.

________________________________
From: Mark Dokter <md...@know-center.at>
Sent: Saturday, February 27, 2021 1:47:05 PM
To: dev@systemds.apache.org
Subject: Print statements in builtin functions

Hi!

While reviewing PRs lately I have noticed quite a number of print
statements in the code. Imho that is ok while developing but should not
be there when merging it in. The internals of builtins don't need
documentary status output like "---=== Method X builtin function
starting now ===---". These wouldn't hurt as much as something that
prints out intermediates. A few more thoughts on it:

* For high frequency bulitins print() statements not only slow down
execution but also fill up logs unnecessarily.

* An "if verbose" toggle might be a half baked remedy as it increases
compile time if there's a lot of debugging code to remove.

* print() statements that are left in the code get in the way of
rewrites and operator fusion.

So I'm inclined to clean out all the print() statements (or at least
comment them out) while merging PRs.

Opinions?

Best regards,
Mark