Page MenuHomePhabricator

(container-supported-dagit-1) - Implement DagsterContainerGraphQLContext and hook into pipelines_or_error
ClosedPublic

Authored by themissinghlink on Apr 7 2020, 4:16 AM.

Details

Summary

This revision introduces a DagsterContainerGraphQLContext and renames the DagsterGraphQLContext to DagsterHandleGraphQLContext to be more explicit. Then to show how it ought to be used, it hooks it into the pipelines_or_error resolver.

I didn't add exception handling to presets yet, because this is only triggered by pipelineOrError resolvers and that would have blown up the revision (refactoring get_pipeline_def_from_selector is gonna be its own revision coming up next).

Test Plan

unit

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

  • fixed rough exception message
schrockn requested changes to this revision.Apr 7 2020, 10:54 PM

cool. some minor feedback

python_modules/dagster-graphql/dagster_graphql/implementation/context.py
33

reloader doesn't make too much sense in this context. Its purpose is to reload the current process to get updated pipeline definitions. The equivalent functionality here will be to refetch the container snapshot from the image

in general. I think we should only pass in the things we use

51

I would say we don't rename this for now to avoid thrash.

python_modules/dagster-graphql/dagster_graphql/schema/pipelines.py
154–156

looking at this, I think it would be cleaner to have the pipeline_snapshot be required and then pass in the presets and have them be optional. This would communicate that only the presets are required rather than the entire PipelineDefinition. It would also make this constructor simpler.

163–166

I would do a check.invariant and give a clear error message if _pipeline_definition is None/presets are unavailable

This revision now requires changes to proceed.Apr 7 2020, 10:54 PM
themissinghlink edited the summary of this revision. (Show Details)
  • got rid of the renaming and addressed the feedback
  • get rid of mocking in tests
schrockn requested changes to this revision.Apr 8 2020, 3:28 PM
schrockn added inline comments.
python_modules/dagster-graphql/dagster_graphql/implementation/context.py
18–20

Not a big fan of the multi-state constructors. I would recommend providing a static method constructor for the case where you want to call get_container_snapshot on the user's behalf.

python_modules/dagster-graphql/dagster_graphql/schema/pipelines.py
154–156

Let's just have this be pipeline_snapshot only. Make it up to callsites to call get_index() appropriately. We can provide a staticmethod ctor as convenience that takes a pipeline def only

This revision now requires changes to proceed.Apr 8 2020, 3:28 PM
schrockn added inline comments.Apr 8 2020, 3:29 PM
python_modules/dagster-graphql/dagster_graphql/schema/pipelines.py
154–156

I would have this take a pipeline index actually. We want the pipeline index to be constructed a limited number of times. It's the pipeline index that should be threaded down the stack here.

python_modules/dagster-graphql/dagster_graphql/implementation/context.py
18–20

This gets tricky because we are getting the snapshot during instantiation. If we instead let folks pass in snapshot_provider functions then this would push snapshot generation to get_repository_snapshot which is wasteful. Since in reality this class is all about snapshots and less about containers, what if we had people pass in snapshots and provide a static method constructor for the case where they want to get_container_snapshot and then pass in repository_snapshots if they don't want to provide it? It would look something like this:

class DagsterSnapshotGraphQLContext(object):
    def __init__(self, repository_snapshot, execution_manager, instance, version=None):
        self._repository_snapshot = check.inst_param(repository_snapshot, 'repository_snapshot', RepositorySnapshot)
        self._instance = check.inst_param(instance, 'instance', DagsterInstance)
        self.execution_manager = check.inst_param(
            execution_manager, 'pipeline_execution_manager', PipelineExecutionManager
        )
        self.version = version

    ....

    def get_repository_snapshot(self):
        return self._repository_snapshot

    @staticmethod
    def with_container(image, execution_manager, instance, version=None):
        return DagsterSnapshotGraphQLContext(
            repository_snapshot=get_container_snapshot(image),
            execution_manager=execution_manager,
            instance=instance,
            version=version,
        )
schrockn added inline comments.Apr 8 2020, 5:57 PM
python_modules/dagster-graphql/dagster_graphql/implementation/context.py
18–20

yup! that's a good structure here. handles both cases and makes testing nicer

  • renamed to dagster snapshot graphql context and made dauphin pipeline take pipeline indices and take presets and edited call sites
themissinghlink planned changes to this revision.Apr 8 2020, 6:28 PM

One second. Have another quick change I need to make.

Ok it is ready to rock now.

  • realized that we don't even need the container constructor, see part 3
schrockn accepted this revision.Apr 8 2020, 8:32 PM

👍🏻

python_modules/dagster-graphql/dagster_graphql/implementation/fetch_runs.py
98

might be worth it to have a

DauphinPipeline.from_pipeline_def helper ctor

This revision is now accepted and ready to land.Apr 8 2020, 8:32 PM
  • from_pipeline_def refactor