feature: scrub logs

[saikonen]

introduces logs scrub run_id/step/task_id command for scrubbing logs of tasks.

by default it deletes both stdout/stderr for the latest attempt of a task.

Final feature set:

• logs [pathspec] works as previously, with the new default subcommand custom click group
• logs show [pathspec] is the default subcommand for logs cmd group
• logs scrub [pathspec] will overwrite logs with a redacted message

also adds support for both show/scrub commands to specify --attempt [number] to target a specific attempt of a task, instead of the latest.

[savingoyal]

@saikonen we could introduce a top-level delete command that has a sub command - logs? wdyt? also, the default behavior could be for --latest-attempt and users can pass in --all-attempts to wipe out logs for all attempts?

[saikonen]

this now introduces logs as a command group, with a default fallback to the new logs show command in case no subcommmand is specified.

the following command still works the same

python LongLog.py logs 1713532810109387/start/1
python LongLog.py logs 1713532810109387/start/1 --stderr

but now we can also support the following

python LongLog.py logs delete 1713532810109387/start/1

[savingoyal]

input-path?

[saikonen]

same as in the existing 'show' command. should this be reworded to something else? pathspec for example?

[savingoyal]

what is the behavior for tasks that haven't finished yet? a few different cases here -

1. the attempt is still in flight
2. the attempt has finished but we expect further attempts

[saikonen]

one approach would be that we instead init the datastore with allow_not_done=False ?

[saikonen]

bunch of fixes regarding this. How things work now:
we rely on allow_not_done=False and check when initializing the datastore in mode="d" that the latest / specified attempt is done, otherwise we raise an error.

• trying to logs scrub [pathspec] on a running task will lead to an error.
• logs scrub [pathspec] --attempt x will work even on a running task, as long as the specified attempt has finished.

so specifically for the above cases:

1. if attempt is in flight, log scrubbing raises an exception for that specific attempt (or no attempt specified)
2. attempt has finished, if scrubbing with --attempt it will work. If not specifying an attempt, the latest attempt will point to an attempt that has not yet finished, raising an exception

[saikonen]

Note: I also tried simply allowing the scrubbing of logs for running tasks, but due to how logs are written in bursts from a buffer, the effects of overwriting a log with the [REDACTED] message were being nullified, as every log write cycle would simply rewrite the scrubbed log lines if they were still in buffer.

[romain-intel]

I would definiltely not advocate for allowing the redaction of in-flight logs. THere are just too many complications. We would have to prevent writing to a log that has been redacted which we can't do with past versions of metaflow so there would be this big gapping hole.

[romain-intel]

I like the current behavior described @saikonen -- ie: can scrub past completed attempts with --attempt; can scrub latest attempt if finished with no arg and can't scrub any other case.

[savingoyal]

added a few comments

[romain-intel]

No issues with the larger feature -- a few comments here and there.

[romain-intel]

is there a case where we want to delete logs for running tasks? It seems in delete mode we may want to ingore allow_not_done?

[saikonen]

I left this in mainly in order to keep the datastore side more generic (allowing delete for in-flight attempts as well, though no use case exists yet). It is definitely not required for this feature, so I could remove it

[romain-intel]

we need to check if this is handled by @madhur-ob 's click parsing thing as well (should be but mentioning).

[saikonen]

tested, and this does not work as-is with the low level click api due to the fallback logic happening when invoking the command. Calling the explicit subcommand works as expected though, but the group level fallback fails

from metaflow.metaflow_runner import MetaflowAPI
from metaflow.cli import start

if __name__ == "__main__":

    api = MetaflowAPI.from_cli("AttemptLog.py", start)

    # works
    cmd = api().logs().show(input_path="217190/start")
    print(cmd)

    # does not
    cmd = api().logs(input_path="217190/start")
    print(cmd)

Will look into how easy of a fix there could be to support it

[romain-intel]

Cool -- we don't need to fix it for this but ya, we need to probbaly have it working at some point. As in, the low level api should be able to handle all our cases.

[romain-intel]

I like the current behavior described @saikonen -- ie: can scrub past completed attempts with --attempt; can scrub latest attempt if finished with no arg and can't scrub any other case.

[savingoyal]

will this also try to scrub in-flight tasks?

[romain-intel]

I like this refactor. I think with a couple name changes, it makes a lot more sense and the code looks clear imho.