Skip to content

Reorganize DagsterInstance with consolidated public API methods and domain separation #31633

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 2 commits into from
Aug 9, 2025
Merged

Reorganize DagsterInstance with consolidated public API methods and domain separation #31633

merged 2 commits into from
Aug 9, 2025

Conversation

schrockn
Copy link
Member

@schrockn schrockn commented Aug 7, 2025 •
edited
Loading

Summary & Motivation

Reorganized the DagsterInstance class to consolidate all public API methods in the main instance.py file with clear domain separation. This refactoring improves API discoverability by moving public methods from mixins to a centralized, well-organized structure in the main Instance class.

Key organizational changes:

  • PUBLIC API METHODS section with domain-based organization (Asset Domain, Run Domain, Event Domain, Storage/Partition Domain)
  • INTERNAL METHODS section clearly separated from public API
  • Consolidated TYPE_CHECKING imports for better organization
  • Removed duplicate public methods from AssetMixin that are now centralized in Instance
  • Added _asset_domain property accessor for mixin integration

The public API surface remains unchanged - this is purely an organizational improvement that makes the Instance API more discoverable and maintainable.

How I Tested These Changes

Existing test suite validates that all public API methods continue to work correctly. The refactoring preserves all existing functionality while improving code organization.

This was referenced Aug 7, 2025
Merged
Merged
Merged
Merged
Merged
Merged
Merged
Closed
Merged
@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.

This was referenced Aug 7, 2025
Merged
Merged
@schrockn schrockn force-pushed the schrockn/organize-instance-with-public-methods branch from 8af97e1 to 97d7399 Compare August 7, 2025 22:11
@schrockn schrockn force-pushed the schrockn/instigator-mixin branch 2 times, most recently from 6ff4727 to 566e9e0 Compare August 7, 2025 23:06
@schrockn schrockn force-pushed the schrockn/organize-instance-with-public-methods branch 2 times, most recently from b397919 to 65682a3 Compare August 8, 2025 11:43
@schrockn schrockn force-pushed the schrockn/instigator-mixin branch from 566e9e0 to 26aa97b Compare August 8, 2025 11:43
@schrockn schrockn force-pushed the schrockn/organize-instance-with-public-methods branch from 65682a3 to ca22ca1 Compare August 8, 2025 11:49
@schrockn schrockn force-pushed the schrockn/instigator-mixin branch from 26aa97b to c2e7024 Compare August 8, 2025 11:49
@schrockn schrockn mentioned this pull request Aug 8, 2025
Merged
@schrockn schrockn force-pushed the schrockn/organize-instance-with-public-methods branch from ca22ca1 to 7031804 Compare August 8, 2025 12:49
@schrockn schrockn force-pushed the schrockn/instigator-mixin branch 2 times, most recently from a12d203 to b6a0875 Compare August 8, 2025 14:43
@schrockn schrockn force-pushed the schrockn/organize-instance-with-public-methods branch from d6a13ef to 8fe47c7 Compare August 8, 2025 14:43
@schrockn schrockn mentioned this pull request Aug 8, 2025
Merged
@schrockn schrockn force-pushed the schrockn/instigator-mixin branch from b6a0875 to fce91ec Compare August 8, 2025 15:07
@schrockn schrockn force-pushed the schrockn/organize-instance-with-public-methods branch 2 times, most recently from 9a9a539 to 347c75e Compare August 8, 2025 17:44
@schrockn schrockn force-pushed the schrockn/instigator-mixin branch from fce91ec to 60b73f3 Compare August 8, 2025 17:44
@schrockn schrockn changed the title reorg Reorganize DagsterInstance with consolidated public API methods and domain separation Aug 8, 2025
@schrockn schrockn changed the base branch from schrockn/improve-bk-error-detective to graphite-base/31633 August 8, 2025 20:23
@schrockn schrockn force-pushed the schrockn/organize-instance-with-public-methods branch from 9fb398d to e7af97a Compare August 8, 2025 20:23
@schrockn schrockn force-pushed the graphite-base/31633 branch from c988a4b to ad69b88 Compare August 8, 2025 20:23
@schrockn schrockn changed the base branch from graphite-base/31633 to schrockn/instigator-mixin August 8, 2025 20:23
@schrockn schrockn marked this pull request as ready for review August 8, 2025 20:31
@schrockn schrockn requested a review from OwenKephart August 8, 2025 20:31
@schrockn schrockn force-pushed the schrockn/organize-instance-with-public-methods branch from e7af97a to ff1f56f Compare August 8, 2025 21:03
@schrockn schrockn force-pushed the schrockn/instigator-mixin branch from ad69b88 to cb3cd39 Compare August 8, 2025 21:03
OwenKephart
OwenKephart requested changes Aug 8, 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.

this one seems obsolete in the context of the desired end state in the above PR (where there shouldn't really be many methods on the DagsterInstance class, all of them just come from the relevant Methods class)

@schrockn schrockn force-pushed the schrockn/instigator-mixin branch 2 times, most recently from 8489104 to c226524 Compare August 8, 2025 22:20
@schrockn schrockn force-pushed the schrockn/organize-instance-with-public-methods branch 2 times, most recently from 9057a37 to 92c01b4 Compare August 8, 2025 22:35
@schrockn schrockn force-pushed the schrockn/instigator-mixin branch from c226524 to 5abb147 Compare August 8, 2025 22:35
@schrockn schrockn force-pushed the schrockn/organize-instance-with-public-methods branch from 92c01b4 to 19a89bf Compare August 8, 2025 22:38
@schrockn schrockn changed the base branch from schrockn/instigator-mixin to graphite-base/31633 August 8, 2025 23:04
@schrockn schrockn force-pushed the graphite-base/31633 branch from 638d75b to 22349c5 Compare August 8, 2025 23:04
@schrockn schrockn force-pushed the schrockn/organize-instance-with-public-methods branch from 19a89bf to dd05d66 Compare August 8, 2025 23:04
@graphite-app graphite-app bot changed the base branch from graphite-base/31633 to master August 8, 2025 23:05
@schrockn schrockn force-pushed the schrockn/organize-instance-with-public-methods branch from dd05d66 to 4a5770e Compare August 8, 2025 23:05
@schrockn schrockn force-pushed the schrockn/organize-instance-with-public-methods branch from 4a5770e to fd5beff Compare August 8, 2025 23:06
@schrockn schrockn mentioned this pull request Aug 8, 2025
Draft
6 tasks
@schrockn schrockn mentioned this pull request Aug 9, 2025
Merged
@schrockn Graphite App
Copy link
Member Author

schrockn commented Aug 9, 2025 •
edited
Loading

Merge activity

  • Aug 9, 12:49 AM UTC: A user started a stack merge that includes this pull request via Graphite.
  • Aug 9, 12:49 AM UTC: @schrockn merged this pull request with Graphite.

@schrockn schrockn merged commit 7a02864 into master Aug 9, 2025
5 of 6 checks passed
@schrockn schrockn deleted the schrockn/organize-instance-with-public-methods branch August 9, 2025 00:49
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