Skip to content

Fix path selector when manifest.json was created in MS Windows#1601

Merged
tatiana merged 2 commits into
mainfrom
normalize-manifest-path
Mar 13, 2025
Merged

Fix path selector when manifest.json was created in MS Windows#1601
tatiana merged 2 commits into
mainfrom
normalize-manifest-path

Conversation

@tatiana
Copy link
Copy Markdown
Collaborator

@tatiana tatiana commented Mar 13, 2025

Users who generated the manifest.json using MS Windows and attempted to use Cosmos path selectors after, such as `path:models/edr/run_results' were unable to do so, because the paths in Windows were different from the selector:

    "model.elementary.model_run_results": {
      "database": "FDH_DEV_DB",
      "schema": "MONITORING",
      "name": "model_run_results",
      "resource_type": "model",
      "package_name": "elementary",
      "path": "edr\\run_results\\model_run_results.sql",
      "original_file_path": "models\\edr\\run_results\\model_run_results.sql",
      "unique_id": "model.elementary.model_run_results",
      "fqn": [
        "elementary",
        "edr",
        "run_results",
        "model_run_results"
      ],

As observed in this example, the property original_file_path used the \\ character as a divider in the path, but the selector checked using the Posix notation.

Since Cosmos implements path selectors using: path_selection in str(node.file_path), we have to normalize the input for the filter to work.

This issue only happened when using LoadMode.DBT_MANIFEST and not LoadMode.DBT_LS since dbt normalizes this internally when handling selectors as part of this command line.

…d to use Cosmos path selectors after,

such as `path:models/edr/run_results' were unable to do so, because the paths in Windows were different from the selector:

```
    "model.elementary.model_run_results": {
      "database": "FDH_DEV_DB",
      "schema": "MONITORING",
      "name": "model_run_results",
      "resource_type": "model",
      "package_name": "elementary",
      "path": "edr\\run_results\\model_run_results.sql",
      "original_file_path": "models\\edr\\run_results\\model_run_results.sql",
      "unique_id": "model.elementary.model_run_results",
      "fqn": [
        "elementary",
        "edr",
        "run_results",
        "model_run_results"
      ],
```

As it can be observedf in this example, the property `original_file_path` used the `\\` character as a divider in the path,
but the selector checked using the Posix notation.

Since Cosmos implements path selectors using: path_selection in str(node.file_path), we have to normalize the input for the
filter to work.

This issue only happened when using `LoadMode.DBT_MANIFEST` and not `LoadMode.DBT_LS`, since dbt normalizes this internally
when handling selectors as part of this command line.
@dosubot dosubot Bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Mar 13, 2025
@cloudflare-workers-and-pages
Copy link
Copy Markdown

cloudflare-workers-and-pages Bot commented Mar 13, 2025

Deploying astronomer-cosmos with  Cloudflare Pages  Cloudflare Pages

Latest commit: 8cb710f
Status: ✅  Deploy successful!
Preview URL: https://c8b29a6b.astronomer-cosmos.pages.dev
Branch Preview URL: https://normalize-manifest-path.astronomer-cosmos.pages.dev

View logs

@netlify
Copy link
Copy Markdown

netlify Bot commented Mar 13, 2025

Deploy Preview for sunny-pastelito-5ecb04 canceled.

Name Link
🔨 Latest commit afa3581
🔍 Latest deploy log https://app.netlify.com/sites/sunny-pastelito-5ecb04/deploys/67d27b59695a990008c95365

@dosubot dosubot Bot added area:selector Related to selector, like DAG selector, DBT selector, etc parsing:dbt_manifest Issues, questions, or features related to dbt_manifest parsing labels Mar 13, 2025
@netlify
Copy link
Copy Markdown

netlify Bot commented Mar 13, 2025

Deploy Preview for sunny-pastelito-5ecb04 canceled.

Name Link
🔨 Latest commit 8cb710f
🔍 Latest deploy log https://app.netlify.com/sites/sunny-pastelito-5ecb04/deploys/67d27b62e42ece0008bafb79

@tatiana tatiana added the customer request An Astronomer customer made requested this label Mar 13, 2025
@codecov
Copy link
Copy Markdown

codecov Bot commented Mar 13, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.46%. Comparing base (b309dac) to head (8cb710f).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1601   +/-   ##
=======================================
  Coverage   97.46%   97.46%           
=======================================
  Files          80       80           
  Lines        4937     4939    +2     
=======================================
+ Hits         4812     4814    +2     
  Misses        125      125           

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

🚀 New features to boost your workflow:
  • Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
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, thanks for normalising the path here for the selectors to work well on Windows generated manifests 🙌🏽

