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

Add triggerer info and extra links section to task instance details tab. #46074

Merged
merged 5 commits into from
Feb 5, 2025

Conversation

tirkarthi
Copy link
Contributor

@tirkarthi tirkarthi commented Jan 26, 2025

Add triggerer info on deferred state and extra links section to task instance details like legacy UI .

Closes #45614

image

@boring-cyborg boring-cyborg bot added the area:UI Related to UI/UX. For Frontend Developers. label Jan 26, 2025
@eladkal
Copy link
Contributor

eladkal commented Jan 26, 2025

Can you verify extra link works also for mapped tasks? We have #43757 open for the old ui

@tirkarthi
Copy link
Contributor Author

The API endpoint to fetch extra links doesn't accept map_index so I guess it's not yet supported and should probably be hidden for mapped tasks until implemented like legacy UI.

@shubhamraj-git
Copy link
Contributor

@tirkarthi Can you build using #46107 and check your changes for mapped tasks ones? This should fix it.

@tirkarthi
Copy link
Contributor Author

Thanks @shubhamraj-git , I had a similar patch locally and I think the PR should work. I will wait for the changes to be merged to rebase here or take the frontend changes in a new PR after the PR.

Copy link
Contributor

@bbovenzi bbovenzi left a comment

Choose a reason for hiding this comment

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

Nice!

I agree, let's add a check for mapIndex for now with a TODO comment to remind us to add in support for mapped tasks soon

@tirkarthi tirkarthi force-pushed the ti-triggerer-extra-links branch from 9fe2b33 to b826122 Compare January 30, 2025 03:13
@pierrejeambrun
Copy link
Member

Is that working locally in breeze ? Are you using another executor because Task SDK does not support extra links so I expect errors there.

@shubhamraj-git
Copy link
Contributor

Is that working locally in breeze ? Are you using another executor because Task SDK does not support extra links so I expect errors there.

@pierrejeambrun Yaa possible, I think --executor CeleryExecutor will work.

@tirkarthi
Copy link
Contributor Author

@pierrejeambrun I am using a virtual environment to pull from git and do uv pip install . with airflow scheduler and airflow dag-processor in different shells. I am using extra links as per the docs and using it on BashOperator from from airflow.providers.standard.operators.bash import BashOperator . So I guess I am not using the task sdk here.

https://airflow.apache.org/docs/apache-airflow/stable/howto/define-extra-link.html

@potiuk
Copy link
Member

potiuk commented Feb 1, 2025

@pierrejeambrun I am using a virtual environment to pull from git and do uv pip install . with airflow scheduler and airflow dag-processor in different shells. I am using extra links as per the docs and using it on BashOperator from from airflow.providers.standard.operators.bash import BashOperator . So I guess I am not using the task sdk here.

https://airflow.apache.org/docs/apache-airflow/stable/howto/define-extra-link.html

See https://github.com/apache/airflow/blob/main/contributing-docs/07_local_virtualenv.rst - when we explain how to work with the new virtualenv model where we have mutliple packages: uv sync --all-extras and uv sync --extra celery are your friends as they are using the uv workspace feature which installs all internal airflow packages as needed and all necessary dependencies for the provider extras you specify.

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, a few suggestions.

@eladkal
Copy link
Contributor

eladkal commented Feb 4, 2025

Are we happy with having it in the details tab?
I have to say, I click on the details tab rarely (for the info that we are having there).
On the other hand, I really need the extra link when I view failure logs.

I am not UX expert but I think we need better place for the extra links than the details tab.

@tirkarthi
Copy link
Contributor Author

In Airflow 2 usually clicking on the task instance in grid leads to the task instance details tab. In Airflow 3 logs are made as the default tab. I am fine with moving extra links there if okay.

@tirkarthi tirkarthi force-pushed the ti-triggerer-extra-links branch from b826122 to 68f5fce Compare February 4, 2025 11:58
@eladkal
Copy link
Contributor

eladkal commented Feb 4, 2025

In Airflow 2 usually clicking on the task instance in grid leads to the task instance details tab. In Airflow 3 logs are made as the default tab. I am fine with moving extra links there if okay.

Getting to the extra links in 2.10 is very inconvenient to my taste.
I would rather the links to be folded/expanded above the logs/events/xcom tabs, but I leave this choice to the UX experts :)

@tirkarthi tirkarthi force-pushed the ti-triggerer-extra-links branch from 68f5fce to 3646bf3 Compare February 5, 2025 11:38
@pierrejeambrun pierrejeambrun merged commit 452f307 into apache:main Feb 5, 2025
35 checks passed
@@ -118,7 +118,7 @@ export const Details = () => {
</Table.Row>
{dagRun.external_trigger ? (
<Table.Row>
<Table.Cell>Externally Trigger Source</Table.Cell>
<Table.Cell>External Trigger Source</Table.Cell>
Copy link
Member

Choose a reason for hiding this comment

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

@pierrejeambrun @bbovenzi We should nix this "external" word here, as some of the values are "timetable", or "asset" which aren't external.

Copy link
Contributor

Choose a reason for hiding this comment

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

Do we always want to show dagRun.triggered_by then? instead of only on external triggers?

Copy link
Member

@pierrejeambrun pierrejeambrun Feb 7, 2025

Choose a reason for hiding this comment

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

While we decide on that we can maybe just remove the External word from here as this is misleading:
#46559

insomnes pushed a commit to insomnes/airflow that referenced this pull request Feb 6, 2025
…ab. (apache#46074)

* Add triggerer info and extra links section to task instance details tab.

* Change button variant from default solid to surface to make it lighter.

* Add map_index support.

* PR comments refactor.

* Add section name for task instance details.
niklasr22 pushed a commit to niklasr22/airflow that referenced this pull request Feb 8, 2025
…ab. (apache#46074)

* Add triggerer info and extra links section to task instance details tab.

* Change button variant from default solid to surface to make it lighter.

* Add map_index support.

* PR comments refactor.

* Add section name for task instance details.
ambika-garg pushed a commit to ambika-garg/airflow that referenced this pull request Feb 17, 2025
…ab. (apache#46074)

* Add triggerer info and extra links section to task instance details tab.

* Change button variant from default solid to surface to make it lighter.

* Add map_index support.

* PR comments refactor.

* Add section name for task instance details.
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 38 : Add extra links to task instance page
7 participants