-
Notifications
You must be signed in to change notification settings - Fork 183
feat: able to display file url from llamacloud #153
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
feat: able to display file url from llamacloud #153
Conversation
🦋 Changeset detectedLatest commit: 25b74a1 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThis update integrates Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 11
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (9)
- .changeset/happy-hairs-kick.md (1 hunks)
- templates/types/streaming/express/src/controllers/service.ts (1 hunks)
- templates/types/streaming/express/src/controllers/stream-helper.ts (3 hunks)
- templates/types/streaming/fastapi/app/api/routers/models.py (2 hunks)
- templates/types/streaming/fastapi/app/service.py (1 hunks)
- templates/types/streaming/nextjs/app/api/chat/service.ts (1 hunks)
- templates/types/streaming/nextjs/app/api/chat/stream-helper.ts (3 hunks)
- templates/types/streaming/nextjs/app/components/ui/chat/chat-message/chat-sources.tsx (5 hunks)
- templates/types/streaming/nextjs/app/components/ui/chat/widgets/PdfDialog.tsx (1 hunks)
Files skipped from review due to trivial changes (1)
- .changeset/happy-hairs-kick.md
Additional context used
Biome
templates/types/streaming/express/src/controllers/service.ts
[error] 7-48: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 37-37: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 42-42: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
templates/types/streaming/nextjs/app/api/chat/service.ts
[error] 7-48: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 37-37: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 42-42: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
Ruff
templates/types/streaming/fastapi/app/api/routers/models.py
3-3:
requests
imported but unusedRemove unused import:
requests
(F401)
Additional comments not posted (9)
templates/types/streaming/fastapi/app/service.py (2)
8-16
: Verify API key presence and add error handling.Ensure that the
LLAMA_CLOUD_API_KEY
environment variable is set. Add error handling for the request to manage potential failures.+ if not os.getenv("LLAMA_CLOUD_API_KEY"): + raise EnvironmentError("LLAMA_CLOUD_API_KEY not set") response = requests.request("GET", url, headers=headers, data=payload) + if response.status_code != 200: + raise requests.HTTPError(f"Failed to fetch files: {response.status_code}")
19-27
: Verify API key presence and add error handling.Ensure that the
LLAMA_CLOUD_API_KEY
environment variable is set. Add error handling for the request to manage potential failures.+ if not os.getenv("LLAMA_CLOUD_API_KEY"): + raise EnvironmentError("LLAMA_CLOUD_API_KEY not set") response = requests.request("GET", url, headers=headers, data=payload) + if response.status_code != 200: + raise requests.HTTPError(f"Failed to fetch file detail: {response.status_code}")templates/types/streaming/nextjs/app/components/ui/chat/widgets/PdfDialog.tsx (1)
27-32
: LGTM!The JSX changes to display the file URL with improved styling and structure look good to me.
templates/types/streaming/express/src/controllers/service.ts (2)
8-16
: Verify API key presence, add error handling, and consider refactoring.Ensure that the
LLAMA_CLOUD_API_KEY
environment variable is set. Add error handling for the request to manage potential failures. Consider refactoring to a simple function as the class contains only static members.+ if (!process.env.LLAMA_CLOUD_API_KEY) { + throw new Error("LLAMA_CLOUD_API_KEY not set"); + } const response = await fetch(url, { method: "GET", headers }); + if (!response.ok) { + throw new Error(`Failed to fetch files: ${response.status}`); + } const data = await response.json(); return data;
19-31
: Verify API key presence, add error handling, and consider refactoring.Ensure that the
LLAMA_CLOUD_API_KEY
environment variable is set. Add error handling for the request to manage potential failures. Consider refactoring to a simple function as the class contains only static members.+ if (!process.env.LLAMA_CLOUD_API_KEY) { + throw new Error("LLAMA_CLOUD_API_KEY not set"); + } const response = await fetch(url, { method: "GET", headers }); + if (!response.ok) { + throw new Error(`Failed to fetch file detail: ${response.status}`); + } const data = (await response.json()) as { url: string }; return data;templates/types/streaming/nextjs/app/api/chat/service.ts (2)
8-16
: Verify API key presence, add error handling, and consider refactoring.Ensure that the
LLAMA_CLOUD_API_KEY
environment variable is set. Add error handling for the request to manage potential failures. Consider refactoring to a simple function as the class contains only static members.+ if (!process.env.LLAMA_CLOUD_API_KEY) { + throw new Error("LLAMA_CLOUD_API_KEY not set"); + } const response = await fetch(url, { method: "GET", headers }); + if (!response.ok) { + throw new Error(`Failed to fetch files: ${response.status}`); + } const data = await response.json(); return data;
19-31
: Verify API key presence, add error handling, and consider refactoring.Ensure that the
LLAMA_CLOUD_API_KEY
environment variable is set. Add error handling for the request to manage potential failures. Consider refactoring to a simple function as the class contains only static members.+ if (!process.env.LLAMA_CLOUD_API_KEY) { + throw new Error("LLAMA_CLOUD_API_KEY not set"); + } const response = await fetch(url, { method: "GET", headers }); + if (!response.ok) { + throw new Error(`Failed to fetch file detail: ${response.status}`); + } const data = (await response.json()) as { url: string }; return data;templates/types/streaming/nextjs/app/components/ui/chat/chat-message/chat-sources.tsx (2)
Line range hint
26-64
: LGTM!The changes in the
ChatSources
component enhance its functionality by adding new logic to handle and display file URLs. The filtering and sorting logic is implemented correctly.
72-117
: LGTM!The new
SourceNodeItem
component correctly handles the display of file URLs and conditionally displays aPdfDialog
component based on the validity of PDF URLs. The logic to check the validity of PDF URLs is implemented correctly.
templates/types/streaming/express/src/controllers/stream-helper.ts
Outdated
Show resolved
Hide resolved
templates/types/streaming/express/src/controllers/stream-helper.ts
Outdated
Show resolved
Hide resolved
templates/types/streaming/express/src/controllers/stream-helper.ts
Outdated
Show resolved
Hide resolved
templates/types/streaming/express/src/controllers/stream-helper.ts
Outdated
Show resolved
Hide resolved
templates/types/streaming/nextjs/app/components/ui/chat/chat-message/chat-sources.tsx
Outdated
Show resolved
Hide resolved
templates/types/streaming/nextjs/app/components/ui/chat/chat-message/chat-sources.tsx
Outdated
Show resolved
Hide resolved
templates/types/streaming/nextjs/app/components/ui/chat/chat-message/chat-sources.tsx
Outdated
Show resolved
Hide resolved
templates/types/streaming/nextjs/app/components/ui/chat/chat-message/chat-sources.tsx
Outdated
Show resolved
Hide resolved
templates/types/streaming/nextjs/app/components/ui/chat/chat-message/chat-sources.tsx
Outdated
Show resolved
Hide resolved
…b.com:run-llama/create-llama into feat/able-to-display-file-url-from-llamacloud
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- templates/types/streaming/express/src/controllers/stream-helper.ts (3 hunks)
- templates/types/streaming/fastapi/app/api/routers/models.py (2 hunks)
- templates/types/streaming/nextjs/app/api/chat/stream-helper.ts (3 hunks)
- templates/types/streaming/nextjs/app/components/ui/chat/chat-message/chat-sources.tsx (5 hunks)
Files skipped from review due to trivial changes (1)
- templates/types/streaming/nextjs/app/components/ui/chat/chat-message/chat-sources.tsx
Files skipped from review as they are similar to previous changes (3)
- templates/types/streaming/express/src/controllers/stream-helper.ts
- templates/types/streaming/fastapi/app/api/routers/models.py
- templates/types/streaming/nextjs/app/api/chat/stream-helper.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 10
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (4)
- templates/types/streaming/express/src/controllers/service.ts (1 hunks)
- templates/types/streaming/express/src/controllers/stream-helper.ts (3 hunks)
- templates/types/streaming/nextjs/app/api/chat/service.ts (1 hunks)
- templates/types/streaming/nextjs/app/api/chat/stream-helper.ts (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- templates/types/streaming/express/src/controllers/stream-helper.ts
- templates/types/streaming/nextjs/app/api/chat/stream-helper.ts
Additional context used
Biome
templates/types/streaming/express/src/controllers/service.ts
[error] 11-102: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 42-42: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 47-47: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 48-48: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
templates/types/streaming/nextjs/app/api/chat/service.ts
[error] 11-102: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
[error] 42-42: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 47-47: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 48-48: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
@marcusschiesser @leehuwuj I have updated code to save LlamaCloud file in local and return the local url. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- templates/types/streaming/fastapi/app/service.py (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- templates/types/streaming/fastapi/app/service.py
TS code looks good - @leehuwuj please check the python code and then you can merge it |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- templates/components/vectordbs/python/none/generate.py (2 hunks)
- templates/types/streaming/fastapi/app/api/routers/chat.py (3 hunks)
- templates/types/streaming/fastapi/app/engine/generate.py (2 hunks)
Additional context used
Ruff
templates/types/streaming/fastapi/app/api/routers/chat.py
47-47: Do not perform function call
Depends
in argument defaults; instead, perform the call within the function, or read the default from a module-level singleton variable(B008)
Additional comments not posted (5)
templates/components/vectordbs/python/none/generate.py (2)
6-9
: Review of import statements and orderThe reordering of imports and the addition of new import statements (
get_documents
andinit_settings
) are consistent with Python best practices, which recommend grouping similar types of imports together. However, the removal of thelogging
import should be verified to ensure logging is not used elsewhere in the code without proper import.
27-27
: Addition ofis_local_file
metadataThe addition of the
is_local_file
metadata field aligns with the PR's objective to manage files from LlamaCloud locally. This change is straightforward and correctly implemented within the loop for setting document metadata.templates/types/streaming/fastapi/app/engine/generate.py (2)
6-15
: Review of import statements and orderThe reordering of import statements to place custom package imports (
from app.engine.loaders
and others) above standard library imports (os
) is a common practice to improve readability. The addition ofget_vector_store
suggests an expansion in functionality which should be cross-verified with usage in the rest of the module.
70-70
: Addition ofis_local_file
metadataSimilar to the previous file, the addition of the
is_local_file
metadata field is implemented correctly. It's important to ensure that this new metadata field is consistently used across all relevant parts of the application to avoid inconsistencies.templates/types/streaming/fastapi/app/api/routers/chat.py (1)
2-20
: Review of new imports and their usageThe addition of
List
fromtyping
andBackgroundTasks
fromFastAPI
are appropriate for the functionalities described. The import ofNodeWithScore
should be verified against its usage in the file to ensure it's not an unused import as previously flagged.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- templates/components/llamaindex/typescript/documents/pipeline.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- templates/components/llamaindex/typescript/documents/pipeline.ts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- templates/types/streaming/express/package.json (1 hunks)
Files skipped from review due to trivial changes (1)
- templates/types/streaming/express/package.json
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- templates/types/streaming/fastapi/app/api/routers/models.py (6 hunks)
- templates/types/streaming/fastapi/app/api/services/file.py (2 hunks)
Files skipped from review as they are similar to previous changes (2)
- templates/types/streaming/fastapi/app/api/routers/models.py
- templates/types/streaming/fastapi/app/api/services/file.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- templates/types/streaming/fastapi/main.py (2 hunks)
Additional comments not posted (3)
templates/types/streaming/fastapi/main.py (3)
7-15
: Import reordering looks good.Reordering the imports for
FastAPI
,CORSMiddleware
, andRedirectResponse
below the router imports is acceptable and maintains consistency.
12-13
: Verify the reasoning behind swapping function calls.Swapping
init_settings()
andinit_observability()
might be to ensure settings are initialized before observability is set up. Verify if this change is necessary and correct.
50-50
: Rearrangement of static file mounting looks good.The rearrangement of static file mounting is acceptable and maintains consistency.
586189f
to
2bb7b67
Compare
…file-url-from-llamacloud
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- templates/types/streaming/fastapi/app/api/services/file.py (3 hunks)
Files skipped from review as they are similar to previous changes (1)
- templates/types/streaming/fastapi/app/api/services/file.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (3)
- templates/components/vectordbs/python/none/generate.py (2 hunks)
- templates/types/streaming/fastapi/app/api/services/llama_cloud.py (1 hunks)
- templates/types/streaming/fastapi/app/engine/generate.py (2 hunks)
Files skipped from review as they are similar to previous changes (3)
- templates/components/vectordbs/python/none/generate.py
- templates/types/streaming/fastapi/app/api/services/llama_cloud.py
- templates/types/streaming/fastapi/app/engine/generate.py
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- templates/components/vectordbs/typescript/llamacloud/generate.ts (2 hunks)
Additional comments not posted (1)
templates/components/vectordbs/typescript/llamacloud/generate.ts (1)
3-3
: Import statement is correct.The new import statement for
getDataSource
is syntactically correct and aligns with the AI-generated summary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Outside diff range, codebase verification and nitpick comments (1)
.changeset/happy-hairs-kick.md (1)
5-5
: Clarify the changeset description.The description should be more specific about the changes made. Consider mentioning that the changes involve displaying files in sources using LlamaCloud indexes and saving files locally with their URLs.
- Display files in sources using LlamaCloud indexes. + Display files in sources using LlamaCloud indexes and save files locally with their URLs.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (11)
- .changeset/happy-hairs-kick.md (1 hunks)
- templates/components/llamaindex/typescript/documents/helper.ts (1 hunks)
- templates/components/llamaindex/typescript/documents/pipeline.ts (1 hunks)
- templates/components/llamaindex/typescript/streaming/events.ts (3 hunks)
- templates/components/vectordbs/python/llamacloud/generate.py (1 hunks)
- templates/components/vectordbs/python/none/generate.py (2 hunks)
- templates/components/vectordbs/typescript/llamacloud/generate.ts (2 hunks)
- templates/types/streaming/fastapi/app/api/routers/models.py (6 hunks)
- templates/types/streaming/fastapi/app/api/services/file.py (3 hunks)
- templates/types/streaming/fastapi/app/engine/generate.py (2 hunks)
- templates/types/streaming/fastapi/main.py (2 hunks)
Files skipped from review as they are similar to previous changes (8)
- templates/components/llamaindex/typescript/documents/helper.ts
- templates/components/llamaindex/typescript/documents/pipeline.ts
- templates/components/vectordbs/python/none/generate.py
- templates/components/vectordbs/typescript/llamacloud/generate.ts
- templates/types/streaming/fastapi/app/api/routers/models.py
- templates/types/streaming/fastapi/app/api/services/file.py
- templates/types/streaming/fastapi/app/engine/generate.py
- templates/types/streaming/fastapi/main.py
Additional comments not posted (4)
templates/components/vectordbs/python/llamacloud/generate.py (1)
33-35
: Ensure metadata field is properly set.The loop correctly sets the
is_local_file
metadata field for each document. Ensure that all documents have ametadata
attribute and that it is a dictionary.templates/components/llamaindex/typescript/streaming/events.ts (3)
11-33
: Ensure proper error handling inappendSourceData
.The function correctly handles errors with a
try-catch
block. Ensure that all possible errors are logged and handled appropriately.
Line range hint
79-87
:
Ensure proper error handling incallbackManager.on("retrieve-end")
.The function correctly handles errors with a
try-catch
block inappendSourceData
. Ensure that any errors inappendEventData
are also logged and handled appropriately.
108-130
: Ensure proper error handling ingetNodeUrl
.The function correctly handles errors with a
try-catch
block. Ensure that all possible errors are logged and handled appropriately.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- helpers/index.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- helpers/index.ts
const pipelineId = metadata["pipeline_id"]; | ||
if (pipelineId && !isLocalFile) { | ||
// file is from LlamaCloud and was not ingested locally | ||
// TODO trigger but don't await file download and just use convention to generate the URL (see Python code) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@thucpn see TODO
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes