Page MenuHomeElementl

[dagit] Add `id` to PartitionSet

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



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

R1 dagster
Lint Not Applicable
Tests Not Applicable

Event Timeline

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.Jan 8 2021, 4:32 AM

I think this probably needs to be more repository-specific. 🤔

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

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.

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

return ExternalPartitionSetOrigin(

can remove this, see above


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


you can remove this

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

including an 'id' field on GET_PARTITION_SET_STATUS_QUERY in 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.Jan 8 2021, 8:36 PM

Remove print, something seems broken

This revision was automatically updated to reflect the committed changes.