Page MenuHomeElementl

Remove instance creation in sensor/schedule gRPC
ClosedPublic

Authored by rexledesma on Mar 10 2021, 9:30 PM.

Details

Summary

User code containers do not write to the db unless a run is launched via Dagit or Daemon. However, currently they read from the db since the instance is constructed when Dagit/Daemon asks for schedule or sensor execution. This should exist solely in Dagit/Daemon, so we defer the creation of the instance unless the user explicitly needs it in their sensor/schedule business logic.

With this change, the db readiness check only should exist in Dagit in Daemon, rather than the user code container.

Test Plan

pytest
integration

Diff Detail

Repository
R1 dagster
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

dgibson requested changes to this revision.EditedMar 10 2021, 10:00 PM

Here's the specific code that we need to remove before it makes sense to land this IMO: https://sourcegraph.com/github.com/dagster-io/dagster/-/blob/python_modules/dagster/dagster/grpc/impl.py#L209

and

https://sourcegraph.com/github.com/dagster-io/dagster/-/blob/python_modules/dagster/dagster/grpc/impl.py#L249

and then we'll similarly need to remove this: https://sourcegraph.com/github.com/dagster-io/dagster/-/blob/python_modules/dagster/dagster/core/definitions/schedule.py#L29

and the similar field on sensors.

Those would be breaking changes for the likely small number of people accessing the instance on their schedules and sensors. Schedule and sensor users who want to use the instance would need to load it themselves by calling DagsterInstance.get().

Once those are gone, all for it.

This revision now requires changes to proceed.Mar 10 2021, 10:00 PM

cc @phra re: ScheduleExecutionContext/SensorExecutionContext changes.

defer creation of Dagster instance to Schedule/Sensor context

remove postgres password env var

python_modules/dagster-test/dagster_test/toys/schedules.py
26–28

this looks a little odd to me... I think my preference would be to not have this be a property, so that callers have more indication that it's a context manager:

prefer build_instance, create_instance, get_instance, etc.

with context.build_instance() as instance:
   # stuff

.instance -> .get_instance()

rexledesma retitled this revision from Remove db check from user code deployments to Remove instance creation in sensor/schedule gRPC .Mar 11 2021, 8:18 PM
rexledesma edited the summary of this revision. (Show Details)
rexledesma retitled this revision from Remove instance creation in sensor/schedule gRPC to Remove instance creation in sensor/schedule gRPC.

use context manager for yielding instance

nice, thanks rex! We should definitely include a breaking change entry in 0.11.0 for this.

This revision is now accepted and ready to land.Mar 11 2021, 9:05 PM