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

Don't crash if operator_extra_links is a property #25332

Closed
wants to merge 1 commit into from

Conversation

uranusjr
Copy link
Member

See issues like #25243, #24676, and #25215.

This makes operators that implement this as an instance property compatible for task-mapping. We still can't show those extra links (since they are only available against a concrete task), but at least they are runnable.

This makes operators that implement this as an instance property
compatible for task-mapping. We still can't show those extra links
(since they are only available against a concrete task), but at least
they are runnable.
Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

I thin k we should make a note about it somewhere in BaseOperatorLink docs and add warning.

We might fix it permanently in the future, but it might take some time. But if we cherry-pick that one to 2.3.4 or 2.4.0 will not have a long-term base operator link treatment improved, this will be an equal (and even worse) surprise to our users if it is not documented.

Right now, the tasks crash and it's easy to see/report, but when the user will stop seeing the links where it is supposed to be, still an issue they will open and we need to explain to them it is intended (with docs link explaining it).

Also a Warning in log in this case sounds very appropriate, as this is pretty surprising behaviour.

We can always update the docs later when we fix it properly and remove the warning - but this commit should have them IMHO.

@josh-fell
Copy link
Contributor

+1 on a warning or some active notification (maybe even a grayed out link button and a tooltip in the Task Instance modal) so functionality isn't "missing" silently.

@uranusjr
Copy link
Member Author

What do y’all think should be a good way to communicate this? This needs to be done during the DAG parcing phase so a log might not be as noticable, but showing a DagWarning feels a bit excessive to me.

@Taragolis
Copy link
Contributor

BTW, does any of Extra Links work on mapped tasks? Or it just only on my side it won't work at all on mapped task.

@uranusjr
Copy link
Member Author

What does not work for you? Open a new issue for that, please.

@Taragolis
Copy link
Contributor

@uranusjr #25360

@potiuk
Copy link
Member

potiuk commented Jul 28, 2022

What do y’all think should be a good way to communicate this? This needs to be done during the DAG parcing phase so a log might not be as noticable, but showing a DagWarning feels a bit excessive to me.

UI information (exactly as @josh-fell mentioned) where we see that links are there but not working (grayed out) with comment. This could be a separate PR @bbovenzi @pierrejeambrun ?

@bbovenzi
Copy link
Contributor

UI information (exactly as @josh-fell mentioned) where we see that links are there but not working (grayed out) with comment. This could be a separate PR @bbovenzi @pierrejeambrun ?

I like Josh's suggestion. We can do that if we add an extra error case in the webserver

@uranusjr
Copy link
Member Author

So essentially, we invent a “fake” extra link class, and use that if the mapped operator fails to access the real extra link attribute, and return some warning information from the webserver when the special extra link class is accessed?

@potiuk
Copy link
Member

potiuk commented Jul 29, 2022

So essentially, we invent a “fake” extra link class, and use that if the mapped operator fails to access the real extra link attribute, and return some warning information from the webserver when the special extra link class is accessed?

If we can't do "real" extra link easily, that's kinda the best way to implement it.

@uranusjr uranusjr marked this pull request as draft August 3, 2022 10:00
@github-actions
Copy link

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions.

@github-actions github-actions bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Sep 18, 2022
@github-actions github-actions bot closed this Sep 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:dynamic-task-mapping AIP-42 stale Stale PRs per the .github/workflows/stale.yml policy file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants