Skip to content

refactor(github): convert github utils to TaskFlow @task and chain - #306

Open
RexBearIU wants to merge 1 commit into
mainfrom
maxtext/user/jackyf/refactor-github-utils
Open

refactor(github): convert github utils to TaskFlow @task and chain#306
RexBearIU wants to merge 1 commit into
mainfrom
maxtext/user/jackyf/refactor-github-utils

Conversation

@RexBearIU

@RexBearIU RexBearIU commented Jul 27, 2026

Copy link
Copy Markdown

Description

Refactored xlml/utils/github.py and dags/multipod/maxtext_e2e_tests.py to follow standard Airflow 2.x TaskFlow conventions:

  • Converted validate_git_trigger and fire_github_callback to TaskFlow tasks using @task.
  • Replaced **context with get_current_context().
  • Replaced >> bitshift operators with chain().
  • Standardized DAG parameter names to commit_sha and github_token (with backward-compatible fallbacks).
  • Added unit tests in xlml/utils/github_test.py.

Tests

  1. Pre-commit & Unit Tests:
pre-commit run --all-files
python3 -m unittest xlml/utils/github_test.py
  1. Cloud Composer Integration Tests:
    Uploaded DAG and utility files to test environment jackyf-test and verified task structure and import errors:
gcloud storage cp dags/multipod/maxtext_e2e_tests.py gs://us-east1-jackyf-test-3fc56b08-bucket/dags/multipod/maxtext_e2e_tests.py
gcloud storage cp xlml/utils/github.py gs://us-east1-jackyf-test-3fc56b08-bucket/dags/xlml/utils/github.py
gcloud composer environments run jackyf-test --location us-east1 --project cloud-ml-auto-solutions dags list-import-errors
gcloud composer environments run jackyf-test --location us-east1 --project cloud-ml-auto-solutions tasks list -- maxtext_e2e_tests

Output from Composer:

  • dags list-import-errors: No import errors for maxtext_e2e_tests.
  • tasks list: Successfully listed all tasks:
    • <Task(_TaskDecorator): fire_github_callback_post_training>
    • <Task(_TaskDecorator): fire_github_callback_pre_training>
    • <Task(TriggerDagRunOperator): trigger_tpu_post_training>
    • <Task(TriggerDagRunOperator): trigger_tpu_pre_training>
    • <Task(_TaskDecorator): validate_git_trigger>

Checklist

  • I have performed a self-review of my code.
  • I have necessary comments in my code, particularly in hard-to-understand areas.
  • I have run one-shot tests and provided workload links above if applicable.
  • I have made or will make corresponding changes to the doc if needed.

@RexBearIU
RexBearIU force-pushed the maxtext/user/jackyf/refactor-github-utils branch from d31880b to afb1667 Compare July 27, 2026 09:22
Comment thread dags/multipod/maxtext_e2e_tests.py
Comment thread xlml/utils/github.py
Comment thread xlml/utils/github.py

def validate_git_trigger(**context):
@task
def validate_git_trigger() -> None:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

how about checking external_trigger instead

  from airflow.operators.python import get_current_context
  from airflow.models.dagrun import DagRun

  context = get_current_context()
  assert dag_run is not None, "..."
  dag_run: DagRun = context.get("dag_run")
  assert dag_run is not None, "..."
  assert dag_run.external_trigger == True, "..."

@RexBearIU RexBearIU Jul 28, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Updated! validate_git_trigger now checks dag_run.external_trigger to ensure manual runs from the Airflow UI are prevented.

Comment thread xlml/utils/github.py Outdated

def fire_github_callback(test_type: str | None = None, **context):
@task
def fire_github_callback(test_type: str | None = None) -> None:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

From a decoupling perspective, repo, token, sha (or commit_sha), github_run_id, and the request body (L75–L78) are tightly coupled with a specific DAG.

Therefore, as a shared utility, this function should be more generalized so that any DAG can trigger the GitHub Dispatches API (rename this function also make more sense) — meaning these parameters need to be passed as arguments.

Alternatively, moving the entire function back into the DAG file as a private helper would be simpler and eliminate any decoupling issues.

@RexBearIU RexBearIU Jul 28, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Thanks for the feedback! We updated trigger_github_repository_dispatch in xlml/utils/github.py to be fully decoupled as a shared utility. It accepts standard GitHub Repository Dispatch API arguments (repo, token, event_type, client_payload) directly as explicit inputs. If explicit arguments are omitted, it gracefully falls back to reading Airflow DAG params from context for backward compatibility. dags/multipod/maxtext_e2e_tests.py has been updated to use trigger_github_repository_dispatch as the primary function name.

@RexBearIU
RexBearIU force-pushed the maxtext/user/jackyf/refactor-github-utils branch from 15a0ce6 to a445997 Compare July 28, 2026 02:32
@RexBearIU
RexBearIU force-pushed the maxtext/user/jackyf/refactor-github-utils branch from a445997 to 49b52e1 Compare July 28, 2026 02:33
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