Page MenuHomePhabricator

[dagit] Add `id` to PartitionSet
ClosedPublic

Authored by dish on Wed, Jan 6, 4:51 PM.

Details

Summary

While looking into #3503 I noticed an Apollo warning about PartitionSet needing an id field for better caching. This adds it, just duplicating the name field.

Test Plan

View partition set in Dagit, click to view different "Last X" slices, verify that the Apollo warning is gone.

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

dish requested review of this revision.Wed, Jan 6, 5:10 PM

This seems reasonable to me, but I'm a JS noob so going to defer to Ben.

This revision is now accepted and ready to land.Fri, Jan 8, 4:32 AM
python_modules/dagster-graphql/dagster_graphql/schema/partition_sets.py
127

I think this probably needs to be more repository-specific. ๐Ÿค”

Use a generated ID instead, to avoid name collisions across repositories.

dish requested review of this revision.Fri, Jan 8, 3:43 PM
dish added a reviewer: dgibson.

Added some Python changes to include a generated ID for PartitionSet. Fairly dogscience, so please let me know if I did it all wrong or missed something.

dgibson requested changes to this revision.Fri, Jan 8, 8:29 PM

generally i think this makes snese, just suggest doign it based on the pipeline rather than the repo

python_modules/dagster/dagster/core/host_representation/handle.py
376โ€“378
return ExternalPartitionSetOrigin(
 self.repository_handle.get_external_origin().get_pipeline_origin(
          self.pipeline_name
),
self.partition_set.name)
python_modules/dagster/dagster/core/host_representation/origin.py
203โ€“204

can remove this, see above

258

this should probably take in an external_pipeline_origin or a repo origin instead, to reflect the fact that (I think) it only needs to be unique per-pipeline, unlike schedules

273โ€“275

you can remove this

This revision now requires changes to proceed.Fri, Jan 8, 8:29 PM

including an 'id' field on GET_PARTITION_SET_STATUS_QUERY in test_partition_sets.py would be cool too just to make sure the new code gets exercised in tests - don't necc. need to verify the value of the result that comes out although that would be even better

ok, I was wrong, these are unique per-repo. What you have here should be fine. (Could still add the 'id' field in the test though)

This revision is now accepted and ready to land.Fri, Jan 8, 8:36 PM

Remove print, something seems broken

This revision was automatically updated to reflect the committed changes.