Fix batchoperator deferrable xcom links#64745
Conversation
When BatchOperator runs with deferrable=True, operator links (job definition, job queue, CloudWatch logs) were not being persisted as XCom values, causing ERROR-level logs when the UI tried to render them. This change: - Extracts link persistence logic into _persist_links() method - Calls _persist_links() from execute_complete() for deferrable tasks - Refactors monitor_job() to use the same method (DRY principle) Fixes XCom not found errors for: - batch_job_definition - batch_job_queue - cloudwatch_events
Added _persist_links(context) call in execute() method before deferring. This ensures operator links (batch_job_definition, batch_job_queue, cloudwatch_events) are persisted as XCom values immediately after job submission, making them available in the UI while the task is deferred.
- Persist placeholder None value for cloudwatch_events XCom when logs aren't available at submission time - Change log level from WARNING to INFO for missing CloudWatch logs - Prevents 'XCom not found' warning in UI while task is deferred - CloudWatch link will be populated when job completes and logs exist
|
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
|
There was a problem hiding this comment.
Pull request overview
This PR aims to ensure the AWS Batch operator’s extra UI links (job definition/queue/logs) are available for deferrable runs by persisting the related XComs before/around deferral and completion.
Changes:
- Persist operator extra-link XComs before deferring in
execute()and again inexecute_complete(). - Refactor link persistence logic into a new
_persist_links()helper. - Adjust CloudWatch log-link persistence and logging behavior.
|
Converting this to draft since there is unaddressed copilot feedback and failing tests. change this to ready for review once all feedback is addressed and the tests are green 🙂 |
- Reduce AWS API calls by 67% to avoid throttling - Fix missing job success check when awslogs disabled - Improve CloudWatch log error visibility - Handle edge cases in unit tests This addresses code review feedback on PR apache#64745.
|
I've implemented the feedback from Guthub copilot.
|
Address GitHub Copilot code review feedback: - Reduce AWS Batch API calls by 67% to prevent throttling - Remove dead code and redundant operations - Fix missing job success check when awslogs disabled - Improve CloudWatch error logging visibility - Use keyword arguments for better test compatibility This optimizes the deferrable execution path by reusing job descriptions and eliminating duplicate link persistence and CloudWatch log fetching. Addresses code review feedback on PR apache#64745. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…hub.com/kakatur/airflow into fix-batchoperator-deferrable-xcom-links
ferruzzi
left a comment
There was a problem hiding this comment.
Left two nipicks, otherwise LGTM
Changed AirflowException to ValueError for job_id checks in execute() and _persist_links() to be consistent with monitor_job() and follow the pattern that ValueError should be used for invalid state/parameters rather than runtime errors. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
@kakatur — There are 1 unresolved review thread on this PR from @ferruzzi. Could you either push a fix or reply in each thread explaining why the feedback doesn't apply? Once you believe the feedback is addressed, mark the thread as resolved so the reviewer isn't re-pinged needlessly. Thanks! Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this two-stage triage process so that our maintainers' limited time is spent where it matters most: the conversation with you. |
The test test_defer_but_failed_due_to_job_id_not_found expects an AirflowException to be raised when job_id is not found before deferral. Changed the validation to raise AirflowException instead of ValueError to match the test expectation and the method's documented behavior. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
The BatchOperator now has 6 raise AirflowException statements (down from 7 on main): - execute() method: raises AirflowException for missing job_id (1) - _persist_links() and monitor_job(): now raise ValueError instead (2 replacements) - Other existing raises remain unchanged (5 unchanged) Total: 7 - 2 + 1 = 6 Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…chOperator Changed job_id validation errors from AirflowException to ValueError since these represent invalid state rather than runtime failures. This follows the coding standard of using specific exceptions instead of the broad AirflowException. Updated test to expect ValueError and use pytest.raises match parameter for more specific error matching. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
…tions - Remove do_xcom_push guard from _persist_cloudwatch_link() to decouple UI link persistence from XCom return value behavior. CloudWatch links now appear in UI regardless of do_xcom_push setting, consistent with other operator links (BatchJobDetailsLink, BatchJobDefinitionLink, etc.) - Update _persist_links type annotations from bare dict to dict[str, Any] for better static type checking and codebase consistency - Remove test workaround that set do_xcom_push=True just to exercise link persistence, making test intent clearer Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
When using BatchOperator in deferrable mode with awslogs_enabled=True, logs were not being fetched from CloudWatch. Only the CloudWatch link was persisted in the UI, but the actual log messages were not printed to the task log. This created an inconsistency with non-deferrable mode where logs are streamed during execution. This commit adds log fetching functionality to execute_complete() that: - Fetches up to 10,000 completed log messages from CloudWatch - Prints each message to the task log for visibility - Works for both success and failure cases - Respects the awslogs_enabled flag - Gracefully handles missing log streams Test coverage expanded to cover all scenarios: - Success with logs enabled - Success with logs disabled - Failure with logs enabled - Failure with logs disabled (new test) All tests ensure CloudWatch links are persisted regardless of awslogs_enabled setting, maintaining UI link access in all cases. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Add robust error handling and performance optimizations based on Copilot review feedback: 1. Exception handling for CloudWatch API failures - Wrap get_last_log_messages() in try/except - Log warning with traceback instead of failing task - Ensures task continues even if log retrieval fails - Matches background thread error handling pattern 2. Performance optimization for XCom disabled case - Add early return when do_xcom_push=False in _persist_cloudwatch_link() - Prevents unnecessary get_job_all_awslogs_info() API call - BaseAwsLink.persist() already checks this, now we skip earlier - Saves 1 API call when XCom push disabled 3. Documentation accuracy - Update execute() docstring to document both exception types - ValueError: raised when job_id is not found - AirflowException: raised for job submission/execution failures - Matches actual code behavior throughout operator All changes validated: - 23/23 unit tests pass - All static checks pass (ruff, prek, mypy) - Custom exception handling test verifies graceful degradation - No breaking changes, fully backward compatible Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions. |
When BatchOperator runs with deferrable=True, operator links (job definition, job queue, CloudWatch logs) were not being persisted as XCom values, causing ERROR-level logs when the UI tried to render them. This change: - Extracts link persistence logic into _persist_links() method - Calls _persist_links() from execute_complete() for deferrable tasks - Refactors monitor_job() to use the same method (DRY principle)
Problem
Fixes missing CloudWatch links when
BatchOperatorruns in deferrable mode. Previously, CloudWatch log links were not persisted to the Airflow UI when tasks deferred to the triggerer.When
deferrable=True, the operator would defer to the triggerer after submitting the Batch job, but the CloudWatch link persistence logic only ran in the non-deferrable path. This meant users couldn't easily access CloudWatch logs from the Airflow UI for deferred tasks, even though the logs existed in AWS.Additionally, the fast-path completion scenario (when a job completes before deferring) and failure cases also lacked proper CloudWatch link persistence.
How to Reproduce
BatchOperatorusingdeferrable=TrueSolution
Core fix: Added CloudWatch link persistence to deferrable execution paths by calling link persistence logic in
execute_complete()after the triggerer signals job completion. (commits dca1ffa, 42bdfd9, 0364247)Comprehensive coverage: Extended fix to handle all execution scenarios:
execute()Code improvements: Extracted
_persist_cloudwatch_link()helper method to eliminate code duplication and ensure consistent link persistence across all code paths. (commits 0a6cbad, 3f17702)Additional Changes
Optimizations based on Copilot Comments:
wait_for_job()call inexecute_complete()(commit 0a6cbad)if not context or "ti" not in context) to skip CloudWatch API calls when context lacks task instance (commit 0058f4d)if not self.do_xcom_push: returnto respectdo_xcom_pushsetting instead of always attempting to push CloudWatch links (commit 3b78fc9)execute_complete()to setjob_idbefore persisting CloudWatch link, ensuring links work on both success and failure paths (commit 3b78fc9)if self.awslogs_enabled, then reverted to preserve UI links in all cases (commits 0058f4d, 4dc6b9d)Test and Type Fixes:
get_job_all_awslogs_info()instead of expecting multipleget_job_description()calls, enabled xcom_push, added proper dict context (commit e9b8d80)get_job_all_awslogs_info,check_job_success) and enabled xcom_push with proper context (commit e9b8d80)AirflowExceptionwithValueErrorfor missing job_id, added type guard for job_id in_persist_cloudwatch_link()(commit 9b210ef)How to Verify the Fix
Manual Testing:
BatchOperatorusingdeferrable=Trueandawslogs_enabled=TrueAutomated Testing:
uv run --project providers/amazon pytest providers/amazon/tests/unit/amazon/aws/operators/test_batch.py::TestBatchOperator::test_execute_without_failures -xvsuv run --project providers/amazon pytest providers/amazon/tests/unit/amazon/aws/operators/test_batch.py::TestBatchOperator::test_execute_complete_success_with_logs -xvsExpected Behavior:
awslogs_enabledsetting (flag only controls log streaming during wait, not link persistence)execute_complete()for better performanceWas generative AI tooling used to co-author this PR?
Generated-by: Claude Code following the guidelines