Disable reporter by default

[andre-merzky]

This fixes #394

[AymenFJA]

LGTM thanks @andre-merzky.

[mtitov]

With the second thought, I think Reporter should be enabled by default, and inside RP we set it disabled by default then.

In the RP examples when we create reporter outside of RP - it is unexpected to have an argument to enable it explicitly. But having reporter inside RP it is on us to integrate it properly, thus inside Session we can set self._reporter = ru.Reporter(..., enabled=False). I personally don't care from where that default value comes (either from default config file, or from such input argument), but it should be in sync with the corresponding env variable (RADICAL_REPORT), which should be able overwrite it.

def __init__(self, name, ns=None, path=None, targets=None, enabled=True):

    env_report = ru_get_env_ns('report', ns)
    if env_report is not None:
        env_report = str(env_report).lower()

        if env_report in ['0', 'false', 'off']:
            self._enabled = False
        else:
            self._enabled = True
    else:
        self._enabled = bool(enabled)

[andre-merzky]

[edited after re-reading your comment]

I don't think that an env variable should overwrite an explicit argument - that is different from what I would expect, and is different from what we do in other places (logger, profiler, other reporter arguments). So we would need to pass an argument flag to the (primary) session. Which is, well, ok, but I am not a fan of adding more flags and switches. Unless we allow to pass a full reporter instance to the session instead of constructing it ourself.

[mtitov]

I don't think that an env variable should overwrite an explicit argument

my proposal was about considering that that input argument is a default value (enabled= -> default=) instead of taking it from a configuration file.

So we would need to pass an argument flag to the (primary) session.

sorry, this one I didn't understand. I thought if to have such input argument, then we would need to update Reporter creation in just this method https://github.com/radical-cybertools/radical.pilot/blob/devel/src/radical/pilot/session.py#L1073-L1083 with default=False. And it would be aligned with - if we want to have a certain default behavior for Reporter in RP, then it should be changed there.

p.s. anyway I don't want to push it further, just it looks less intuitive for user (i.e., creating an instance of Reporter which is not active unless we provide an extra argument)

[codecov]

Codecov Report

Attention: Patch coverage is 90.00000% with 1 line in your changes missing coverage. Please review.

Project coverage is 61.69%. Comparing base (5b5e2c3) to head (3abcac6).
Report is 223 commits behind head on devel.

Files with missing lines	Patch %	Lines
src/radical/utils/reporter.py	90.00%	1 Missing ⚠️

Additional details and impacted files

@@           Coverage Diff           @@
##            devel     #395   +/-   ##
=======================================
  Coverage   61.69%   61.69%           
=======================================
  Files          61       61           
  Lines        6837     6840    +3     
=======================================
+ Hits         4218     4220    +2     
- Misses       2619     2620    +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mtitov]

LGTM! Thanks, Andre!

p.s. I've added couple comments just for a small cleanup, you can skip them if you want