Page MenuHomeElementl

[dagit] Add `id` to PartitionSet
ClosedPublic

Authored by dish on Jan 6 2021, 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
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

dish requested review of this revision.Jan 6 2021, 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.Jan 8 2021, 4:32 AM
python_modules/dagster-graphql/dagster_graphql/schema/partition_sets.py
128

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.Jan 8 2021, 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.Jan 8 2021, 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
377โ€“379
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
204โ€“205

can remove this, see above

259

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

274โ€“276

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 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.Jan 8 2021, 8:36 PM

Remove print, something seems broken

This revision was automatically updated to reflect the committed changes.