Adapt method difference between classic and operator KubeCluster

[john-jam]

The adapt method in the classic.KubeCluster implementation relies on the distributed.Cluster adapt method and is synchronous if we create the cluster with asynchronous=False or asynchronous=True.

The adapt method in the operator.KubeCluster implementation calls the distributed.SyncMethodMixin sync method and is asynchronous if we create the cluster with asynchronous=True (return a future from the sync method).

This creates a difference on how one should handle the KubeCluster implementations. For example, in this issue on the prefect-dask repository, when using the operator.KubeCluster implementation, the adapt method should be awaited, and when using the operator.KubeCluster implementation, it shouldn't.

@jacobtomlinson Do you have any idea how and where (upstream/downstream) one could fix this?

[jacobtomlinson]

That's an interesting difference. In the new implementation all we are doing is creating a k8s resource via the API. We could definitely make this always sync if that would help with consistency.

[john-jam]

If you think this way is more consistent I can create a PR to make the adapt method always sync.
My first guess would be to just force the asynchronous argument to False here so this condition won't make the sync method return a future.

[jacobtomlinson]

If you set asynchronous=True naively I would expect all methods to behave like coroutines. But the classic implementation didn't do this, so we've broken things.

However, I think we are doing the right thing here. Calling .adapt in the new implementation makes an HTTP call to the k8s API to create the DaskAutoscaler resource, so it is doing IO and should technically be awaited. However, in other cluster manager implementations, it doesn't do any IO it just starts an async periodic callback so it makes sense to be sync.

Ideally the Prefect runner would call inspect.isawaitable() on it and then take the right action. Perhaps this would be the better PR to make.

[john-jam]

That makes sense for the new implementation point of view.
I'll try to re-submit a PR on the prefect repo then.
Thanks for those clarifications!