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
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
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
| python_modules/dagster-graphql/dagster_graphql/schema/partition_sets.py | ||
|---|---|---|
| 127 | I think this probably needs to be more repository-specific. 🤔 | |
Comment Actions
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.
Comment Actions
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 | |
Comment Actions
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
Comment Actions
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)