feat(serialization): implement truncation logic for rendered values#61878
feat(serialization): implement truncation logic for rendered values#61878potiuk merged 25 commits intoapache:mainfrom
Conversation
…n template fields Added a new function to truncate rendered values based on a specified maximum length, ensuring that truncation messages are prioritized. This functionality is integrated into the serialization of template fields, enhancing the handling of long strings in the system.
Modified the test for runtime task instances to dynamically retrieve the rendered fields from the mock supervisor communications, ensuring accurate assertions for the SetRenderedFields message type. This change enhances the robustness of the test by adapting to varying truncation formats based on configuration.
…ialization Replaced direct truncation messages in test assertions with calls to the new serialize_template_field function. This change ensures consistency in how large strings and objects are handled in the rendered task instance fields tests, leveraging the updated serialization logic for better clarity and maintainability.
SameerMesiah97
left a comment
There was a problem hiding this comment.
I think this needs some refactoring. The current implementation looks functional but it is not very clear. I think the algorithm should be as follows:
-
If
max_length <= 0,return "" -
Build the truncation message once and, if
max_lengthis smaller than its length, return it immediately. -
Determine the quoting strategy and compute the formatting overhead (prefix + quotes + suffix) and calculate available space.
-
If available space is less than
MIN_CONTENT_LENGTH, return the truncation message only. -
Otherwise slice the content to fit and construct the final string and ensure the result does not exceed
max_length, trimming it if necessary.
Currently, I think you are mixing 3, 4 and 5 when they should each be in disparate blocks of code. And this is resulting in needless duplication.
Also, I was just wondering why this has to be duplicated across 2 files? Perhaps, a maintainer/committer could weigh in on this but is it possible to have this in airflow.utils.helpers instead?
…dant safe wrapper
I implemented all your feedback and would love for you to review my pr again! |
pierrejeambrun
left a comment
There was a problem hiding this comment.
Looks good overall but would love another pair of eyes.
A couple of suggestions.
There was a problem hiding this comment.
Pull request overview
Introduces a shared truncate_rendered_value helper to standardize truncation of rendered template fields across Airflow core and the task SDK, and updates serialization/task-runner code paths to use it.
Changes:
- Added shared
truncate_rendered_valueutility (new shared package) and wired it into core + task SDK serialization. - Updated tests to cover truncation edge cases and adjusted existing assertions to accommodate the new behavior.
- Registered the new shared package in workspace/build configuration for core and task SDK.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| task-sdk/tests/task_sdk/execution_time/test_task_runner.py | Loosens rendered-fields assertion by deriving expected data from the mocked call |
| task-sdk/src/airflow/sdk/execution_time/task_runner.py | Replaces inline truncation formatting with truncate_rendered_value |
| task-sdk/src/airflow/sdk/_shared/template_rendering | Adds shared-module path link for template rendering utilities |
| task-sdk/pyproject.toml | Adds build mapping for shared template rendering package |
| shared/template_rendering/tests/template_rendering/test_truncate_rendered_value.py | Adds unit tests for truncation helper edge cases and exact output |
| shared/template_rendering/tests/template_rendering/init.py | Initializes shared template_rendering test package |
| shared/template_rendering/src/airflow_shared/template_rendering/init.py | Implements truncate_rendered_value |
| shared/template_rendering/pyproject.toml | Defines new shared workspace package metadata/build config |
| pyproject.toml | Registers new shared package in dev deps + workspace members |
| airflow-core/tests/unit/serialization/test_helpers.py | Adds unit tests for truncation helper and very-small max length behavior |
| airflow-core/tests/unit/models/test_renderedtifields.py | Updates expected truncation outputs to use serialize_template_field |
| airflow-core/src/airflow/utils/helpers.py | Adds module logger |
| airflow-core/src/airflow/serialization/helpers.py | Uses shared truncate_rendered_value instead of inline truncation |
| airflow-core/src/airflow/_shared/template_rendering | Adds shared-module path link for template rendering utilities |
| airflow-core/pyproject.toml | Adds build mapping for shared template rendering package |
shared/template_rendering/src/airflow_shared/template_rendering/__init__.py
Outdated
Show resolved
Hide resolved
shared/template_rendering/src/airflow_shared/template_rendering/__init__.py
Outdated
Show resolved
Hide resolved
shared/template_rendering/src/airflow_shared/template_rendering/__init__.py
Show resolved
Hide resolved
shared/template_rendering/src/airflow_shared/template_rendering/__init__.py
Show resolved
Hide resolved
shared/template_rendering/tests/template_rendering/test_truncate_rendered_value.py
Outdated
Show resolved
Hide resolved
|
@uranusjr — Could you please check whether your review feedback on this PR has been addressed? The author appears to have responded to your comments. If the concerns are resolved, please resolve the conversation threads. Thank you! |
|
@uranusjr — Could you please check whether your review feedback on this PR has been addressed? @imrichardwu appears to have responded to your comments. @imrichardwu, do you believe the reviewer's concerns have been resolved? If the concerns are resolved, please resolve the conversation threads. Thank you! |
Yes |
|
Merging it regardless. @uranusjr -> please check it retroactively if you stil have concerns. |
This pull request create truncate_rendered_value helper into the shared library so it can be used consistently across Airflow core and the task SDK without crossing package boundaries.
Truncation logic changes
Tests
Closes: #59877
Was generative AI tooling used to co-author this PR?
{pr_number}.significant.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.