Page MenuHomePhabricator

Change the default system for loading and executing user code to be gRPC instead of the CLI API

Authored by dgibson on Oct 6 2020, 2:14 PM.



Kind of an RFC. Alex and I (and hopefully others?) have been using this flag locally to deprecate the CLI API and do everything over gRPC for some time and have fixed a number of issues that came up, it seems pretty stable at this point. Is it time to flip the switch and make it the default? We still have an opt-out switch if new surprising issues come up.

Test Plan

BK, launch runs in dagit and verify they are happening over gRPC

Diff Detail

R1 dagster
changethedefault (branched from master)
Lint OK
No Unit Test Coverage

Event Timeline

zesty - I think I'm down. Would have been nice to wire up telemetry so we would know if anyone else had used it yet.

dgibson removed reviewers: alangenfeld, schrockn, max, nate.

er hang on a sec

497 ↗(On Diff #23329)

Would be nice to give ppl some hints about what to do next


do bools just default true? this is a little confusing to me

14 ↗(On Diff #23329)

but this isn't the default anymore?


We weren't using the default value in the config here, changing it does nothing.

14 ↗(On Diff #23329)

it is the default (managed grpc) - the other option is now deployed/static grpc server. Will rename the other option to make more clear


huh that's troubling

14 ↗(On Diff #23329)

ah brilliant

this week? next week? @catherinewu / @rexledesma do we have telemetry on this at all ?

@alangenfeld re: telemetry, @rexledesma added telemetry for launch_scheduled_execution and is creating a doc to share (1) how to add telemetry to other functions and (2) how to query the collected data via sql. Are you thinking of adding telemetry for workspace_from_load_target, including the type of load_target -- should be similar to log_repo_stats unless we want to intercept the load_target within the telemetry_wrapper

This revision is now accepted and ready to land.Oct 16 2020, 6:39 PM