clearml icon indicating copy to clipboard operation
clearml copied to clipboard

Why does `Task.connect` only give a warning when a parameter is skipped?

Open mmiller-max opened this issue 1 year ago • 2 comments

One of my colleagues was caught out by Task.connect not erroring when a datetime.datetime was included in a dictionary passed to task.connect. I was wondering why the decision was made to make this a warning rather than an error? Warnings can get lost in a lot of printed text which means that key parameters that should be being logged aren't.

Two further questions that may help it move forward:

  1. Would it be possible to add a keyword argument to connect that meant it does error when a parameter is ignored, or possibly an environment variable used so it could be set on a project level?
  2. Why must only basic typed be logged, is it so they can be parsed by get_parameters_as_dict? I wonder if it's possible to add some special cases for things like datetimes.

Thanks!

mmiller-max avatar Aug 04 '22 13:08 mmiller-max

Hi @mmiller-max,

Sorry for only getting to this now 🙁

I was wondering why the decision was made to make this a warning rather than an error? Warnings can get lost in a lot of printed text which means that key parameters that should be being logged aren't.

Do you mean the log message itself, or the behavior (i.e. warn and skip vs error and exit)?

Would it be possible to add a keyword argument to connect that meant it does error when a parameter is ignored, or possibly an environment variable used so it could be set on a project level?

I guess so, shouldn't be very complicated 🙂

Why must only basic typed be logged, is it so they can be parsed by get_parameters_as_dict? I wonder if it's possible to add some special cases for things like datetimes.

You're quite right. I think specific handling for special cases can surely be added - would you care to suggest a PR? 🙂

jkhenning avatar Aug 21 '22 20:08 jkhenning

Sorry for only getting to this now 🙁

No worries!

Do you mean the log message itself, or the behavior (i.e. warn and skip vs error and exit)?

The behaviour, I would've thought an error would make more sense. Possibly just me and I guess it would not be backwards compatible!

Would it be possible to add a keyword argument to connect that meant it does error when a parameter is ignored, or possibly an environment variable used so it could be set on a project level?

I guess so, shouldn't be very complicated 🙂

Happy to do a PR for this, do you think a kwarg or an environment variable is a better approach here?

You're quite right. I think specific handling for special cases can surely be added - would you care to suggest a PR? 🙂

Sure thing!

mmiller-max avatar Aug 23 '22 09:08 mmiller-max