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

Create TaskFlow API to use WeaviateHook and use taskflow API for ask-astro-load.py #132

Merged
merged 27 commits into from
Nov 23, 2023
Merged

Create TaskFlow API to use WeaviateHook and use taskflow API for ask-astro-load.py #132

merged 27 commits into from
Nov 23, 2023

Conversation

sunank200
Copy link
Collaborator

@sunank200 sunank200 commented Nov 16, 2023 •
edited

  • I have taken airflow Docs ingestion for my implementation. Hence commits from Michael.
  • Create AskAstroWeaviateHook which inherits from OSS WeaviateHook.
  • Add get_schema, check_schema, create_schema, handle_upsert_rollback and ingest_data
  • Use Taskflow API for ask-astro-load.py.

closes: #134

@cloudflare-pages Cloudflare Pages
Copy link

cloudflare-pages bot commented Nov 17, 2023 •
edited

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: 93ea3d8
Status: ✅  Deploy successful!
Preview URL: https://3db4b985.ask-astro.pages.dev
Branch Preview URL: https://oss-weaviate-integration.ask-astro.pages.dev

View logs

Lee-W
Lee-W reviewed Nov 20, 2023
View reviewed changes
airflow/Dockerfile Outdated Show resolved Hide resolved
airflow/README.md Outdated Show resolved Hide resolved
airflow/requirements.txt Outdated Show resolved Hide resolved
airflow/requirements.txt Outdated Show resolved Hide resolved
airflow/include/airflow_provider_weaviate-0.0.1-py3-none-any.whl Outdated Show resolved Hide resolved
@sunank200 sunank200 marked this pull request as ready for review November 20, 2023 11:20
@sunank200 sunank200 changed the title Create TaskFlow API to use WeaviateHook. Create TaskFlow API to use WeaviateHook and use taskflow API for ask-astro-load.py Nov 20, 2023
This was referenced Nov 20, 2023
Merged
Closed
Lee-W
Lee-W reviewed Nov 21, 2023
View reviewed changes
airflow/dags/ingestion/ask-astro-load-airflow-docs.py Outdated Show resolved Hide resolved
airflow/dags/ingestion/ask-astro-load-airflow-docs.py Outdated Show resolved Hide resolved
airflow/dags/ingestion/ask-astro-load-airflow-docs.py Show resolved Hide resolved
airflow/dags/ingestion/ask-astro-load-github.py Show resolved Hide resolved
airflow/dags/ingestion/ask-astro-load.py Outdated Show resolved Hide resolved
airflow/include/tasks/extract/github.py Outdated Show resolved Hide resolved
airflow/include/tasks/extract/github.py Outdated Show resolved Hide resolved
airflow/include/tasks/extract/stack_overflow.py Outdated Show resolved Hide resolved
airflow/include/tasks/extract/stack_overflow.py Outdated Show resolved Hide resolved
airflow/include/tasks/extract/utils/html_helpers.py Outdated Show resolved Hide resolved
@mpgreg mpgreg mentioned this pull request Nov 21, 2023
Closed
11 tasks
mpgreg
mpgreg suggested changes Nov 21, 2023
View reviewed changes
Copy link
Contributor

@mpgreg mpgreg left a comment

Choose a reason for hiding this comment

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

Some logic errors but mostly I'm concerned about readablity and generalizability. For the sake of the reference implementation we want to be very clear about DAG/task code and what should (eventually) go into the hook.

airflow/dags/ingestion/ask-astro-load.py Outdated Show resolved Hide resolved
airflow/include/data/schema.json Outdated Show resolved Hide resolved
airflow/include/tasks/extract/stack_overflow.py Outdated Show resolved Hide resolved
airflow/include/tasks/ingest.py Show resolved Hide resolved
airflow/dags/ingestion/ask-astro-load.py Outdated Show resolved Hide resolved
airflow/include/tasks/extract/utils/weaviate/ask_astro_weaviate_hook.py Outdated Show resolved Hide resolved
airflow/include/tasks/extract/utils/weaviate/ask_astro_weaviate_hook.py Outdated Show resolved Hide resolved
airflow/dags/ingestion/ask-astro-load.py Show resolved Hide resolved
airflow/dags/ingestion/ask-astro-load.py Show resolved Hide resolved
airflow/dags/ingestion/ask-astro-load-github.py Show resolved Hide resolved
@sunank200 sunank200 force-pushed the oss_weaviate_integration branch 4 times, most recently from 57af49b to 64d97f0 Compare November 21, 2023 21:35
@sunank200
Copy link
Collaborator Author

Some logic errors but mostly I'm concerned about readablity and generalizability. For the sake of the reference implementation we want to be very clear about DAG/task code and what should (eventually) go into the hook.

@mpgreg I have fixed most of your comments. Please check. Also just to be clear. This hook is not a drop-in replacement for LLM providers. It requires work contextual change to be added for LLM provider

Lee-W
Lee-W approved these changes Nov 22, 2023
View reviewed changes
Copy link
Collaborator

@Lee-W Lee-W left a comment

Choose a reason for hiding this comment

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

There are only some nitpicks from my side. other parts look good to me