@tatiana tatiana merged commit 9a1c8fe into main Mar 13, 2025
@tatiana tatiana deleted the normalize-manifest-path branch March 13, 2025 07:13
tatiana added a commit that referenced this pull request Mar 13, 2025
Makes sure the fixes:
- Fix path selector when manifest.json was created in MS Windows (#1601)
- Fix select behaviour using LoadMode.MANIFEST and a path with star (#1602)

Work from an end-to-end perspective, solving Astro customer's original issue.
@tatiana tatiana added this to the Cosmos 1.9.1 milestone Mar 13, 2025
tatiana added a commit that referenced this pull request Mar 13, 2025
Makes sure the fixes:
- Fix path selector when manifest.json was created in MS Windows (#1601)
- Fix select behaviour using LoadMode.MANIFEST and a path with star
(#1602)

Work from an end-to-end perspective, solving Astro customer's original
issue.
pankajkoti pushed a commit that referenced this pull request Mar 13, 2025
Users who generated the `manifest.json` using MS Windows and attempted
to use Cosmos path selectors after, such as
`path:models/edr/run_results' were unable to do so, because the paths in
Windows were different from the selector:

```
    "model.elementary.model_run_results": {
      "database": "FDH_DEV_DB",
      "schema": "MONITORING",
      "name": "model_run_results",
      "resource_type": "model",
      "package_name": "elementary",
      "path": "edr\\run_results\\model_run_results.sql",
      "original_file_path": "models\\edr\\run_results\\model_run_results.sql",
      "unique_id": "model.elementary.model_run_results",
      "fqn": [
        "elementary",
        "edr",
        "run_results",
        "model_run_results"
      ],
```

As observed in this example, the property `original_file_path` used the
`\\` character as a divider in the path, but the selector checked using
the Posix notation.

Since Cosmos implements path selectors using: path_selection in
str(node.file_path), we have to normalize the input for the filter to
work.

This issue only happened when using `LoadMode.DBT_MANIFEST` and not
`LoadMode.DBT_LS` since dbt normalizes this internally when handling
selectors as part of this command line.

(cherry picked from commit 9a1c8fe)
pankajkoti pushed a commit that referenced this pull request Mar 13, 2025
Makes sure the fixes:
- Fix path selector when manifest.json was created in MS Windows (#1601)
- Fix select behaviour using LoadMode.MANIFEST and a path with star
(#1602)

Work from an end-to-end perspective, solving Astro customer's original
issue.

(cherry picked from commit 3138892)
@pankajkoti pankajkoti mentioned this pull request Mar 13, 2025
@tatiana tatiana mentioned this pull request Mar 13, 2025
tatiana added a commit that referenced this pull request Mar 17, 2025
Bug Fixes

* Fix import error in dbt bigquery adapter mock for ``dbt-bigquery<1.8``
for ``ExecutionMode.AIRFLOW_ASYNC`` by @pankajkoti in #1548
* Fix ``operator_args`` override configuration by @ghjklw in #1558
* Fix missing ``install_dbt_deps`` in ``ProjectConfig`` ``__init__``
method by @ghjklw in #1556
* Fix dbt project parsing ``dbt_vars`` behavior passed via
``operator_args`` by @AlexandrKhabarov in #1543
* Avoid reading the connection during DAG parsing of the async BigQuery
operator by @joppevos in #1582
* Fix: Workaround to incorrectly raised ``gcsfs.retry.HttpError``
(Invalid Credentials, 401) by @tatiana in #1598
* Fix the async execution mode read sql files for dbt packages by
@pankajastro in #1588
* Improve BQ async error handling by @tatiana in #1597
* Fix path selector when ``manifest.json`` is created using MS Windows
by @tatiana in #1601
* Fix log that prints 'Total filtered nodes' by @tatiana in #1603
* Fix select behaviour using ``LoadMode.MANIFEST`` and a path with star
by @tatiana in #1602
* Support ``on_warning_callback`` with ``TestBehavior.BUILD`` and
``ExecutionMode.LOCAL`` by @corsettigyg in #1571
* Fix ``DbtRunLocalOperator.partial()`` support by @tatiana @ashb in
#1609
* fix: ``container_name`` is null for ecs integration by @nicor88 in
#1592

Docs

* Improve MWAA getting-started docs by removing unused imports by
@jx2lee in #1562

Others

* Disable ``example_cosmos_dbt_build.py`` DAG in CI by @pankajastro in
#1567
* Upgrade GitHub Actions Ubuntu version by @tatiana in #1561
* Update GitHub bug issue template by @pankajastro in #1586
* Enable DAG ``example_cosmos_dbt_build.py`` in CI by @pankajastro in
#1573
* Run async DAG in DAG without setup/teardown task by @pankajastro in
#1599
* Add test case that fully covers recent select issue by @tatiana in
#1604
* Add CI job to test multiple dbt versions for the async DAG by
@pankajkoti in #1535
* Improve unit tests speed from 89s to 14s by @tatiana in #1600
* Pre-commit updates: #1560, #1583, #1596


Closes: #1550

Mergeable version of
#1607

Co-authored-by: Pankaj Singh
<98807258+pankajastro@users.noreply.github.com>
Co-authored-by: Pankaj Koti <pankajkoti699@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:selector Related to selector, like DAG selector, DBT selector, etc customer request An Astronomer customer made requested this parsing:dbt_manifest Issues, questions, or features related to dbt_manifest parsing size:S This PR changes 10-29 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants