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.

Sign up for GitHub

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

Jump to bottom

ci(airflow): Replace type hints with CatalogProtocol #845

Merged
merged 5 commits into from
Sep 25, 2024
Merged

ci(airflow): Replace type hints with CatalogProtocol #845

merged 5 commits into from
Sep 25, 2024

Conversation

ankatiyar
Copy link
Contributor

@ankatiyar ankatiyar commented Sep 23, 2024 •
edited
Loading

Description

Fix #840

Development notes

Replace DataCatalog with CatalogProtocol

Checklist

  • Opened this PR as a 'Draft Pull Request' if it is work-in-progress
  • Updated the documentation to reflect the code changes
  • Added a description of this change in the relevant RELEASE.md file
  • Added tests to cover my changes

@ankatiyar ankatiyar marked this pull request as ready for review September 23, 2024 11:38
merelcht
merelcht approved these changes Sep 23, 2024
View reviewed changes
Copy link
Member

@merelcht merelcht left a comment

Choose a reason for hiding this comment

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

⭐

ElenaKhaustova
ElenaKhaustova reviewed Sep 23, 2024
View reviewed changes
Copy link
Contributor

@ElenaKhaustova ElenaKhaustova left a comment

Choose a reason for hiding this comment

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

@ankatiyar should we also specify kedro version when release? Cause CatalogProtocol will be available only in 0.19.9.

@ankatiyar
Copy link
Contributor Author

@ankatiyar should we also specify kedro version when release? Cause CatalogProtocol will be available only in 0.19.9.

@ElenaKhaustova Yeah, you're right, let me see if I can make this work with both older and newer versions otherwise we might need to pin airflow plugin to newer versions of Kedro

@ankatiyar
Add try except for import
e66ce25 
Signed-off-by: Ankita Katiyar <[email protected]>
@ankatiyar
Ignore bandit warnings
9e594ed 
Signed-off-by: Ankita Katiyar <[email protected]>
@ankatiyar
Copy link
Contributor Author

@merelcht @ElenaKhaustova I've made some changes and checked with released Kedro 0.19.8 version as well as the latest main version of Kedro that the tests pass and airflow plugin works. The mypy check doesn't for older versions complaining that CatalogProtocol doesn't exist in kedro.io but if I add an ignore statement it complains on CI because we run these tests with the main version so I've left it as it is.

I've also added ignore statements to the bandit failures in the experimental dataset (see #850) Will leave that ticket open for the actual solution, but this will unblock the CI.

Would you mind taking another look?

@ankatiyar ankatiyar mentioned this pull request Sep 25, 2024
Open
merelcht
merelcht approved these changes Sep 25, 2024
View reviewed changes
Copy link
Member

@merelcht merelcht left a comment

Choose a reason for hiding this comment

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

LGTM! 👍

ElenaKhaustova
ElenaKhaustova approved these changes Sep 25, 2024
View reviewed changes
Copy link
Contributor

@ElenaKhaustova ElenaKhaustova left a comment

Choose a reason for hiding this comment

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

LGTM, thank you!

Left one non-blocking comment.

@ankatiyar
Remove any
2651901 
Signed-off-by: Ankita Katiyar <[email protected]>
@ankatiyar ankatiyar enabled auto-merge (squash) September 25, 2024 11:55
@ankatiyar ankatiyar merged commit 0f0c59e into main Sep 25, 2024
30 checks passed
@ankatiyar ankatiyar deleted the ci-airflow branch September 25, 2024 12:01
This was referenced Sep 25, 2024
Closed
Closed
Closed
harm-matthias-harms pushed a commit to harm-matthias-harms/kedro-plugins that referenced this pull request Oct 1, 2024
@ankatiyar @harm-matthias-harms
ci(airflow): Replace type hints with CatalogProtocol (kedro-org#845)
2cfe5e5 
* Replace type checking with CatalogProtocol

Signed-off-by: Ankita Katiyar <[email protected]>

* Add try except for import

Signed-off-by: Ankita Katiyar <[email protected]>

* Ignore bandit warnings

Signed-off-by: Ankita Katiyar <[email protected]>

* Remove any

Signed-off-by: Ankita Katiyar <[email protected]>

---------

Signed-off-by: Ankita Katiyar <[email protected]>
Signed-off-by: Harm Matthias Harms <[email protected]>
MinuraPunchihewa pushed a commit to MinuraPunchihewa/kedro-plugins that referenced this pull request Oct 1, 2024
@ankatiyar @MinuraPunchihewa
ci(airflow): Replace type hints with CatalogProtocol (kedro-org#845)
5b2faac 
* Replace type checking with CatalogProtocol

Signed-off-by: Ankita Katiyar <[email protected]>

* Add try except for import

Signed-off-by: Ankita Katiyar <[email protected]>

* Ignore bandit warnings

Signed-off-by: Ankita Katiyar <[email protected]>

* Remove any

Signed-off-by: Ankita Katiyar <[email protected]>

---------

Signed-off-by: Ankita Katiyar <[email protected]>
Signed-off-by: Minura Punchihewa <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ElenaKhaustova ElenaKhaustova ElenaKhaustova approved these changes

@merelcht merelcht merelcht approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

kedro-airflow: Nightly build failure
3 participants
@ankatiyar @merelcht @ElenaKhaustova