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

Upgrade databricks provider dependency #43272

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

dcmshi
Copy link

@dcmshi dcmshi commented Oct 22, 2024

continuation of #42626
patching some of the mypy test failures


^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

Copy link

boring-cyborg bot commented Oct 22, 2024

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
Here are some useful points:

  • Pay attention to the quality of your code (ruff, mypy and type annotations). Our pre-commits will help you with that.
  • In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
  • Consider using Breeze environment for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
  • Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
  • Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
  • Be sure to read the Airflow Coding style.
  • Always keep your Pull Requests rebased, otherwise your build might fail due to changes not related to your commits.
    Apache Airflow is a community-driven project and together we are making it better 🚀.
    In case of doubts contact the developers at:
    Mailing List: [email protected]
    Slack: https://s.apache.org/airflow-slack

@dcmshi dcmshi force-pushed the dshi/upgrade_databricks_prodiver_dependency_version_patch_mypy branch 2 times, most recently from 53b14d2 to 97d12ea Compare October 28, 2024 15:31
@dcmshi
Copy link
Author

dcmshi commented Oct 28, 2024

provider checks are good locally

hatch run pre-commit run mypy-providers --all-files

Screenshot 2024-10-28 at 1 47 07 PM

@dcmshi dcmshi force-pushed the dshi/upgrade_databricks_prodiver_dependency_version_patch_mypy branch from 4981110 to 817c01a Compare October 30, 2024 15:31
@dcmshi
Copy link
Author

dcmshi commented Oct 30, 2024

reran with manual mypy-providers check

hatch run pre-commit run --color always --verbose --hook-stage manual  mypy-providers --all-files
Screenshot 2024-10-30 at 11 31 15 AM

@dcmshi dcmshi force-pushed the dshi/upgrade_databricks_prodiver_dependency_version_patch_mypy branch 3 times, most recently from f8067c9 to f372a8c Compare November 1, 2024 15:17
@dcmshi dcmshi marked this pull request as ready for review November 1, 2024 15:17
@dcmshi dcmshi changed the title Dshi/upgrade databricks prodiver dependency version patch mypy upgrade databricks prodiver dependency version patch mypy Nov 1, 2024
@dcmshi dcmshi force-pushed the dshi/upgrade_databricks_prodiver_dependency_version_patch_mypy branch from f372a8c to d26b588 Compare November 1, 2024 19:39
@potiuk potiuk changed the title upgrade databricks prodiver dependency version patch mypy Upgrade databricks provideer dependency Nov 1, 2024
@potiuk potiuk changed the title Upgrade databricks provideer dependency Upgrade databricks provider dependency Nov 1, 2024
@dcmshi
Copy link
Author

dcmshi commented Nov 4, 2024

just syncing my fork, but the manual checks were passing before this so I think this should be ready for review?

@dcmshi dcmshi force-pushed the dshi/upgrade_databricks_prodiver_dependency_version_patch_mypy branch 2 times, most recently from 53c3e9a to 7daf6cf Compare November 4, 2024 19:54
@potiuk
Copy link
Member

potiuk commented Nov 4, 2024

Approved the workflow: 👀

@dcmshi dcmshi force-pushed the dshi/upgrade_databricks_prodiver_dependency_version_patch_mypy branch 2 times, most recently from bf65dff to c9bf287 Compare November 5, 2024 16:50
@dcmshi
Copy link
Author

dcmshi commented Nov 5, 2024

sorry I think I needed to rebase again, there were quite a few test failures in the test_backfill_endpoint module, but I think not related to my changes, I do see it was removed in this PR: https://github.com/apache/airflow/pull/43649/files . Could you please help me trigger checks whenever you get chance again 🙇

CI run test failures: https://github.com/apache/airflow/actions/runs/11671655535/job/32506671351?pr=43272
Screenshot 2024-11-05 at 11 48 25 AM

@dcmshi dcmshi force-pushed the dshi/upgrade_databricks_prodiver_dependency_version_patch_mypy branch 2 times, most recently from 924f052 to 175059d Compare November 5, 2024 19:31
Copy link
Contributor

Choose a reason for hiding this comment

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

How are changes to snowflake tests related to this PR?

Copy link
Author

Choose a reason for hiding this comment

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

The Snowflake test module is using the Row class from databricks.sql.types[0], which triggered the mypy checks on the module when trying to upgrade the databricks-sql-connecter package. I just went and patched the errors mypy was flagging, since I think refactoring the module to not use the Row class is probably it's own change there, but open to suggestions if this isn't the way we'd like to go.

[0] https://github.com/apache/airflow/pull/43272/files/175059dfe5cfb1f64297a8e26bcfe278edd7ebfb#diff-6fa19a213d772197807bb12413c4731e6b8a001a241f7f9009e152a1aac1f066R33

Copy link
Contributor

Choose a reason for hiding this comment

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

Wait. What??
I am not near my laptop so I can't investigate this right now but this sounds very odd. We need to look into the commit that added it to see what was the reason for creating such odd coupling.

Copy link
Author

Choose a reason for hiding this comment

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

I think this is the original PR which introduces the import[0], and then this PR adds logic to mock the import later[1]. It looks to be testing the return of a SQLExecuteQueryOperator but the test_exec_success doesn't seem to be a Snowflake specific test?

[0] #28006
[1] #38074

Copy link
Contributor

Choose a reason for hiding this comment

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

I dont think this is right.
Snowflake tests shuld not use databricks SDK

Copy link
Member

Choose a reason for hiding this comment

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

Yeah. I think this is a mistake someone used databricks row in the snowflake tests - they "match" as they represent the output of dbapi that has generally similar structure, but this should be, I think fixed to have either some generic Row structure used or creating of own Row class in the tests.

Copy link
Member

Choose a reason for hiding this comment

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

I think refactoring the module to not use the Row class is probably it's own change there, but open to suggestions if this isn't the way we'd like to go.

Yes. That's the way to go. Likely what would be great is to add similar tests that we have in snowflake now to databricks tests if possible, using databricks row, but to change the snowflake one to not use the databricks row

@dcmshi dcmshi force-pushed the dshi/upgrade_databricks_prodiver_dependency_version_patch_mypy branch 3 times, most recently from e38e057 to 071f5ad Compare November 8, 2024 16:00
@dcmshi dcmshi force-pushed the dshi/upgrade_databricks_prodiver_dependency_version_patch_mypy branch 2 times, most recently from 325468e to 5dfa0f5 Compare November 14, 2024 04:55
@dcmshi dcmshi force-pushed the dshi/upgrade_databricks_prodiver_dependency_version_patch_mypy branch from 5dfa0f5 to 4886f4a Compare November 15, 2024 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants