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

Amazon Bedrock - Knowledge Bases and Data Sources #39245

Merged
merged 27 commits into from May 2, 2024

Conversation

ferruzzi
Copy link
Contributor

Adds hooks, operators, sensors, triggers, and waiters to enable support for Amazon Bedrock Knowledge Bases. This includes Bedrock Data Sources and Ingestion Jobs, and some light additions (a thin hook, and a waiter/sensor/trigger set) for Amazon OpenSearch Serverless which are needed to make the system test work end-to-end.

System test and unit tests are included, as well as docs. Static checks, unit tests, and build-docs all pass locally.


^ 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.

@vincbeck
Copy link
Contributor

Not a big deal but you could have split this PR in 2; Bedrock and Opensearch. Especially because you do not mention opensearch in the PR title.

@ferruzzi
Copy link
Contributor Author

I'm really not sure what that failing static check is trying to tell me. Wouldn't mind a hand on that if someone knows what it means.

@vincbeck
Copy link
Contributor

I'm really not sure what that failing static check is trying to tell me. Wouldn't mind a hand on that if someone knows what it means.

I think, it tries to tell you that you created a doc (opensearchserverless.rst) that you do not reference anywhere. Adding a how-to-guide section in provider.yaml which references the new doc should solve it

@ferruzzi
Copy link
Contributor Author

ferruzzi commented Apr 25, 2024

Adding a how-to-guide section in provider.yaml which references the new doc should solve it

I SEE! Some of the integrations list a how-to and others don't, I'll add that in and see if that fixes it.

I'll have the missing deferrable implementations up some time later today, I want to make sure I run them through manual testing which takes a bit of time per run.

@ferruzzi ferruzzi force-pushed the ferruzzi/bedrock/m5-knowledge-bases branch from be1caac to d604fff Compare April 25, 2024 19:00
@ferruzzi ferruzzi force-pushed the ferruzzi/bedrock/m5-knowledge-bases branch from 32c7b5e to 73c0fa6 Compare April 26, 2024 21:00
@ferruzzi
Copy link
Contributor Author

Alright. @vincbeck I believe I have address the concerns you raised

I just ran the system test end to end three times:

default_args={deferrable=True, wait_for_completion=False}
default_args={deferrable=False, wait_for_completion=True}
default_args={deferrable=False, wait_for_completion=False}

and it passes all three ways.

While I was at it, I was pretty behind so I also rebased. Sorry for the force push.

I'll have to sort out that failing static check on Monday, it looks like maybe I have a boto call in top-level code somewhere in the system test, perhaps? Not sure.

Copy link
Contributor

@vincbeck vincbeck left a comment

Choose a reason for hiding this comment

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

Beside the unit test failure you mentioned, LGTM!

@ferruzzi ferruzzi force-pushed the ferruzzi/bedrock/m5-knowledge-bases branch from 98fd413 to d2fba72 Compare April 29, 2024 22:40
@ferruzzi
Copy link
Contributor Author

Alright, got all the tests passing and rebased since it's been a few days. That was an adventure, sorry about that.

@ferruzzi
Copy link
Contributor Author

how could the change in 6d5cdd2 have broken three test which were all passing before that commit? It's copy and pasted dag declaration from the rest of our system tests.

- The template fields are not substituted yet in the init so call_args was preserving and using the template string rather than the substituted value.
- Some of the names became too long once our env_id was appended, so I shortened them down.
@ferruzzi
Copy link
Contributor Author

ferruzzi commented May 1, 2024

Assuming the CI goes green, I'm done.

@vincbeck - you may want to have a peek at the changes since you approved, or not.
@Taragolis - I ended up doing the generic base sensor your way and it worked perfectly. Thanks!

@ferruzzi ferruzzi merged commit 598398a into apache:main May 2, 2024
40 checks passed
@ferruzzi ferruzzi deleted the ferruzzi/bedrock/m5-knowledge-bases branch May 2, 2024 18:07
RodrigoGanancia pushed a commit to RodrigoGanancia/airflow that referenced this pull request May 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants