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

Fix detached test tasks names so they do not exceed 250 chars #1464

Merged
merged 5 commits into from
Jan 15, 2025
Merged

Conversation

tatiana
Copy link
Collaborator

@tatiana tatiana commented Jan 14, 2025

Since we introduced detached test tasks in #1433 (released in 1.8.0), users started facing issues due to very long task names exceeding Airflow's limits.

Example of Python traceback reported by user:

     Traceback (most recent call last):
      File "/home/airflow/.local/lib/python3.12/site-packages/airflow/models/baseoperator.py", line 968, in __init__
        validate_key(task_id)
      File "/home/airflow/.local/lib/python3.12/site-packages/airflow/utils/helpers.py", line 55, in validate_key
        raise AirflowException(f"The key has to be less than {max_length} characters")
    airflow.exceptions.AirflowException: The key has to be less than 250 characters

This PR fixes this issue. In case the name exceeds Airflow's limit (250 ATM), it will name the detached test using:

  • "detached_{incremental unique number}_test"

We also considered naming the new test using:

  • "parent1_parent2_..._test" - but that may not solve the issue, especially in circumstances where the same parents may have multiple detached nodes. Perhaps in future, we could bundle them in a single task.

Closes: #1440

@dosubot dosubot bot added the size:M This PR changes 30-99 lines, ignoring generated files. label Jan 14, 2025
Copy link

cloudflare-workers-and-pages bot commented Jan 14, 2025

Deploying astronomer-cosmos with  Cloudflare Pages  Cloudflare Pages

Latest commit: d8152c0
Status: ✅  Deploy successful!
Preview URL: https://d342de62.astronomer-cosmos.pages.dev
Branch Preview URL: https://issue-1440.astronomer-cosmos.pages.dev

View logs

@dosubot dosubot bot added the area:execution Related to the execution environment/mode, like Docker, Kubernetes, Local, VirtualEnv, etc label Jan 14, 2025
Copy link

netlify bot commented Jan 14, 2025

Deploy Preview for sunny-pastelito-5ecb04 canceled.

Name Link
🔨 Latest commit d8152c0
🔍 Latest deploy log https://app.netlify.com/sites/sunny-pastelito-5ecb04/deploys/6787c89c1568b40008962cab

Copy link

codecov bot commented Jan 14, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.95%. Comparing base (c359813) to head (d8152c0).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1464   +/-   ##
=======================================
  Coverage   96.94%   96.95%           
=======================================
  Files          73       73           
  Lines        4359     4370   +11     
=======================================
+ Hits         4226     4237   +11     
  Misses        133      133           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@tatiana tatiana mentioned this pull request Jan 14, 2025
Copy link
Contributor

@pankajkoti pankajkoti left a comment

Choose a reason for hiding this comment

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

LGTM. Minor suggestion in-line to avoid initialising list in the args.

cosmos/airflow/graph.py Outdated Show resolved Hide resolved
cosmos/airflow/graph.py Show resolved Hide resolved
Since we introduced detached test tasks in #1433 (released in 1.8.0) users started facing issues due to very long task names exceeding Airflow's limits.

Example of stacktrace reported by user:
```
 Traceback (most recent call last):
  File "/home/airflow/.local/lib/python3.12/site-packages/airflow/models/baseoperator.py", line 968, in __init__
    validate_key(task_id)
  File "/home/airflow/.local/lib/python3.12/site-packages/airflow/utils/helpers.py", line 55, in validate_key
    raise AirflowException(f"The key has to be less than {max_length} characters")
airflow.exceptions.AirflowException: The key has to be less than 250 characters
```

This PR fixes this issue. In case the name exceeds Airflow's limit (250 ATM), it will name the detached test using:
- "detached_{incremental unique number}_test"

Closes: #1440
@tatiana tatiana merged commit 42d30a1 into main Jan 15, 2025
67 checks passed
@tatiana tatiana deleted the issue-1440 branch January 15, 2025 15:00
tatiana added a commit that referenced this pull request Jan 15, 2025
Since we introduced detached test tasks to fix a customer issue in #1433
(release 1.8.1), we changed how Cosmos renders DAGs, with the chance
that Cosmos significantly changed how it renders a dbt project - even
when users did not change their `DbtDag` or `DbtTaskGroup`
configuration. This is unacceptable in a micro release - and for this
reason we're reverting this change and making the feature opt-in.

PR #1433 led to issues such as #1464, reported by multiple Cosmos users,
and also issues that did not become Github issues, such as an Astro
customer who reported that when they upgraded to Cosmos 1.8.1, the
number of tasks increased dramatically. One problem with #1433 was that
it did not empower users to opt in or out of having detached test nodes,
solving the problem for some but causing problems for many.

This PR aims to solve this problem by introducing a new property to
`RenderConfig`: `should_detach_multiple_parents_tests`. We are reverting
the Cosmos DAG rendering to what it was in 1.8.0 and before: by default,
it will not detach tests with multiple parents. Users must opt-in for
this behaviour if and when they want to.

We understand this may be perceived as a breaking change by some, but it
is the correct way to move forward and avoid causing further disruption.

We are planning to review the current implementation, as described in
#1469. For now, this PR documents the current behaviour and
empowers users.
tatiana added a commit that referenced this pull request Jan 15, 2025
Bug Fixes
* Fix ``httpx.get`` exception handling while emitting telemetry by
@tatiana in #1439
* Fix (not) rendering detached tests in ``TestBehavior.NONE`` and
``AFTER_ALL`` by @tatiana in #1463
* Fix detached test tasks names so they do not exceed 250 chars by
@tatiana in #1464

Enhancement
* Allow users to opt-in or out (default) of detached test nodes by
@tatiana in #1470. Learn more about this
[here](https://astronomer.github.io/astronomer-cosmos/configuration/testing-behavior.html).

Docs
* Docs: Fix broken links and rendering by @pankajastro in #1437
* Update ``operator args`` docs to include ``install_deps`` by @tatiana
in #1456
* Improve Cosmos ``select`` docs to include latest graph operator
support by @tatiana in #1467

Others
* Upgrade GitHub action artifacts upload-artifact & download-artifact to
v4 by @pankajkoti in #1445
* Enable Depandabot to scan outdated Github Actions dependencies by
@tatiana in #1446
* Pre-commit hook updates in #1459, #1441
* Dependabot Github action updates in #1451, #1452, #1453, #1454, #1455
@tatiana tatiana added this to the Cosmos 1.8.2 milestone Jan 15, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:execution Related to the execution environment/mode, like Docker, Kubernetes, Local, VirtualEnv, etc size:M This PR changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug] The key has to be less than 250 characters
2 participants