Page MenuHomePhabricator

Add query / subscription graphql entry points for getting `computeLogs`
ClosedPublic

Authored by prha on Tue, Aug 27, 11:23 PM.

Details

Summary

Exposes stdout/stderr for a given run step in GraphQL via the PipelineRun and Subscription objects.

Depends on D893

Test Plan

hooked up to dagit

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

prha created this revision.Tue, Aug 27, 11:23 PM
prha updated this revision to Diff 4055.EditedTue, Aug 27, 11:40 PM

reorganize

prha updated this revision to Diff 4056.EditedTue, Aug 27, 11:41 PM

reorganize

prha added a reviewer: Restricted Project.Tue, Aug 27, 11:43 PM
prha edited the summary of this revision. (Show Details)Tue, Aug 27, 11:46 PM
Harbormaster failed remote builds in B3243: Diff 4056!
alangenfeld added inline comments.
python_modules/dagster-graphql/dagster_graphql/schema/runs.py
37–40

hm i wonder if we whouldnt hang this off the individual steps in the executionPlan instead of having a root field with a step key arg

62

justdoit

python_modules/dagster/dagster/core/execution/logs.py
20–63

im not claiming its better but worth taking a look at https://dagster.phacility.com/D885 since its solving similar problems.

  • tracking more state in event_handler might make things easier - it could hold cursor and observer for example
  • single observer multiple handlers
  • if we come up with a way for the watcher to know things are over we could have the event handler tear it self down when the writing is done. [1]
177–180

[1]

this tear down happens at the end of the compute step right? we should be able to use that to unroll things

183–189
  • probably should use bytes instead of lines
  • use seek to prevent re-reading
alangenfeld requested changes to this revision.Wed, Aug 28, 8:59 PM

any way around we should add at least a basic test

This revision now requires changes to proceed.Wed, Aug 28, 8:59 PM
prha added inline comments.Thu, Aug 29, 10:51 PM
python_modules/dagster-graphql/dagster_graphql/schema/runs.py
37–40

Yeah... I'm not sure what's right.

I think ExecutionPlan could make sense, but it's defined outside of the concept of a run. What I really want is something like an ExecutedExecutionPlan, which is a concept that we don't have...

python_modules/dagster/dagster/core/execution/logs.py
20–63

yeah, I need to figure out not just tearing down when the compute is done, but also when there are no subscribers.... actively working on this, will update.

183–189

I'll switch to string instead of lines (read vs readline), but we need to coerce to string to return it in graphql anyway.

But seek is a great idea... will switch.

prha updated this revision to Diff 4127.Thu, Aug 29, 10:59 PM

change to use file.read instead of file.readline, using file.seek for cursor

prha updated this revision to Diff 4147.Fri, Aug 30, 9:56 PM

add ComputeLogManager to manage watchers

prha updated this revision to Diff 4157.Sat, Aug 31, 1:50 AM

restructured log watcher to have single polling observer and used the pattern matching event handler

alangenfeld requested changes to this revision.Tue, Sep 3, 4:37 PM

Open to naming suggestions... don't love runStepLogs, but kinda stuck with it anyway

well you started calling stuff compute log while iterating on the diff so maybe that name is better?

probably worth adding an integration test at the GraphQL layer - ideally testing the subscription too though i don't know how reasonable that is.

python_modules/dagster/dagster/core/execution/logs.py
178–179

comment here as to why youre doing this

This revision now requires changes to proceed.Tue, Sep 3, 4:37 PM
prha updated this revision to Diff 4197.Tue, Sep 3, 11:49 PM

add GQL snapshot test

prha retitled this revision from RFC: Add query / subscription graphql entry points for getting runStepLogs to Add query / subscription graphql entry points for getting `computeLogs`.Tue, Sep 3, 11:50 PM
prha edited the summary of this revision. (Show Details)
prha updated this revision to Diff 4198.Wed, Sep 4, 12:01 AM
prha edited the summary of this revision. (Show Details)

tests

prha updated this revision to Diff 4199.Wed, Sep 4, 12:08 AM

sync graphql schema

prha updated this revision to Diff 4202.Wed, Sep 4, 12:37 AM

rebase

schrockn added inline comments.
python_modules/dagster-graphql/dagster_graphql/schema/runs.py
124

is this all of stdout/stderr? worthy of a description field

This revision is now accepted and ready to land.Fri, Sep 6, 3:23 PM