Skip to content
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

AIP-65 | Add dag versions to DAGRunResponse #46484

Conversation

jason810496
Copy link
Member

@jason810496 jason810496 commented Feb 5, 2025

depends on: #46565
closes: #45995


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@boring-cyborg boring-cyborg bot added the area:UI Related to UI/UX. For Frontend Developers. label Feb 5, 2025
@jason810496
Copy link
Member Author

Hi @pierrejeambrun,

Could you help me confirm whether the current implementation of airflow/api_fastapi/core_api/routes/public/dag_run.py aligns with the issue? I'm not entirely sure if this is the best approach to retrieve DAG versions for each dag_run.

If there are no concerns with the current implementation (my main concern is performance), I’ll move some common queries to the api_fastapi.common.db module and complete the /recent_dag_runs UI endpoint. (/recent_dag_runs also depends on DAGRunResponse.)

Thanks!

Copy link
Member

@pierrejeambrun pierrejeambrun left a comment

Choose a reason for hiding this comment

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

Thanks, a few comments.

@jason810496
Copy link
Member Author

Hi @pierrejeambrun,

Do I need to configure DagBundlesManager explicitly to set ("dag_processor", "dag_bundle_config_list")?
I'm encountering the following error when running the test_dag_run test:

   |   File "/opt/airflow/airflow/api_fastapi/core_api/datamodels/dag_versions.py", line 43, in bundle_url
    |     return DagBundlesManager().view_url(self.bundle_name, self.bundle_version)
    |   File "/opt/airflow/airflow/dag_processing/bundles/manager.py", line 144, in view_url
    |     bundle = self.get_bundle(name, version)
    |   File "/opt/airflow/airflow/dag_processing/bundles/manager.py", line 130, in get_bundle
    |     raise ValueError(f"Requested bundle '{name}' is not configured.")
    | ValueError: Requested bundle 'dag_maker' is not configured.

I checked test_dag_versions.py but didn’t find any additional configurations for DagBundlesManager. However, in test_dag_versions, DagBundlesManager._bundle_config does contain the dag_maker key, whereas in test_dag_run, it does not.

Comparing conf.getjson("dag_processor", "dag_bundle_config_list") in both tests:

test_versions output:

bundle_name: another_bundle_name, bundle_version: some_commit_hash
backends
[{'classpath': 'airflow.dag_processing.bundles.git.GitDagBundle',
  'kwargs': {'refresh_interval': 0, 'subdir': 'dags', 'tracking_ref': 'main'},
  'name': 'dag_maker'},
 {'classpath': 'airflow.dag_processing.bundles.git.GitDagBundle',
  'kwargs': {'refresh_interval': 0, 'subdir': 'dags', 'tracking_ref': 'main'},
  'name': 'another_bundle_name'}]

test_dag_run output:

bundle_name: dag_maker, bundle_version: None
backends
[{'classpath': 'airflow.dag_processing.bundles.local.LocalDagBundle',
  'kwargs': {},
  'name': 'dags-folder'}]
[2025-02-19T08:25:53.289+0000] {manager.py:101} INFO - DAG bundles loaded: dags-folder, example_dags
_bundle_config
{'dags-folder': (<class 'airflow.dag_processing.bundles.local.LocalDagBundle'>,
                 {}),
 'example_dags': (<class 'airflow.dag_processing.bundles.local.LocalDagBundle'>,
                  {'path': '/opt/airflow/airflow/example_dags'})}
FAILED

I even tried explicitly setting ("dag_processor", "dag_bundle_config_list") using conf_vars, but the configuration still does not include dag_maker for bundle_name.

Would appreciate any guidance on this—thanks!

@pierrejeambrun
Copy link
Member

Yes bundles need to be configured.

You can take a look at the make_dag_with_multiple_versions which does that. (You need a configuration + git connection for it to work in your test)

@jason810496
Copy link
Member Author

Yes bundles need to be configured.

You can take a look at the make_dag_with_multiple_versions which does that. (You need a configuration + git connection for it to work in your test)

Big thanks to @pierrejeambrun! You saved my day. I'm still fixing tests locally, then the PR will be ready.

@jason810496 jason810496 force-pushed the feature/AIP-65/Add-dag-versions-to-DagRunResponse branch 2 times, most recently from 411ef41 to 7b8281c Compare February 21, 2025 02:28
@jason810496 jason810496 changed the title [WIP] AIP-65 | Add dag versions to DAGRunResponse AIP-65 | Add dag versions to DAGRunResponse Feb 21, 2025
@jason810496 jason810496 force-pushed the feature/AIP-65/Add-dag-versions-to-DagRunResponse branch from a53c21c to 512a519 Compare February 21, 2025 06:19
Copy link
Member

@pierrejeambrun pierrejeambrun left a comment

Choose a reason for hiding this comment

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

Nice. Code looks good. Just a few minor comments, non blocking

We just need to rebase and rerun the CI I think there was some unrelated failure.

@jason810496
Copy link
Member Author

jason810496 commented Feb 21, 2025

Nice. Code looks good. Just a few minor comments, non blocking

We just need to rebase and rerun the CI I think there was some unrelated failure.

For the test CLI that fail is a not flaky test. It's caused by DagBundleManager logging a new line, and break the json.loads ( currently out of laptop, I will push the fix tomorrow )

@jason810496 jason810496 force-pushed the feature/AIP-65/Add-dag-versions-to-DagRunResponse branch from 512a519 to 5f0df91 Compare February 22, 2025 10:40
Copy link
Member

@pierrejeambrun pierrejeambrun left a comment

Choose a reason for hiding this comment

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

Looks good, some tests need fixing.

@jason810496
Copy link
Member Author

Looks good, some tests need fixing.

The CI failure is due to sqlalchemy.exc.OperationalError: (sqlite3.OperationalError) database is locked. We may need to retrigger the SQLite test ~

@pierrejeambrun
Copy link
Member

The CI failure is due to sqlalchemy.exc.OperationalError: (sqlite3.OperationalError) database is locked. We may need to retrigger the SQLite test.

Ok, I just restarted the failed step.

@pierrejeambrun
Copy link
Member

Failing again, that's not flaky

@pierrejeambrun
Copy link
Member

I have identified the problem, I will fix it in another PR that's not directly related to this one.

@pierrejeambrun
Copy link
Member

This should fix your CI issue #47016

@pierrejeambrun pierrejeambrun force-pushed the feature/AIP-65/Add-dag-versions-to-DagRunResponse branch from 5f0df91 to 657ba82 Compare February 25, 2025 14:30
@pierrejeambrun
Copy link
Member

Rebased, conflict solved.

@pierrejeambrun pierrejeambrun merged commit 8379804 into apache:main Feb 25, 2025
45 checks passed
ambika-garg pushed a commit to ambika-garg/airflow that referenced this pull request Feb 28, 2025
* AIP-65 | Add dag versions to DAGRunResponse

* fixup! Fix configure_git_connection_for_dag_bundle fixture import

* fixup! Fix dag_versions property

* Fix test_cli_assets_materialize

* Fix nits in test_dag_run

* Fix conflicts, fix CI

---------

Co-authored-by: pierrejeambrun <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:UI Related to UI/UX. For Frontend Developers.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AIP-65 | Add dag versions to DagRunResponse
3 participants