Skip to content

Remove duplicated validation logic from Instance.get_event_records #31612

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Aug 7, 2025
Merged

Remove duplicated validation logic from Instance.get_event_records #31612

merged 1 commit into from
Aug 7, 2025
+0 −24

Conversation

schrockn
Copy link
Member

@schrockn schrockn commented Aug 7, 2025 •
edited
Loading

Summary & Motivation

Removes duplicated validation logic from the Instance.get_event_records method that was warning users about using deprecated event types. This validation logic was accidentally left in place when the business logic was moved to the domain class, creating duplication.

The removed warnings covered:

  • Asset materialization planned events with partition subsets
  • Asset materialization events (suggesting to use fetch_materializations)
  • Asset observation events (suggesting to use fetch_observations)
  • Pipeline events (suggesting to use fetch_run_status_changes)

How I Tested These Changes

Existing test suite covers the core functionality.

@schrockn Graphite App
Copy link
Member Author

schrockn commented Aug 7, 2025 •
edited
Loading

This stack of pull requests is managed by Graphite. Learn more about stacking.

@schrockn schrockn mentioned this pull request Aug 7, 2025
Merged
@schrockn schrockn changed the title elim-dup-business-logic0 Remove deprecated warnings from DagsterInstance.get_event_records Aug 7, 2025
@schrockn schrockn requested a review from OwenKephart August 7, 2025 04:18
@schrockn schrockn marked this pull request as ready for review August 7, 2025 04:18
This was referenced Aug 7, 2025
Merged
Merged
Merged
@schrockn schrockn force-pushed the schrockn/ir-3-get-event-records branch from 5886bd0 to 62505e6 Compare August 7, 2025 05:18
This was referenced Aug 7, 2025
Merged
Merged
@schrockn schrockn force-pushed the schrockn/ir-2-factory branch from 2f76743 to c0c8346 Compare August 7, 2025 12:04
@schrockn schrockn force-pushed the schrockn/ir-3-get-event-records branch from 62505e6 to 38d470f Compare August 7, 2025 12:04
This was referenced Aug 7, 2025
Closed
Merged
Merged
@schrockn schrockn force-pushed the schrockn/ir-2-factory branch from c0c8346 to d91a8cf Compare August 7, 2025 13:32
@schrockn schrockn force-pushed the schrockn/ir-3-get-event-records branch from 38d470f to 6c90968 Compare August 7, 2025 13:32
@schrockn schrockn changed the base branch from schrockn/ir-2-factory to graphite-base/31612 August 7, 2025 14:06
@schrockn schrockn changed the base branch from graphite-base/31612 to schrockn/ir-2-factory August 7, 2025 15:13
@schrockn schrockn force-pushed the schrockn/ir-3-get-event-records branch from 6c90968 to fd197b3 Compare August 7, 2025 15:43
@schrockn schrockn force-pushed the schrockn/ir-2-factory branch from d91a8cf to 3d156ec Compare August 7, 2025 15:43
OwenKephart
OwenKephart approved these changes Aug 7, 2025
View reviewed changes
Copy link
Contributor

@OwenKephart OwenKephart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Title for this PR is weird

@schrockn schrockn changed the title Remove deprecated warnings from DagsterInstance.get_event_records Eliminate duplicated code Aug 7, 2025
@schrockn schrockn changed the title Eliminate duplicated code Remove duplicated validation logic from Instance.get_event_records Aug 7, 2025
@schrockn schrockn force-pushed the schrockn/ir-2-factory branch from 3d156ec to 50d15f1 Compare August 7, 2025 16:29
@schrockn schrockn force-pushed the schrockn/ir-3-get-event-records branch from fd197b3 to 1abc0e8 Compare August 7, 2025 16:29
@schrockn schrockn changed the base branch from schrockn/ir-2-factory to graphite-base/31612 August 7, 2025 17:16
@schrockn schrockn force-pushed the graphite-base/31612 branch from 50d15f1 to d80fa6b Compare August 7, 2025 17:19
@schrockn schrockn force-pushed the schrockn/ir-3-get-event-records branch from 1abc0e8 to 61b69c6 Compare August 7, 2025 17:19
@graphite-app graphite-app bot changed the base branch from graphite-base/31612 to master August 7, 2025 17:19
@schrockn schrockn force-pushed the schrockn/ir-3-get-event-records branch 2 times, most recently from 6f9c19b to 965fe00 Compare August 7, 2025 18:17
This was referenced Aug 7, 2025
Merged
Merged
@schrockn schrockn force-pushed the schrockn/ir-3-get-event-records branch from 965fe00 to 770dcc9 Compare August 7, 2025 22:11
@schrockn Graphite App
Copy link
Member Author

schrockn commented Aug 7, 2025 •
edited
Loading

Merge activity

  • Aug 7, 10:43 PM UTC: A user started a stack merge that includes this pull request via Graphite.
  • Aug 7, 10:44 PM UTC: Graphite rebased this pull request as part of a merge.
  • Aug 7, 10:46 PM UTC: @schrockn merged this pull request with Graphite.

@schrockn
Remove duplicated validation logic from Instance.get_event_records
8e6f24d 
Removes duplicated validation logic that was warning users about using deprecated event types. This validation logic was accidentally left in place when the business logic was moved to the domain class, creating duplication.
@schrockn schrockn force-pushed the schrockn/ir-3-get-event-records branch from 770dcc9 to 8e6f24d Compare August 7, 2025 22:44
@schrockn schrockn merged commit 15d8c91 into master Aug 7, 2025
4 of 5 checks passed
@schrockn schrockn deleted the schrockn/ir-3-get-event-records branch August 7, 2025 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OwenKephart OwenKephart OwenKephart approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@schrockn @OwenKephart