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.
Details
- Reviewers
bengotow dgibson sandyryza - Commits
- R1:1925f1043faf: [dagit] Add `id` to PartitionSet
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
python_modules/dagster-graphql/dagster_graphql/schema/partition_sets.py | ||
---|---|---|
128 | I think this probably needs to be more repository-specific. 🤔 |
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
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 |
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)