Fix Cosmos enable_cache setting

[tatiana]

As of Cosmos 1.4.1, users are not able to disable the cache in Cosmos using AIRFLOW__COSMOS__ENABLE_CACHE=0.

During a recent refactoring #975, the enable_cache was changed to a non-boolean config. This PR fixes this issue.

[netlify]

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

Name	Link
🔨 Latest commit	cf36cc7
🔍 Latest deploy log	https://app.netlify.com/sites/sunny-pastelito-5ecb04/deploys/66603d9dadfc8d00083c93b6

[pankajastro]

Thanks for fixing it!

[maslick]

@tatiana Does AIRFLOW__COSMOS__ENABLE_CACHE=False work in 1.4.1?

[tatiana]

@maslick There was an issue in the 1.4.1 release with this configuration, this was solved and is available on 1.4.2 and 1.4.3

[maslick]

@tatiana I understand. The reason I am asking is that we're using AIRFLOW__COSMOS__ENABLE_CACHE=False (and not AIRFLOW__COSMOS__ENABLE_CACHE=0 as in the description of this PR) with 1.4.1, but we can't easily migrate to 1.4.2/1.4.3 at the moment. So my question is, would no caching even work with 1.4.1 (AIRFLOW__COSMOS__ENABLE_CACHE=False)?

[tatiana]

@maslick In Cosmos 1.4.1, if you want to disable the cache, you can do this via

export AIRFLOW__COSMOS__ENABLE_CACHE=''

If you check this PR, we fixed how we retrieve the envvar to use Airflow conf.getboolean:

enable_cache = conf.getboolean("cosmos", "enable_cache", fallback=True)

In Cosmos 1.4.1, this was implemented conf.get by mistake.

From Cosmos 1.4.2, both AIRFLOW__COSMOS__ENABLE_CACHE=False as well as AIRFLOW__COSMOS__ENABLE_CACHE=0 will work.

Did you have any issues with cache that made you want to switch off this feature?

[maslick]

@tatiana our issue with cache enabled was that it quickly fills up the 30 GB storage volume on MWAA workers. We are running a large number of DAGs (>500) with lots of dbt models/transformations... We can't increase the storage volume on MWAA either. That's why we decided to disable cache entirely. Of course, this would increase the parsing times...

[josefbits]

@tatiana With cache disabled it's no longer possible to do partial parsing from a supplied manifest. Is this intended behavior and or is there some config that would need to change to get it working?

[tatiana]

@maslick this issue will hopefully be resolved once #927 is implemented. I'm sorry that the cache had such a massive impact on your deployment - we weren't expecting for that to take 30 GB volume storage. It's a shame MWAA doesn't handle better temp files.

@josefbits I believe this is unintended behaviour. Please open an issue using https://github.com/astronomer/astronomer-cosmos/issues, and we'll follow up on this ASAP. Contributions are also welcome; it should be a small fix.

[josefbits]

@tatiana Thanks, I've opened #1041 for this. I'll take a look at it more tomorrow, but maybe someone with more knowledge of the codebase can figure it out quicker.

[josefbits]

@tatiana Not sure if this is the right place to continue this discussion, but with regards to

@maslick this issue will hopefully be resolved once #927 is implemented. I'm sorry that the cache had such a massive impact on your deployment - we weren't expecting for that to take 30 GB volume storage. It's a shame MWAA doesn't handle better temp files.

Is there some specific reason that you need to cache multiple copies of the manifests? My best guess was in case of write locks by multiple Airflow tasks. Right now Cosmos is caching once per DbtTaskGroup and for projects with a large number of models, dags, task groups etc it can become problematic on managed Airflow solutions.

Speaking of dbt parsing specifically, I've done a bunch of comparisons on the manifest.json before and after run, with partial parse enabled/disabled, and different methods to change the dbt state. From what I've seen the only thing that really changes is metadata like created_at, raw_code, and compile/build paths. This is, of course, assuming parity between CI build environment and run time, and that one is not doing something strange that might change the model code or dependencies.

Just looking for a bit more insight :)

[tatiana]

@josefbits, thanks for your insights!
This is a better place to discuss this: #1042

The issue we're discussing was not introduced by this PR, which was already merged last week - so new discussions may get lost.