Gracefully error when users set imcompatible RenderConfig.dbt_deps and operator_args install_deps

[tatiana]

Customer is facing this error when RenderConfig(dbt_deps=True) and operator_args={"install_deps": False}:

This issue does not happen if both of them are False.

Closes: #1458
Closes: #1457

[netlify]

✅ Deploy Preview for sunny-pastelito-5ecb04 canceled.

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

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.13%. Comparing base (be44d7c) to head (40191e0).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1505   +/-   ##
=======================================
  Coverage   97.13%   97.13%           
=======================================
  Files          77       77           
  Lines        4505     4507    +2     
=======================================
+ Hits         4376     4378    +2     
  Misses        129      129           

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

[pankajkoti]

Can we please reconfirm the case when the user has not explicitly set install_deps in their task, what value comes here for install_deps?

I am thinking that when user has not specified install_deps in their task_args, assuming it would be False by default and at the same time if they have specified render_config.dbt_deps=False, we would raise this error and this should not be the case, no?

I am thinking if we could rather change this to,

if render_config.dbt_deps and task_args.get("install_deps") ==  False

[tatiana]

@pankajkoti the current "default" value of install_deps is True at

astronomer-cosmos/cosmos/operators/local.py

Line 185 in 002c919

self.install_deps = install_deps and has_non_empty_dependencies_file(Path(self.project_dir))

I agree we should improve this; perhaps I could isolate and unify the logic as a function, e.g. "calculate_default_install_deps`, and use it in both places?

We need render_config.dbt_deps and the default task_args.get("install_deps") behaviour in the operators to either be + or -, and it seems the proposed alternative wouldn't give us this.

[pankajkoti]

the current "default" value of install_deps is True at

Wouldn’t the current default value of install_deps be False? I mean at

astronomer-cosmos/cosmos/operators/local.py

Line 145 in 002c919

install_deps: bool = False,

Yes, since we’re validating arguments before initializing the operators, it looks like setting the default value is deferred until the operators are actually instantiated. This could cause some confusion because if the user hasn’t explicitly set install_deps in their DAG operators, the key won’t exist in task_args at this stage, meaning task_args.get("install_deps") would return None.

I agree that having a common function to determine the default value would be beneficial, as it could be used in both places. Additionally, we’d need to account for cases where the user provides a custom value for a specific operator, ensuring that we correctly handle overrides while still computing the default value when necessary.

[tatiana]

Wouldn’t the current default value of install_deps be False? I mean at

From a function signature definition, yes, False is the default value. But from an initialization perspective, we're changing the "default" behaviour

astronomer-cosmos/cosmos/operators/local.py

Line 185 in 002c919

self.install_deps = install_deps and has_non_empty_dependencies_file(Path(self.project_dir))

That means that even if users explicitly set True, the install_ops may become False.

Additionally, we’d need to account for cases where the user provides a custom value for a specific operator, ensuring that we correctly handle overrides while still computing the default value when necessary.

Yes, I believe we should not allow users -as of now - to override this property. Otherwise they will end up seeing the original exception reported by our customer. WDYT?

[pankajkoti]

yes, probably we could brainstorm this more together over a call when possible?