Page MenuHomeElementl
Differential D9046

Use grpc rather than ipc to decide if a grpc server is running
ClosedPublic
Actions

Authored by • dgibson on Jul 23 2021, 6:55 PM.
Tags
None
Referenced Files
F2890905: D9046.diff
Fri, Mar 24, 3:09 AM
Unknown Object (File)
Fri, Mar 17, 3:05 PM
Unknown Object (File)
Fri, Mar 17, 5:45 AM
Unknown Object (File)
Thu, Mar 16, 5:15 AM
Unknown Object (File)
Tue, Mar 14, 9:48 PM
Unknown Object (File)
Fri, Mar 10, 4:04 PM
Unknown Object (File)
Feb 14 2023, 1:35 PM
Unknown Object (File)
Feb 14 2023, 2:23 AM
View All 90 Files
Subscribers
None

Details

Reviewers
alangenfeld
• max
Commits
R1:1a5433f9d9dc: Use grpc rather than ipc to decide if a grpc server is running
Summary

A user is reporting that writing to a tempdir on their machine takes 6 minutes. This is a nice excuse to clean up the way we do gRPC server startup - we have a perfectly good way to check if a server is running, so let's use it.

The one thing that we lose here is reporting a load error on startup in a structured way, but the --lazy-load-user-code

Test Plan

BK, run dagit locally, gRPC servers still load, simulate load failure, get a clear error in dagit

Diff Detail

Repository
R1 dagster
Branch
grpcserver (branched from master)
Lint
Lint Passed
Unit
No Test Coverage

Event Timeline

• dgibson created this revision.Jul 23 2021, 6:55 PM
• dgibson updated this revision to Diff 42222.Jul 23 2021, 7:08 PM

this means we now need to lazy-load the server so that the error can be passed along in a structured way later, but that's fine

Harbormaster returned this revision to the author for changes because remote builds failed.Jul 23 2021, 7:31 PM
Harbormaster failed remote builds in B34157: Diff 42222!
Harbormaster failed remote builds in B34156: Diff 42221!Jul 23 2021, 7:41 PM
• dgibson updated this revision to Diff 42239.Jul 23 2021, 8:24 PM

up

• dgibson published this revision for review.Jul 23 2021, 8:27 PM
• dgibson added inline comments.
python_modules/dagster/dagster/grpc/server.py
359–363

this is for idempoetency - weirdly the error changes to be less useful the second time you try to load it

Harbormaster failed remote builds in B34173: Diff 42239!Jul 23 2021, 9:09 PM
• dgibson updated this revision to Diff 42251.Jul 26 2021, 2:05 PM

up

Harbormaster completed remote builds in B34182: Diff 42251.Jul 26 2021, 2:52 PM
alangenfeld added inline comments.Jul 26 2021, 3:50 PM
python_modules/dagster/dagster/core/host_representation/grpc_server_registry.py
182

when is this False? should we drop the bool arg for GrpcServerProcess?

python_modules/dagster/dagster/grpc/server.py
359–363

hmm this seems odd - do you have an example?

Should encode probably a comment here

also worth changing the name of _serializable_load_error to be more specific since it only applies to one of this classes N responses-which-may-error

• dgibson added inline comments.Jul 26 2021, 3:59 PM
python_modules/dagster/dagster_tests/core_tests/host_representation_tests/test_grpc_server_registry.py
73 ↗(On Diff #42251)

@alangenfeld example of what you're asking for is here.

First load says "object is not callable", second load says (paraphrased) "error_repo object does not exist in module" (I think somewhere deep in python module loading code it doesn't bother to try again loading a module that it loaded before when there was a load error)

alangenfeld added a comment.Jul 26 2021, 4:25 PM

ah ok so it is the module load - the error condition you describe makes sense then but definitely worth commenting in [1] that we are assuming any exception that occurs is coming from the target code and due to python module loading behavior we capture, keep, and always re-throw that original error.

python_modules/dagster/dagster/grpc/server.py
361–369

[1]

it's surprising to me looking at this code that the python module load happens in one of these properties - I assume due to work happening in a @property? Might be worth making it a getter for clarity.

370

looking at [2] can this not be scoped to DagsterUserCodeProcessError? At least for the load problem behavior we are targeting with this idempotent capture?

python_modules/dagster/dagster_tests/core_tests/host_representation_tests/test_grpc_server_registry.py
61–82 ↗(On Diff #42251)

[2]

alangenfeld accepted this revision.Jul 26 2021, 4:27 PM

the broad except + assumptions spooks me a bit, so at least make sure the assumptions are clearly commented in-case someone bumps in to this debugging

This revision is now accepted and ready to land.Jul 26 2021, 4:27 PM
• dgibson added inline comments.Jul 27 2021, 1:46 PM
python_modules/dagster/dagster/grpc/server.py
370

DagsterUserCodeProcessError is thrown a layer above this (in the client making the gRPC call)

• dgibson updated this revision to Diff 42348.Jul 27 2021, 1:57 PM

rebase, feedback, add comments

This revision was landed with ongoing or failed builds.Jul 27 2021, 2:26 PM
Closed by commit R1:1a5433f9d9dc: Use grpc rather than ipc to decide if a grpc server is running (authored by • dgibson). · Explain Why
This revision was automatically updated to reflect the committed changes.
• dgibson added a commit: R1:1a5433f9d9dc: Use grpc rather than ipc to decide if a grpc server is running.
Harbormaster completed remote builds in B34262: Diff 42348.Jul 27 2021, 2:41 PM
• dgibson added a reverting change: D9079: Revert "Use grpc rather than ipc to decide if a grpc server is running".Jul 27 2021, 3:13 PM
• dgibson added a reverting change: R1:5b6e23b470c5: Revert "Use grpc rather than ipc to decide if a grpc server is running".Jul 27 2021, 3:14 PM

Revision Contents
Changeset List

PathSize
python_modules/
dagster/
dagster/
core/
host_representation/
1 line
grpc/
117 lines
dagster_tests/
general_tests/
grpc_tests/
323 lines

Diff 42222

View Options

python_modules/dagster/dagster/core/host_representation/grpc_server_registry.py

Loading...
View Options

python_modules/dagster/dagster/grpc/server.py

Loading...
View Options

python_modules/dagster/dagster_tests/general_tests/grpc_tests/test_persistent.py

Loading...