getindata/dbt-airflow-factory

Required packages should be required

Closed this issue · 5 comments

pgoslatara commented

Not all packages required by dbt-airflow-factory are installed by default. Steps to reproduce:

  1. Create a new virtual environment.
  2. Run:
python -c "from dbt_airflow_factory.airflow_dag_factory import AirflowDagFactory"
Traceback (most recent call last):
  File "<string>", line 1, in <module>
  File "<REDACTED>/.venv/lib/python3.11/site-packages/dbt_airflow_factory/airflow_dag_factory.py", line 5, in <module>
    from airflow import DAG
ModuleNotFoundError: No module named 'airflow'
p-pekala commented

Hi @pgoslatara ,
This tool is meant to be used as an Airflow plugin so the airflow needs to be already installed in the context. Also, we don't want to specify its version as it should work with any (it was tested with 1.10+ and 2.X).

pgoslatara commented

@p-pekala Agreed on the usage of this package, The associated PR (#98 ) does not set a constraint on the apache-airflow version.

Are you sure that all Airflow instances will have the Airbyte, k8s and Slack provider packages installed? Because I don't think this is the case, so if I want to use this package I now have multiple installations steps:

  1. Install this package.
  2. See that it's dependencies are not met.
  3. Install the Airbyte provider.
  4. Realise there are more unmet dependencies.
  5. Rinse and repeat for k8s and Slack.
p-pekala commented

These packages are optional so we don't want to force everyone to install all of them if they are not going to use them.

pgoslatara commented

@p-pekala I think this is the point I'm (trying poorly 😄) to make! I'm using this package in my Airflow instance. But I don't use Airbyte or k8s or Slack, yet I have to install all of them! My feeling is that either the package should restructured to not require these to run (likely a lot of work and added complexity) or the packages should be installed by default (my preferred option, albeit with wider version contraints than currently exists).

p-pekala commented

I see your point now. So it shouldn't work like this and that is the bug we need to fix. We usually used all these packages and we missed that so far. We can add it to the required packages until we fix it properly.

  • 👍1