Skip to content
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.

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

Google provider : Unnecessary imports for CloudSQL operators #40175

Closed
2 tasks done
bourliam opened this issue Jun 11, 2024 · 6 comments · Fixed by #41009
Closed
2 tasks done

Google provider : Unnecessary imports for CloudSQL operators #40175

bourliam opened this issue Jun 11, 2024 · 6 comments · Fixed by #41009
Assignees
Labels
area:providers kind:bug This is a clearly a bug

Comments

@bourliam
Copy link

Apache Airflow Provider(s)

google

Versions of Apache Airflow Providers

google, ssh, slack

Apache Airflow version

2.6.3

Operating System

composer

Deployment

Google Cloud Composer

Deployment details

No response

What happened

When trying to load the dag for a test with a CloudSQLExportInstanceOperator (from airflow.providers.google.cloud.operators.cloud_sql) I have a module not found error (ModuleNotFoundError: No module named 'airflow.providers.mysql').

I do not use mysql, so I should not have to install this module.

What you think should happen instead

We should not need those dependencies if not necessary. Moreover it is only used for type checking in another operator (CloudSQLExecuteQueryOperator)

How to reproduce

Import a CloudSQL operator without the mysql provider installed for example

Anything else

No response

Are you willing to submit PR?

  • Yes I am willing to submit a PR!

Code of Conduct

@bourliam bourliam added area:providers kind:bug This is a clearly a bug needs-triage label for new issues that we didn't triage yet labels Jun 11, 2024
Copy link

boring-cyborg bot commented Jun 11, 2024

Thanks for opening your first issue here! Be sure to follow the issue template! If you are willing to raise PR to address this issue please do so, no need to wait for approval.

@shahar1 shahar1 added duplicate Issue that is duplicated and removed needs-triage label for new issues that we didn't triage yet labels Jun 11, 2024
@shahar1
Copy link
Contributor

shahar1 commented Jun 11, 2024

Thanks for creating the issue, but it seems to be solved in a later version by #33783.
As a Cloud Composer user, you might consider upgrading to composer-2.X.X-airflow-2.7.3 to handle this.

@shahar1 shahar1 closed this as not planned Won't fix, can't repro, duplicate, stale Jun 11, 2024
@bourliam
Copy link
Author

Hello @shahar1, Thanks for the reply but this was not solved (at least not for me) :

I have been digging a little bit more and the module not found error comes from the file google/cloud/hooks/cloud_sql.py where the mysql import is "mandatory" even if I do not use MySql databases.

@shahar1
Copy link
Contributor

shahar1 commented Jun 12, 2024

airflow.providers.google.cloud.operators.cloud_sql

Oh, I think that I see it now - I assume that you're referring to these imports in hooks/cloud_sql.py:

from airflow.providers.mysql.hooks.mysql import MySqlHook
from airflow.providers.postgres.hooks.postgres import PostgresHook

They are currently being used in this operator for initializing the hook, not only for typing:

    def get_database_hook(self, connection: Connection) -> PostgresHook | MySqlHook:
        """Retrieve database hook.

        This is the actual Postgres or MySQL database hook that uses proxy or
        connects directly to the Google Cloud SQL database.
        """
        if self.database_type == "postgres":
            db_hook: PostgresHook | MySqlHook = PostgresHook(connection=connection, schema=self.database) # <-
        else:
            db_hook = MySqlHook(connection=connection, schema=self.database) # <-
        self.db_hook = db_hook
        return db_hook

Maybe you could import them internally, so you won't need them both installed to use this hook.
Reopening, feel free to contribute a PR - I'd be happy to review it.

@shahar1 shahar1 reopened this Jun 12, 2024
@shahar1 shahar1 removed the duplicate Issue that is duplicated label Jun 12, 2024
@jhongy1994
Copy link
Contributor

jhongy1994 commented Jul 24, 2024

Hello. I am interested in working on this issue. Can I take it over?

@shahar1 shahar1 assigned jhongy1994 and unassigned bourliam Jul 24, 2024
@shahar1
Copy link
Contributor

shahar1 commented Jul 24, 2024

Hello. I am interested in working on this issue. Can I take it over?

I assigned you :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:providers kind:bug This is a clearly a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants