radical-cybertools / radical.utils

Utility classes and tools for various radical projects
Other
8 stars 6 forks source link

Disable reporter by default #395

Closed andre-merzky closed 11 months ago

andre-merzky commented 12 months ago

This fixes #394

mtitov commented 12 months ago

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 commented 12 months ago

[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 commented 12 months ago

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[bot] commented 11 months ago

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 :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## devel #395 +/- ## ======================================= Coverage 61.69% 61.69% ======================================= Files 61 61 Lines 6837 6840 +3 ======================================= + Hits 4218 4220 +2 - Misses 2619 2620 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.