Prefect: PIN 16: Add `Result` subclasses corresponding to all current result handlers

[lauralorenz]

Use Case

Blocked by #2139. See PIN 16 (PR #2134) for background.

Solution

Implement a Result subclass for each of the current result handlers that knows how to .read, write, and check if it .exists:

• [x] AzureResultHandler
• [x] GCSResultHandler
• [x] LocalResultHandler
• [x] S3ResultHandler
• [x] SecretResultHandler
• [x] JSONResultHandler

Please do not implement ConstantResultHandler as part of this ticket as it will be handled separately in #2145.

Alternatives

Please describe any alternatives you've considered, even if you've dismissed them

[wagoodman]

Suggested refactor of the directory layout:

src/prefect/engine/
├── (result.py) # <--- delete
├── result/
│   ├── __init__.py # <-- import and expose base for backwards compatibility 
│   ├── base.py # <-- today's result.py
│   ├── constant_result.py
│   ├── gcs_result.py
│   ├── json_result.py
│   ├── local_result.py
│   ├── s3_result.py
│   └── secret_result.py
├── result_handlers/
└── ...

There seem to be a couple of different patterns in the repo for this case, happy to hear thoughts. @lauralorenz I could start working on some of this assuming that #2139 is mostly "complete enough"?

[lauralorenz]

@wagoodman I see about the directory structure: mimics engine/executors, environments/storage and engine/result_handlers (today), differs from environments/execution or agent, the latter group having provider subpackages. I think the division might be historically arbitrary but one side effect of the provider subpackages is you have more room to store auxillary files (like agent/kubernetes/rbac.yaml). I don't short/medium-term anticipate needing to do that for Results and we can always change it later, so I think what you have here is best organized.

Definitely pick it up, and if you can stand to derive from a non-master branch for now it does seem to me #2139 is getting pretty stable ;) (shudders in git rebase -i)

[lauralorenz]

Looks like #2139 is officially closed so we are in the clear, and I assigned this to you @wagoodman for realsies if you are still in

[wagoodman]

There's two paths to go here depending on the timeline for ResultHandlers:

1. Assuming ResultHandlers will be around for a while: during the extended depreciation period, we would instantiate existing ResultHandlers within the new Result classes (so result.write() would be a passthrough to self.result_handler.write() within the result instance, and so on). In this way we don't need to maintain similar (identical) code in two spots in the code base. On the release where we get rid of ResultHandlers for good, we migrate the code to Results.

2. If we want to get rid of ResultHandlers within a short period: duplicate the code to Results subclasses.

My vote is option 1, but I don't have a lot of context about the lifespan of ResultHandlers. Any thoughts here?

cc: @cicdw

[cicdw]

@wagoodman I think we should go with option 2 to avoid maintaining weird multi-class interactions - if we want to deprecate result handlers then I don't think we should be using them in a first class way within our codebase. Instead, we can autoconvert result handlers passed to tasks into the corresponding Result instance.

In terms of timeline, I propose that 0.12.0 can include the removal of ResultHandlers.

[cicdw]

Officially closed with #2305