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

readme for agent app sample and main readme updates #56

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

Conversation

FMurray
Copy link

@FMurray FMurray commented Dec 17, 2024

Related Issues/PRs

General chore item

What changes are proposed in this pull request?

Adding and updating readmes incl. getting started, contributing, local development

How is this PR tested?

@prithvikannan prithvikannan self-requested a review December 17, 2024 20:39
Copy link
Collaborator

@prithvikannan prithvikannan left a comment

Choose a reason for hiding this comment

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

Thanks a lot @FMurray!! LGTM with a handful of comments

README.md Outdated
- Start with `agent_app_sample_code/A_POC_app` to build a proof of concept
- Then explore `agent_app_sample_code/B_quality_iteration` to improve quality
- Uses Databricks' Mosaic AI Agent Framework for enterprise features
Copy link
Collaborator

Choose a reason for hiding this comment

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

what does "enterprise features" mean here? can we be more explicit and say something like "agent serving" and "agent eval"

README.md Outdated
- Check out `agent_app_sample_code`

- **For OpenAI SDK Integration:**
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- **For OpenAI SDK Integration:**
- **For Agent Application in pure Python + OpenAI:**

README.md Outdated
Comment on lines 31 to 48

```
├── agent_app_sample_code/ # Sample code for agent applications
│ ├── agents/ # Agent code
│ ├── 03_agent_proof_of_concept.py # Example of a proof of concept agent
│ └── ... # Additional directories and files
├── openai_sdk_agent_app_sample_code/ # Sample code using OpenAI SDK
│ └── ... # Directories and files
├── rag_app_sample_code/ # Sample code for RAG applications
│ ├── A_POC_app/ # Proof-of-Concept applications
│ ├── pdf_uc_volume/ # Example of a RAG application using a PDFs
│ ├── B_quality_iteration/ # Code for quality iteration
│ └── ... # Additional directories and files
├── genai_cookbook/ # Documentation and learning materials
├── data/ # Sample data for testing and development
├── dev/ # Development tools and scripts
└── README.md # This README file
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we drop this part? seems hard to maintain as we iterate on this repo

## TL;DR:

Choose the recipe that best matches your needs:
Copy link
Collaborator

Choose a reason for hiding this comment

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

can we emphasize the 10 minute demo?

Copy link
Author

Choose a reason for hiding this comment

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

wdym by emphasize?

- Each notebook contains detailed instructions and explanations

### Option 2: Running Locally
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this fully supported? we dont recommend this in our docs anywhere, so just want to make sure.

Copy link

@jiayi-wu-3150 jiayi-wu-3150 Dec 17, 2024

Choose a reason for hiding this comment

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

Or it means for running via VS Code but using Databricks' runtime? I feel a bit concerned as we are cloud platform and customers may feel confusing if we suggest run stuff locally.

Copy link
Author

Choose a reason for hiding this comment

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

That's fair - I think we need to document it, but it might not work consistently across all "recipes". We should probably let each one define their local development workflows if applicable.

Copy link
Author

Choose a reason for hiding this comment

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

Or it means for running via VS Code but using Databricks' runtime? I feel a bit concerned as we are cloud platform and customers may feel confusing if we suggest run stuff locally.

Yes, this means running in VSCode with databricks connect.

README.md Outdated
![Alt text](rag_app_sample_code/dbxquality.png)

## Getting Started
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe worth mentioning prereqs like UC


### Prerequisites

- Databricks Runtime 14.0 or higher
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- Databricks Runtime 14.0 or higher
- Databricks Runtime 14.0 or higher or serverless

Choose a reason for hiding this comment

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

I am not sure if that could be done by purely Serverless. In the AI cookbook documentation, it mentions both https://ai-cookbook.io/nbs/6-implement-overview.html#requirements.

My understanding is the AI Cookbook includes many components and integrates various products, each with its own requirements. Some products, like Mosaic AI vector search, use Serverless runtime, while others rely on the DBR environment. This is why both DBR 14.3 (Non-ML) and Serverless runtime are mentioned. Not sure if that's correct.

Copy link
Author

Choose a reason for hiding this comment

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

Yeah we should specify 'serverless notebook'


```python
# 1. Configure your agent
from agents.agent_config import AgentConfig, RetrieverToolConfig, LLMConfig
Copy link
Collaborator

Choose a reason for hiding this comment

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

not related to this PR, but i feel importing from agents... for cookbook util code, and then using from databricks_agents... for the actual SDK is confusing.

Copy link
Author

Choose a reason for hiding this comment

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

💯

README.md Outdated
The `rag_app_sample_code` directory contains sample code for Retrieval-Augmented Generation (RAG) applications.

The `genai_cookbook` directory contains a 10 minute getting started guide.
Copy link

@jiayi-wu-3150 jiayi-wu-3150 Dec 17, 2024

Choose a reason for hiding this comment

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

would dev and quick_start_demo got a word to say here, or they are good to go?

Choose a reason for hiding this comment

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

I wonder which part this 10 minute demo has been simplified compared to agent_app_sample_code?

Copy link
Author

Choose a reason for hiding this comment

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

dev is for publishing this as a jupyter book, IMO we need to remove it from this repo

README.md Outdated

- For bugs or feature requests, [create an issue](https://github.com/databricks/genai-cookbook/issues)
- For questions, start a [GitHub Discussion](https://github.com/databricks/genai-cookbook/discussions)
Copy link

@jiayi-wu-3150 jiayi-wu-3150 Dec 17, 2024

Choose a reason for hiding this comment

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

nit: the link doesn't work. maybe remove or update the link?

Copy link

@jiayi-wu-3150 jiayi-wu-3150 left a comment

Choose a reason for hiding this comment

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

LGTM with comments. Can you take another look at the local running part? I know some code developers may prefer that way. Not sure how we would recommend that.

@FMurray
Copy link
Author

FMurray commented Dec 27, 2024

LGTM with comments. Can you take another look at the local running part? I know some code developers may prefer that way. Not sure how we would recommend that.

I ensured that the general instructions are valid but not that every example runs locally. I think it can be left to contributors of to test that independently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants