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 support for XCom page in browse and task instance tab #44869

Merged
merged 6 commits into from
Dec 16, 2024

Conversation

tirkarthi
Copy link
Contributor

Add support to display XCom details for a task instance in a tab. Add a clipboard button next to the value so that user can copy the value easily. The PR also adds global XCom page under browse similar to Events page currently.

Notes for review and self :

  1. Events page is at src/pages/Events/ should I move this also to src/pages/XCom from current airflow/ui/src/pages/TaskInstance/XCom . I started with task instance and later realized it's easier to make this page global .
  2. Added run_id to the response for better filtering in retrieving value API in the global XCom page without any run id in the URL params.
  3. Each XCom entry needs an API call passing stringify: true to get the value. I have added a skeleton but is it possible to set some value to reuse the Skeleton from Table's component.
  4. Clipboard button was automatically generated from https://www.chakra-ui.com/docs/components/clipboard . Do I need to move these to individual files or disable the warnings?
  5. The value can be pretty printed and highlighted for JSON like legacy UI but can try it in another PR.
/home/karthikeyan/stuff/python/airflow/airflow/ui/src/components/ui/clipboard.tsx
   38:27  warning  Declare only one React component per file  react/no-multi-comp
   47:31  warning  Declare only one React component per file  react/no-multi-comp
   61:32  warning  Declare only one React component per file  react/no-multi-comp
   72:30  warning  Declare only one React component per file  react/no-multi-comp
   92:36  warning  Declare only one React component per file  react/no-multi-comp
  104:31  warning  Declare only one React component per file  react/no-multi-comp

Related #44667

image

image

XCom under browse page

image

@boring-cyborg boring-cyborg bot added the area:UI Related to UI/UX. For Frontend Developers. label Dec 12, 2024
@tirkarthi
Copy link
Contributor Author

Not sure why my pre-commit run and the one in CI are different.

@jscheffl
Copy link
Contributor

In the legacy UI in 2.10 I invested a bit of efforts making XCom better readable for users in #40640 . Especially for Dicts and lists. Can you re-apply such feature also for the new XCom display, such that dicts are not just dumped as text? (I know I generated a set of side-effects as bugs which needed to be fixed, so in the new UI we have the chance to make it "right the first time" also with a re-usable component for DAG Run Conf.

@tirkarthi
Copy link
Contributor Author

Thanks @jscheffl, it needs porting ReactJSON related component from legacy UI and would like to tackle it in a separate PR as noted in the PR description since it seems to take fair bit of work.

@jscheffl
Copy link
Contributor

Thanks @jscheffl, it needs porting ReactJSON related component from legacy UI and would like to tackle it in a separate PR as noted in the PR description since it seems to take fair bit of work.

Fair. As long as it is not forgotten :-D And does not need to be e exactly the same component as in legacy if there are better components nowadays.

Regarding (3) in your list above: In the past "Rest API" call the public XCom endpoint was used and for adding the "ReactJSON" I needed to add the "stringify" option as workarouns as the stringified version before used Python-style quotes which were not JSON parsable in the React / Javascript code. In the legacy this is really bad (in my view) and it would be better if in the new UI it is made "right" from the beginning. I wanted to prevent this in the past not having a breaking change in the REST API - but now would be the time with FastAPI anyway. So stringify might need to be set to false later back again to get a native object, else would be a waste to re-parse the string to JSON in TypeScript again after REST endpoint

@tirkarthi
Copy link
Contributor Author

Thanks @jscheffl, here is one rough attempt I can think of. Pass stringify: false in the API. Try JSON.stringify() and set highlight to true to use <SyntaxHighlighter />. On error just convert to string using String() and set highlight to be false to render it as normal <Text>. But this also means if there is __str__ implemented for the custom object in Python land then JS String() value might be different in case the object doesn't fit JSON.stringify.

image

@jscheffl
Copy link
Contributor

Thanks @jscheffl, here is one rough attempt I can think of. Pass stringify: false in the API. Try JSON.stringify() and set highlight to true to use <SyntaxHighlighter />. On error just convert to string using String() and set highlight to be false to render it as normal <Text>. But this also means if there is __str__ implemented for the custom object in Python land then JS String() value might be different in case the object doesn't fit JSON.stringify.

image

Yeah, but this is only rough :-) Then rather to it "right" in a follow-up 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.

I agree with moving this to a more global component like we did for events.

Also +1 on trying to use syntax highlighter. I wonder if we should update xcom entry to tell us what type the value is so the UI isn't guessing

airflow/ui/src/components/ui/clipboard.tsx Outdated Show resolved Hide resolved
airflow/ui/src/pages/TaskInstance/XCom/XCom.tsx Outdated Show resolved Hide resolved
airflow/ui/src/pages/TaskInstance/XCom/XCom.tsx Outdated Show resolved Hide resolved
airflow/ui/src/router.tsx Show resolved Hide resolved
@bbovenzi bbovenzi merged commit 2e2c656 into apache:main Dec 16, 2024
49 checks passed
LefterisXefteris pushed a commit to LefterisXefteris/airflow that referenced this pull request Jan 5, 2025
* Add support for XCom tab in Task Instance detail page.

* Add support for XCom page in browse section.

* Disable sorting since API doesn't support it.

* Fix run_id in xcom tests.

* Fix generated files.

* Refactor XCom page and XCom columns.
@tirkarthi tirkarthi mentioned this pull request Jan 6, 2025
2 tasks
got686-yandex pushed a commit to got686-yandex/airflow that referenced this pull request Jan 30, 2025
* Add support for XCom tab in Task Instance detail page.

* Add support for XCom page in browse section.

* Disable sorting since API doesn't support it.

* Fix run_id in xcom tests.

* Fix generated files.

* Refactor XCom page and XCom columns.
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.

3 participants