Page MenuHomePhabricator

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

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

Details

Summary

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

Repository
R1 dagster
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

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

python_modules/dagster/dagster/core/host_representation/repository_location.py
498–500

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

python_modules/dagster/dagster/core/instance/config.py
56–57

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

python_modules/dagster/dagster_tests/scheduler_tests/test_scheduler_failure_recovery.py
13–14

but this isn't the default anymore?

python_modules/dagster/dagster/core/instance/config.py
56–57

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

python_modules/dagster/dagster_tests/scheduler_tests/test_scheduler_failure_recovery.py
13–14

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

python_modules/dagster/dagster/core/instance/config.py
56–57

huh that's troubling

python_modules/dagster/dagster_tests/scheduler_tests/test_scheduler_failure_recovery.py
13–14

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.Fri, Oct 16, 6:39 PM