Conversation
| return !sessionId || filename.startsWith(USER_NAMESPACE_PREFIX); | ||
| } | ||
|
|
||
| function getUserArtifactsDir(baseRoot: string): string { |
There was a problem hiding this comment.
Doesn't look like this was done in these two methods
| try { | ||
| await fs.access(artifactDir); | ||
| } catch { | ||
| return undefined; |
There was a problem hiding this comment.
Is this worth logging for debug purposes?
This goes for the rest of the "return undefined;" here, just not leaving a comment for each one. Some of them might point to bugs, like no version for a filename.
There was a problem hiding this comment.
It looks like the rest were changed but not this one
kalenkevich
force-pushed
the
feat/file_artifact_service
branch
7 times, most recently
from
35a726b to
396f1b5
Compare
kalenkevich
force-pushed
the
feat/file_artifact_service
branch
3 times, most recently
from
109eca2 to
2470ce8
Compare
ScottMansfield
left a comment
ScottMansfield
left a comment
There was a problem hiding this comment.
I don't think this needs to be resolved in this PR but we should consider the similarities of a filesystem in GCS and a filesystem on disk. They're not too different and maybe the code could be largely shared.
| let mimeType: string | undefined; | ||
| if (artifact.inlineData) { | ||
| const data = artifact.inlineData.data || ''; | ||
| // We need to store binary data in base64 format. |
There was a problem hiding this comment.
nit: This can still be more clear. GenAI SDK Part data is in Base64 format. See https://googleapis.github.io/js-genai/release_docs/interfaces/types.Part.html
| try { | ||
| await fs.access(artifactDir); | ||
| } catch { | ||
| return undefined; |
There was a problem hiding this comment.
It looks like the rest were changed but not this one
| return !sessionId || filename.startsWith(USER_NAMESPACE_PREFIX); | ||
| } | ||
|
|
||
| function getUserArtifactsDir(baseRoot: string): string { |
There was a problem hiding this comment.
Doesn't look like this was done in these two methods
| const relative = file.name.substring(usernamePrefix.length); | ||
| const parts = relative.split('/'); | ||
| if (parts.length >= 2) { | ||
| fileNames.add(`${parts.slice(0, -1).join('/')}`); |
There was a problem hiding this comment.
unnecessary interpolation
| if (!file.name.startsWith(prefix)) continue; | ||
| const relative = file.name.substring(prefix.length); | ||
| const parts = relative.split('/'); | ||
| // Expect at least name and version: name/version | ||
| // If name contains slashes: dir/name/version | ||
| if (parts.length < 2) continue; | ||
|
|
||
| const name = parts.slice(0, -1).join('/'); |
There was a problem hiding this comment.
This is the third time this logic shows up in these few functions. I was going to ignore it for 2, but with a third this needs to be factored out into a function
kalenkevich
force-pushed
the
feat/file_artifact_service
branch
from
9462f0d to
fd38679
Compare
kalenkevich
force-pushed
the
feat/file_artifact_service
branch
from
fd38679 to
6b710dd
Compare
2 participants
Link to Issue or Description of Change
1. Link to an existing issue (if applicable):
2. Or, if no issue exists, describe the change:
Problem:
The ADK TS lacked a local filesystem-backed artifact service (aligned with python file service), which is essential for local development and testing. Additionally, the existing
GcsArtifactServiceandInMemoryArtifactServicehad inconsistent implementations and lacked a shared test suite to ensure parity and correctness across all artifact service implementations.Solution:
This PR introduces
FileArtifactServiceto support local artifact storage with versioning and metadata management. It also establishes a shared test suite (runArtifactServiceTests) incore/test/artifacts/artifact_service_test_utils.tsthat enforces a consistent interface and behavior across all artifact services.GcsArtifactServiceandInMemoryArtifactServicehave been refactored and updated to pass this shared test suite, ensuring alignment and reliability.Testing Plan
Unit Tests:
Summary of passed npm test results:
core/test/artifacts/file_artifact_service_test.ts: VerifiesFileArtifactServiceusing the shared test suite.core/test/artifacts/gcs_artifact_service_test.ts: VerifiesGcsArtifactServiceimprovements and alignment.core/test/artifacts/in_memory_artifact_service_test.ts: VerifiesInMemoryArtifactServicefunctionality.Manual End-to-End (E2E) Tests:
Checklist
Additional context
This aligns the TypeScript implementation of
FileArtifactServicewith the Python ADK and ensures consistent behavior across artifact services.