Page MenuHomePhabricator

RFC: add origin information into the repository picker
ClosedPublic

Authored by prha on Jul 22 2020, 9:46 PM.

Details

Summary

it's a little chunky, but here's a version with more origin info:

Test Plan

bk

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.Jul 22 2020, 9:46 PM
prha updated this revision to Diff 19335.Jul 22 2020, 10:00 PM

fix snapshots

prha retitled this revision from add origin information into the repository picker to RFC: add origin information into the repository picker.Jul 22 2020, 10:00 PM
prha edited the summary of this revision. (Show Details)
prha requested review of this revision.Jul 22 2020, 10:15 PM
schrockn requested changes to this revision.Jul 22 2020, 10:19 PM
schrockn added subscribers: dgibson, alangenfeld.

This looks reasonable. I think you should maybe talk to @alangenfeld and @dgibson quick about supporting gprc location types. It might change the structure pretty substantially so might want to just do it now.

Also in the case where the executable is the same executable as the one that is hosting dagit should we omit it? Might be cleaner

js_modules/dagit/src/nav/RepositoryPicker.tsx
95

why function and not attribute?

This revision now requires changes to proceed.Jul 22 2020, 10:19 PM
alangenfeld added inline comments.Jul 22 2020, 10:25 PM
js_modules/dagit/src/schema.graphql
1078–1079

to set this up to be forward looking we should change the type to PythonRepositoryOrigin then later introduce a RepositoryOrigin interface / union

alangenfeld added inline comments.Jul 22 2020, 10:28 PM
js_modules/dagit/src/nav/RepositoryPicker.tsx
89–101

I think it would be good to make this a re-usable little component then you could replace the current call site of codePointerDescription with it. I am working on a instance level schedule page that could use it too

prha updated this revision to Diff 19352.Jul 23 2020, 4:31 AM
  • extract RepositoryInformation component
  • renamed python origin
  • added instance query, hook for dagit executable path

yeah, this is timely since I just introduced a base RepositoryOrigin and new subclass in D3975.

I had been planning to possibly remove executable_path and the code pointer stuff from the repos running in GRPC servers, in place of just an identifier for the repo, but knowing that we may want to still surface that information in dagit means I should probably not do that.

js_modules/dagit/src/RepositoryInformation.tsx
9–14

we should imagine a world someday where these specific keys aren't set for every repo.

Might be overkill while everything's still in python, but we could also consider allowing the repos to supply key-value metadata pairs for this on the server rather than assuming a specific structure on the client?

python_modules/dagster-graphql/dagster_graphql/schema/external.py
39

will this error out if you call it on a repo with a GrpcServer origin rather than a Python origin?

There should be some dagster-graphql test coverage that includes those types of origins now (for example sqlite_instance_external_grpc_server_env), not sure if it goes through this specific codepath though

bengotow added inline comments.Jul 23 2020, 2:59 PM
python_modules/dagster-graphql/dagster_graphql/schema/external.py
163–175

This looks good but I think I agree with @dgibson's comment above - exposing a set of arbitrary key-value pairs specific to the pointer type and rendering them seems like it might be more flexible than coercing these different pieces of data into "value" and "attribute"? I wouldn't particularly expect "attribute" to be a function name but admittedly I am not a python developer.

Other than that this looks great though! The UI is indeed slightly chunky but we can tweak it as we go and if people start adding more than a few repositories.

alangenfeld added inline comments.Jul 23 2020, 3:04 PM
js_modules/dagit/src/RepositoryInformation.tsx
22–49

*

prha updated this revision to Diff 19389.Jul 23 2020, 4:20 PM
  • make code pointer metadata generic
prha updated this revision to Diff 19399.Jul 23 2020, 5:34 PM
  • update web tests

@prha https://dagster.phacility.com/D3991 is the diff after which code_repository_dict would no longer be exposed on the origin for some repos (unless we want to keep it around to support this feature)

dgibson added inline comments.Jul 24 2020, 3:58 PM
python_modules/dagster-graphql/dagster_graphql/schema/external.py
83

(after which this would error out on grpc repos since there is no code_pointer field)

prha added inline comments.Jul 24 2020, 4:28 PM
python_modules/dagster-graphql/dagster_graphql/schema/external.py
83

Yeah, I can hold off on this until D3991 lands and then just hoist up the relevant info onto the graphql origin field

prha updated this revision to Diff 19700.Jul 28 2020, 11:44 PM
  • handle repository origin union, with grpc
bengotow accepted this revision.Aug 4 2020, 6:34 PM

This looks great to me 👍 I feel like the metadata approach gives this a lot of flexibility and the code looks clean. I think we can tweak the visual appearance a bit but the ideal styling sort of depends on how many options end up appearing in that menu in the common case, so it might be best to wait and see how this gets used!

schrockn accepted this revision.Aug 4 2020, 9:13 PM
This revision is now accepted and ready to land.Aug 4 2020, 9:13 PM
prha updated this revision to Diff 20099.Aug 4 2020, 11:23 PM

rebase

This revision was automatically updated to reflect the committed changes.