Skip to content

Fix test_integration_virtualenv_operator #1718

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.

Sign up for GitHub

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

Closed
pankajastro wants to merge 2 commits into from
Closed

Fix test_integration_virtualenv_operator #1718

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8509943
Fix test_integration_virtualenv_operator
pankajastro Apr 28, 2025
ab82177
Fix command
pankajastro Apr 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 8 additions & 1 deletion dev/dags/example_virtualenv_mini.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,11 @@
from cosmos.operators.virtualenv import DbtSeedVirtualenvOperator
from cosmos.profiles import PostgresUserPasswordProfileMapping

try:
from airflow.providers.standard.operators.bash import BashOperator
except ImportError:
from airflow.operators.bash import BashOperator

DEFAULT_DBT_ROOT_PATH = Path(__file__).resolve().parent / "dbt"
DBT_PROJ_DIR = Path(os.getenv("DBT_ROOT_PATH", DEFAULT_DBT_ROOT_PATH)) / "jaffle_shop"

Expand All @@ -21,6 +26,8 @@
)

with DAG("example_virtualenv_mini", start_date=datetime(2022, 1, 1)) as dag:
pre = BashOperator(task_id="pre", bash_command="mkdir -p /tmp/persistent-venv2")
Copy link
Copy Markdown
Contributor

@pankajkoti pankajkoti Apr 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I know this is still is draft, but taking an early review. I don't think this could be the right approach. Ideally DbtSeedVirtualenvOperator itself should create this path.

Copy link
Copy Markdown
Contributor

@pankajkoti pankajkoti Apr 29, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

also, what if we try "/tmp/persistent-venv2" instead of Path("/tmp/persistent-venv2") in the argument value for the operator? does it have an effect in case of Airflow 3 and help with the rendering and at the same the operator execution is not impacted?

We can reflect that in our docs that the type needs to be a string and not Path.

Copy link
Copy Markdown
Contributor Author

@pankajastro pankajastro Apr 29, 2025

Choose a reason for hiding this comment

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

I think from a rendering point of view it should work, but type for virtualenv_dir is Path or None

Copy link
Copy Markdown
Contributor Author

@pankajastro pankajastro Apr 29, 2025

Choose a reason for hiding this comment

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

https://github.com/astronomer/astronomer-cosmos/blob/main/cosmos/operators/virtualenv.py#L79C9-L79C23

Copy link
Copy Markdown
Contributor Author

@pankajastro pankajastro Apr 29, 2025

Choose a reason for hiding this comment

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

I have created #1721 with your suggestion if CI is happy we can close this PR and merge #1721

Copy link
Copy Markdown
Contributor

@pankajkoti pankajkoti Apr 29, 2025

Choose a reason for hiding this comment

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

yes, the CI was unhappy, but I pushed a commit 1f1947c that seems to help


seed_operator = DbtSeedVirtualenvOperator(
profile_config=profile_config,
project_dir=DBT_PROJ_DIR,
Expand All @@ -32,4 +39,4 @@
py_requirements=["dbt-postgres"],
virtualenv_dir=Path("/tmp/persistent-venv2"),
)
seed_operator
pre >> seed_operator
2 changes: 0 additions & 2 deletions tests/operators/test_virtualenv.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -381,8 +381,6 @@ def test__release_venv_lock_current_process(tmpdir):
assert not lockfile.exists()


# TODO: Make test compatible with Airflow 3.0. Issue: https://github.com/astronomer/astronomer-cosmos/issues/1707
@pytest.mark.skipif(AIRFLOW_VERSION.major == 3, reason="Test need to be updated for Airflow 3.0")
@pytest.mark.skipif(
AIRFLOW_VERSION < Version("2.5"),
reason="This error is only reproducible with dag.test, which was introduced in Airflow 2.5",
Expand Down