airflow/include/tasks/extract/utils/weaviate/ask_astro_weaviate_hook.py Outdated Show resolved Hide resolved
airflow/include/tasks/extract/utils/weaviate/ask_astro_weaviate_hook.py Outdated Show resolved Hide resolved
airflow/include/tasks/extract/utils/weaviate/ask_astro_weaviate_hook.py Outdated Show resolved Hide resolved
airflow/include/tasks/extract/utils/weaviate/ask_astro_weaviate_hook.py Outdated Show resolved Hide resolved
@sunank200 sunank200 force-pushed the oss_weaviate_integration branch 2 times, most recently from d15958d to c036d67 Compare November 23, 2023 08:08
@sunank200
Fix pre-commit error
c8bc5ee
@sunank200
Add ingest and upsert for weavaite
c0ccb2e
@sunank200
Update dockerfile
4a9d428
@sunank200
Fix comments from Wei
2ffceb8
@sunank200
Add context manager for batch ingestion
9945a3b
@sunank200
Remove the code which is not in scope for this PR
e2fc253
@sunank200
Fix comments from Michael
b238278
@sunank200
Use AskAstroWeaviateHook for ask-astro-load-slack.py DAG (#161)
d4c9c31 
- Use `AskAstroWeaviateHook` as the taskflow API for
[ask-astro-load-slack.py](https://github.com/astronomer/ask-astro/blob/main/airflow/dags/ingestion/ask-astro-load-slack.py)

closes: #140
@sunank200
Use AskAstroWeaviateHook for ask-astro-load-stackoverflow.py DAG (#160)
0761bbc 
- Use `AskAstroWeaviateHook` as the taskflow API for
[ask-astro-load-stackoverflow.py](https://github.com/astronomer/ask-astro/blob/main/airflow/dags/ingestion/ask-astro-load-stackoverflow.py)

closes: #139
@sunank200
Use AskAstroWeaviateHook for ask-astro-load-registry.py DAG (#159)
efcf8f9 
- Use `AskAstroWeaviateHook` as the taskflow API for
[ask-astro-load-registry.py](https://github.com/astronomer/ask-astro/blob/main/airflow/dags/ingestion/ask-astro-load-registry.py)

closes: #138
@sunank200
Use AskAstroWeaviateHook for ask-astro-load-github.py DAG (#158)
2849ec6 
- Use `AskAstroWeaviateHook` as the taskflow API for
[ask-astro-load-github.py](https://github.com/astronomer/ask-astro/blob/main/airflow/dags/ingestion/ask-astro-load-github.py)

closes: #137
@sunank200
Use AskAstroWeaviateHook for ask-astro-load-blogs.py (#156)
d8f4fd6 
- Use `AskAstroWeaviateHook` as the taskflow API for
[ask-astro-load-blogs.py](https://github.com/astronomer/ask-astro/blob/main/airflow/dags/ingestion/ask-astro-load-blogs.py)

closes: #135
@sunank200
Use AskAstroWeaviateHook for ask_astro_load_airflow_docs DAG (#155)
3e494fa 
- Implement AskAstroWeaviateHook which inherits from WeavaiteHook from
OSS.
- Use AskAstroWeaviateHook as the taskflow API for
[ask-astro-load-airflow-docs.py](https://github.com/astronomer/ask-astro/blob/45ce6543d044a977d97e9314e443f629604e84a9/airflow/dags/ingestion/ask-astro-load-airflow-docs.py)


closes: #141 

Note: Merge this PR before merging
[132](#132)
@sunank200
Fix the comments from Wei
f5ddc1e
@sunank200
Fix comments from Michael
63bcb55
mpgreg
mpgreg suggested changes Nov 23, 2023
View reviewed changes
Copy link
Contributor

@mpgreg mpgreg left a comment

Choose a reason for hiding this comment

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

Looks great @sunank200. Just a couple of small suggestions.

airflow/include/tasks/extract/utils/weaviate/ask_astro_weaviate_hook.py Outdated Show resolved Hide resolved
airflow/include/tasks/extract/utils/weaviate/ask_astro_weaviate_hook.py Outdated Show resolved Hide resolved
airflow/include/tasks/extract/utils/weaviate/ask_astro_weaviate_hook.py Outdated Show resolved Hide resolved
airflow/include/tasks/extract/utils/weaviate/ask_astro_weaviate_hook.py Outdated Show resolved Hide resolved
airflow/include/tasks/extract/utils/weaviate/ask_astro_weaviate_hook.py Outdated Show resolved Hide resolved
airflow/include/tasks/extract/utils/weaviate/ask_astro_weaviate_hook.py Outdated Show resolved Hide resolved
@sunank200 sunank200 merged commit 1067794 into main Nov 23, 2023
7 checks passed
@sunank200 sunank200 deleted the oss_weaviate_integration branch November 23, 2023 11:24
@sunank200 sunank200 mentioned this pull request Nov 27, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Lee-W Lee-W Lee-W approved these changes

@mpgreg mpgreg mpgreg approved these changes

@pankajastro pankajastro Awaiting requested review from pankajastro pankajastro is a code owner

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

Successfully merging this pull request may close these issues.

Create AskAstroWeaviateHook which uses OSS Weavaite Hook in ask-astro-load.py
3 participants
@sunank200 @Lee-W @mpgreg