Skip to content

[dagster-airlift] Move around classes #29154

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

Open
wants to merge 1 commit into
base: dpeng817/fix_prop
Choose a base branch
from

Conversation

dpeng817
Copy link
Contributor

@dpeng817 dpeng817 commented Apr 9, 2025

Summary & Motivation

This file was getting too big and is about to get bigger. Move around non-serialized airflow representations to a different file.

How I Tested These Changes

Existing tests

@dpeng817 dpeng817 changed the title Move around classes [dagster-airlift] Move around classes Apr 9, 2025
@dpeng817 dpeng817 requested review from OwenKephart and prha April 9, 2025 18:56
@dpeng817 dpeng817 marked this pull request as ready for review April 9, 2025 18:56
Comment on lines 28 to 31
@property
def finished(self) -> bool:
from dagster_airlift.core.airflow_instance import TERMINAL_STATES
return self.state in TERMINAL_STATES
Copy link
Contributor

Choose a reason for hiding this comment

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

There's a circular import issue here: airflow_instance.py imports from runtime_representations.py and now runtime_representations.py imports from airflow_instance.py.

To resolve this, consider either:

  1. Moving TERMINAL_STATES to a common module that both can import
  2. Defining TERMINAL_STATES in runtime_representations.py and importing it in airflow_instance.py instead
  3. Passing TERMINAL_STATES as a parameter when creating DagRun instances

This will maintain clean dependency structure and prevent potential import-time issues.

Suggested change
@property
def finished(self) -> bool:
from dagster_airlift.core.airflow_instance import TERMINAL_STATES
return self.state in TERMINAL_STATES
@property
def finished(self) -> bool:
from dagster_airlift.core.constants import TERMINAL_STATES
return self.state in TERMINAL_STATES

Spotted by Diamond

Is this helpful? React 👍 or 👎 to let us know.

@dpeng817 dpeng817 force-pushed the dpeng817/fix_prop branch from 485ea40 to 1bb7131 Compare April 25, 2025 00:28
@dpeng817 dpeng817 force-pushed the dpeng817/runtime_representations branch from 6deb8db to c2a259c Compare April 25, 2025 00:28
This was referenced Apr 25, 2025
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.

3 participants