Page MenuHomePhabricator
Differential D4679

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

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

Details

Reviewers
alangenfeld
schrockn
max
nate
Commits
R1:7aa975ca24e4: Change the default system for loading and executing user code to be gRPC…
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

dgibson created this revision.Oct 6 2020, 2:14 PM
Harbormaster completed remote builds in B19178: Diff 23306.Oct 6 2020, 2:33 PM
dgibson requested review of this revision.Oct 6 2020, 2:33 PM
alangenfeld added a comment.Oct 6 2020, 2:35 PM

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 planned changes to this revision.Oct 6 2020, 2:40 PM
dgibson removed reviewers: alangenfeld, schrockn, max, nate.

er hang on a sec

dgibson updated this revision to Diff 23310.Oct 6 2020, 2:44 PM

up

Harbormaster failed remote builds in B19182: Diff 23310!Oct 6 2020, 3:02 PM
dgibson updated this revision to Diff 23326.Oct 6 2020, 5:46 PM

test fixes

dgibson updated this revision to Diff 23327.Oct 6 2020, 5:46 PM
dgibson added reviewers: alangenfeld, schrockn, max, nate.

up

dgibson updated this revision to Diff 23328.Oct 6 2020, 5:48 PM

up

dgibson updated this revision to Diff 23329.Oct 6 2020, 6:00 PM

one more test fix

Harbormaster failed remote builds in B19197: Diff 23328!Oct 6 2020, 6:08 PM
Harbormaster failed remote builds in B19196: Diff 23327!Oct 6 2020, 6:11 PM
Harbormaster failed remote builds in B19195: Diff 23326!Oct 6 2020, 6:14 PM
Harbormaster completed remote builds in B19198: Diff 23329.Oct 6 2020, 6:20 PM
max added inline comments.Oct 7 2020, 3:47 PM
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?

dgibson added inline comments.Oct 7 2020, 3:49 PM
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

max added inline comments.Oct 7 2020, 4:35 PM
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

alangenfeld added subscribers: rexledesma, catherinewu.Oct 13 2020, 3:10 PM

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

catherinewu added a comment.Oct 13 2020, 5:28 PM

@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

alangenfeld accepted this revision.Oct 16 2020, 6:39 PM

illallowit

This revision is now accepted and ready to land.Oct 16 2020, 6:39 PM
dgibson updated this revision to Diff 24014.Oct 19 2020, 7:46 PM

up

Harbormaster failed remote builds in B19791: Diff 24014!Oct 19 2020, 8:03 PM
dgibson updated this revision to Diff 24028.Oct 19 2020, 9:16 PM

up

Harbormaster failed remote builds in B19804: Diff 24028!Oct 19 2020, 9:34 PM
dgibson updated this revision to Diff 24074.Oct 20 2020, 4:52 PM

rebase

Harbormaster failed remote builds in B19844: Diff 24074!Oct 20 2020, 5:13 PM
dgibson updated this revision to Diff 24080.Oct 20 2020, 5:28 PM

test fix

Harbormaster failed remote builds in B19850: Diff 24080!Oct 20 2020, 5:47 PM
dgibson updated this revision to Diff 24083.Oct 20 2020, 5:52 PM

test fix

Harbormaster completed remote builds in B19853: Diff 24083.Oct 20 2020, 6:13 PM
Closed by commit R1:7aa975ca24e4: Change the default system for loading and executing user code to be gRPC… (authored by dgibson). · Explain WhyOct 20 2020, 6:16 PM
This revision was automatically updated to reflect the committed changes.
dgibson added a commit: R1:7aa975ca24e4: Change the default system for loading and executing user code to be gRPC….

Revision Contents
Changeset List

PathSize
integration_tests/
test_suites/
k8s-integration-test-suite/
6 lines
python_modules/
dagster-graphql/
dagster_graphql/
test/
2 lines
dagster_graphql_tests/
graphql/
5 lines
23 lines
71 lines
dagster/
dagster/
core/
host_representation/
8 lines
4 lines
instance/
4 lines
dagster_tests/
api_tests/
1 line
4 lines
core_tests/
instance_tests/
19 lines
launcher_tests/
7 lines
34 lines
2 lines
scheduler_tests/
9 lines
96 lines
31 lines
libraries/
dagster-cron/
dagster_cron_tests/
6 lines

Diff 24087

View Options

integration_tests/test_suites/k8s-integration-test-suite/test_scheduler.py

Loading...
View Options

python_modules/dagster-graphql/dagster_graphql/test/utils.py

Loading...
View Options

python_modules/dagster-graphql/dagster_graphql_tests/graphql/graphql_context_test_suite.py

Loading...
View Options

python_modules/dagster-graphql/dagster_graphql_tests/graphql/test_dagster_environment.py

Loading...
View Options

python_modules/dagster-graphql/dagster_graphql_tests/graphql/test_reload_repostitory_location.py

Loading...
View Options

python_modules/dagster/dagster/core/host_representation/handle.py

Loading...
View Options

python_modules/dagster/dagster/core/host_representation/repository_location.py

Loading...
View Options

python_modules/dagster/dagster/core/instance/config.py

Loading...
View Options

python_modules/dagster/dagster_tests/api_tests/test_api_snapshot_repository.py

Loading...
View Options

python_modules/dagster/dagster_tests/api_tests/utils.py

Loading...
View Options

python_modules/dagster/dagster_tests/core_tests/instance_tests/test_instance.py

Loading...
View Options

python_modules/dagster/dagster_tests/core_tests/launcher_tests/test_default_run_launcher.py

Loading...
View Options

python_modules/dagster/dagster_tests/core_tests/test_origin.py

Loading...
View Options

python_modules/dagster/dagster_tests/core_tests/test_pipeline_execution.py

Loading...
View Options

python_modules/dagster/dagster_tests/scheduler_tests/test_scheduler_failure_recovery.py

Loading...
View Options

python_modules/dagster/dagster_tests/scheduler_tests/test_scheduler_run.py

Loading...
View Options

python_modules/dagster/dagster_tests/scheduler_tests/test_scheduler_timezones.py

Loading...
View Options

python_modules/libraries/dagster-cron/dagster_cron_tests/test_cron_scheduler.py

Loading...