Prefect: PIN 15: Cloud Client Refactor

[joshmeek]

PIN 15: Cloud Client Refactor

Date: January 10, 2020

Author: Josh Meek

Status

Proposed

Context

Interaction with the Prefect Cloud API should take place in the easiest and clearest way possible. Currently the Cloud API Client interaction looks something like this:

from prefect import Client

client = Client()

client.create_project(...)
client.create_flow_run(...)
client.get_flow_run_info(...)
client.set_flow_run_state(...)

Every query and mutation that would call Cloud's GraphQL API exists at the top level of the Client. This has a few drawbacks:

• the client.py file has no order and is almost 1200 lines
• looking for a function on the Client yields a large list to sift through
• the feel of the Client is not functional in nature and has no separation of components

The current Client also _only_ contains 15 functions (one of which is deprecated).

Proposal

This PIN suggests refactoring the Client to have a more natural separation of Cloud objects with a clear function-based aspect. The Client would now act similarly to other Cloud-based clients that engineers are used to.

The Client will follow a new client.OBJECT.ACTION pattern and it would look like this:

from prefect import Client
client = Client()

flow = client.flow(id="...", ...) # get a Cloud flow based on `id`
# `flow` is now an object which has actions that can be called

print(flow)                     # outputs serialized flow information

flow.update_project(...)        # update project
flow.set_schedule_active(...)   # set schedule active
flow.run(...)                   # run flow, creates flow run
flow.archive(...)               # archive
flow.delete(...)                # delete
# etc.

Some actions would immediately return usable objects:

flow_run = flow.run(...)        # returns a flow run client object that can be used

flow_run.state                  # current state
flow_run.cancel(...)            # cancel the run
#etc.

Calling object would act as a query, calling action would act as a mutation. Other mutations could be surfaced at the higher level if they are unrelated to Cloud objects (e.g. sendFeedback) but these will be added on an adhoc basis.

In order to perform simple queries with the Client (as it currently stands) users may find themselves having to manually write GraphQL:

client.graphql(query="query { flow (where: { id: { _eq: ... } } { name } }")

This can be take a more natural programmatic approach by using the new form:

client.flow(id="...").get("name")

Consequences

The proposed refactor has some beneficial consequences:

• Client usability would increase
• a large amount of new functionality would be added that currently does not exist
• a lot of the GraphQL written for some CLI commands could be replaced with small Client calls

Of course, there are other actionable consequences as well:

• the old Client methods would need to receive a deprecation warning (to be removed at a later release)
• documentation would need to be updated
• Cloud Runners / CLI / etc. would be updated to use the new paradigm

Actions

In order to realize this PIN the _only_ actions needed are to add the new OBJECT.ACTION functionality and subsequent unit tests. Besides that there are supplemental actions which may happen at the same time or in a later Pull Request:

• add deprecation warnings to old Client methods
• update / add new Client documentation (promote new functionality)
• refactor Cloud Runners / CLI / etc. to use new client methods
• eventually remove old Client methods in a later release

[cicdw]

From the UX perspective I really like this idea overall; two things stand out to me:

1.) This call pattern stood out to me:

flow = client.flow(id="...", ...) # get a Cloud flow based on `id`
# `flow` is now an object which has actions that can be called
flow.run(...)                   # run flow, creates flow run

which I think will be incredibly confusing. We probably want to do something like flow.create_run(...) to avoid confusing users with the Core-only flow.run(...) pattern.

2.) Efficiency of queries / mutations. Right now, if I give you a Flow ID or Flow Run ID, you can directly call a mutation without a preceding query (the query aspect is implicit in the ID). In this pattern, it _appears_ at first glance to always require a query to create the top level object client.flow_run(id=...) prior to making a subsequent mutation. This could become quite painful in distributed environments or for large Flows. I suggest we add a section to this PIN outlining our plan to preserve the number of API calls required to run a flow. To be clear, I believe this is as simple as creating an object without querying the API, and only hitting the API whenever a method on an object is called, I'd just like to call this out.

[joshmeek]

1.) Yeah that makes sense to me! create_run is better because you are actually _creating_ the flow run

2.) I fully agree with this. We can avoid the first query unless explicitly made. Will update the PIN. If we want to query what are your thoughts on a query or get function?

It could look something like:

flow = client.flow(id="...") # object (no query ran)
flow.query()  # serialized flow 

[cicdw]

Yea, and maybe the query method should take arguments for the attributes to query (like half of a GraphQL payload). For example, flow.query("environment{ labels }")

[jlowin]

One recommendation stemming from Chris' point: run is almost always a verb, so my expectation is that client.run() runs something, rather than creating a "run" noun.

I anticipate two call patterns, one for creating a new object and one for retrieving an existing one. Once created / retrieved, the object can be manipulated via methods.

f = client.create_flow(...) # returns a CloudFlow object (or whatever)
# OR
f = client.get_flow(id) # returns a CloudFlow object (or whatever)
# THEN
f.delete()
f.change_project()



fr = client.create_flow_run(...) # returns a CloudFlowRun object (or whatever)
# OR
fr = client.get_flow_run(...) # returns a CloudFlowRun object (or whatever)
# THEN
fr.get_state()
fr.delete()
fr.run()

[joshmeek]

@jlowin I think the only issue with that is in the GraphQL API the mutation createFlowRun is associated with _both_ creating the Flow Run and scheduling it

[jlowin]

That's a great point - flow_run might need special casing where create_flow_run takes a schedule parameter, but still returns an object you can interact with (to query latest state or delete, but not run as I wrote above!)

[wagoodman]

I like the proposed organizational structure, it helps move from a "flat" API to a more an-object-for-a-noun hierarchical structure. Sticking closer to to the noun.verb that the PIN suggested, expressing get/create would be on the noun:

f = client.flow(name="...", ...).create()
f = client.flow(id="...").get()  # or accept name

As pointed out previously, we've reclassified the 'run' verb to be a noun, since we consider previous runs to be distinct and actionable. For instance, per our API a flow_run is a noun, so to be consistent with expectations we should be operating with flow_run.ACTION:

fr = client.flow_run(id="...").create()
fr = client.flow_run(id="...").get()

Though, if I have a flow object, it is very natural to want to be able to get one (or more) flow_run for a given flow. Thus we'd want to get a list of nouns from an action on another noun.

fr  = client.flow(name="...", ...).get_run()
fr  = client.flow(name="...", ...).create_run()

This would be notably different than mixing the noun and verb in a single call (but also unlike the above example, where we are getting a noun from a verb on another noun... just at the root level):

fr = client.create_flow_run(...)
fr = client.get_flow_run(...)

This gets to an inconsistency: do we want to "get" and "create" objects from root? (e.g. client.get_flow_run or client.create_flow_run)... or do we want to have the noun always be an object we create before hand to collect attributes for a future action?

So in short, I like moving introducing richer objects to interact with via the client, and moving actions over to these richer objects. I'm a bit stuck on where the line is for exception cases.

[joshmeek]

I'm starting to believe that the functions available from the root client may be on a case-by-case basis.

For example the function client.register makes sense to me to be root since you're providing a Prefect Core Flow object and getting a Cloud Flow metadata object in return. The pattern of having to call client.Flow().register seems backwards in this case.

In @jlowin's comment above he wrote:

fr = client.create_flow_run(...)
fr = client.get_flow_run(...)

but I am still under the impression that it would be better as:

fr = client.Flow(...).create_run()
fr.query() / get() ...etc...

Alternatively we could also go with something along the lines of initializing an empty flow run client object:

fr = client.FlowRun()
fr.create_run() / create() ...etc...

[joshmeek]

Closing, stale PIN