Skip to content

Consolidate DagsterInstance architecture: merge mixins and domains into Methods classes #31634

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.

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

Merged
merged 3 commits into from
Aug 9, 2025

Conversation

schrockn
Copy link
Member

@schrockn schrockn commented Aug 7, 2025

Summary & Motivation

Consolidates DagsterInstance architecture from a 3-layer structure (Instance → Mixins → Domains) to a streamlined 2-layer structure (Instance → Methods). This eliminates the delegation overhead and complexity while preserving all existing functionality and APIs.

Key Changes

  • Architecture Simplification: Replaced mixin+domain pattern with single "Methods" classes that DagsterInstance inherits from directly
  • Performance Improvement: Eliminated delegation layer for all domain operations
  • Code Organization: All Methods classes organized in new methods/ folder for clear separation of concerns
  • API Preservation: All public methods and signatures remain unchanged

Completed Domain Consolidations

  • SettingsMethods - Settings and configuration management
  • StorageMethods - Storage optimization and maintenance operations
  • DaemonMethods - Daemon heartbeat and status monitoring
  • RunLauncherMethods - Run submission and coordination
  • EventMethods - Event logging and retrieval
  • SchedulingMethods - Schedule and sensor management
  • AssetMethods - Asset operations and materialization tracking

How I Tested These Changes

Existing test suite passes with all domain consolidations. Each Methods class preserves identical functionality to the original mixin+domain implementation.

Changelog

Internal architectural improvement - no user-facing API changes.

@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/merge-mixins-implementatinos branch from dd1f5d3 to 5dbdcae Compare August 7, 2025 22:11
@schrockn schrockn force-pushed the schrockn/organize-instance-with-public-methods branch from 97d7399 to b397919 Compare August 7, 2025 23:06
@schrockn schrockn force-pushed the schrockn/merge-mixins-implementatinos branch 2 times, most recently from 749437f to b79f160 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/merge-mixins-implementatinos branch from b79f160 to 377eaa3 Compare August 8, 2025 11:49
@schrockn schrockn force-pushed the schrockn/merge-mixins-implementatinos branch from 377eaa3 to a4ab586 Compare August 8, 2025 12:49
@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/merge-mixins-implementatinos branch from a4ab586 to a310cab Compare August 8, 2025 13:18
@schrockn schrockn force-pushed the schrockn/organize-instance-with-public-methods branch from d6a13ef to 8fe47c7 Compare August 8, 2025 14:43
@schrockn schrockn force-pushed the schrockn/merge-mixins-implementatinos branch from a310cab to 7dd60b6 Compare August 8, 2025 14:43
@schrockn schrockn force-pushed the schrockn/merge-mixins-implementatinos branch from 7dd60b6 to de1c931 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/merge-mixins-implementatinos branch from de1c931 to 15a8cc8 Compare August 8, 2025 17:44
@schrockn schrockn force-pushed the schrockn/organize-instance-with-public-methods branch from ff1f56f to 9057a37 Compare August 8, 2025 22:20
@schrockn schrockn force-pushed the schrockn/merge-mixins-implementatinos branch 2 times, most recently from 5fa42b5 to 8103da3 Compare August 8, 2025 22:35
@schrockn schrockn force-pushed the schrockn/organize-instance-with-public-methods branch from 9057a37 to 92c01b4 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 force-pushed the schrockn/merge-mixins-implementatinos branch from 8103da3 to 62fda75 Compare August 8, 2025 22:38
Copy link
Member Author

schrockn commented Aug 8, 2025

@OwenKephart we need the AssetMethods syntax because of multiple inheritance. Otherwise we're relying on MRO which is fragile.

@schrockn schrockn force-pushed the schrockn/organize-instance-with-public-methods branch 2 times, most recently from dd05d66 to 4a5770e Compare August 8, 2025 23:05
@schrockn schrockn force-pushed the schrockn/merge-mixins-implementatinos branch from 36a5889 to 53e4379 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 force-pushed the schrockn/merge-mixins-implementatinos branch from 53e4379 to 64586b8 Compare August 8, 2025 23:06
@schrockn schrockn force-pushed the schrockn/merge-mixins-implementatinos branch from 64586b8 to baccbd4 Compare August 9, 2025 00:01
@schrockn schrockn mentioned this pull request Aug 9, 2025
@schrockn schrockn changed the base branch from schrockn/organize-instance-with-public-methods to graphite-base/31634 August 9, 2025 00:49
@schrockn schrockn force-pushed the schrockn/merge-mixins-implementatinos branch from baccbd4 to 87e3221 Compare August 9, 2025 00:49
@schrockn schrockn force-pushed the graphite-base/31634 branch from 0b6e577 to 7a02864 Compare August 9, 2025 00:49
@schrockn schrockn force-pushed the schrockn/merge-mixins-implementatinos branch from 87e3221 to c3a78bf Compare August 9, 2025 00:50
@schrockn schrockn changed the base branch from graphite-base/31634 to master August 9, 2025 00:50
@schrockn schrockn force-pushed the schrockn/merge-mixins-implementatinos branch from c3a78bf to 06e3479 Compare August 9, 2025 00:50
@schrockn schrockn requested a review from a team as a code owner August 9, 2025 11:41
Copy link

github-actions bot commented Aug 9, 2025

Deploy preview for dagster-docs ready!

Preview available at https://dagster-docs-d769zu5ux-elementl.vercel.app
https://schrockn-merge-mixins-implementatinos.archive.dagster-docs.io

Direct link to changed pages:

Copy link
Member Author

schrockn commented Aug 9, 2025

Merge activity

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

@schrockn schrockn merged commit 6a89d57 into master Aug 9, 2025
8 checks passed
@schrockn schrockn deleted the schrockn/merge-mixins-implementatinos branch August 9, 2025 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants