Metadata agent #741
Metadata agent #741
Conversation
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
WalkthroughAdds a new Python agent (src/agents/metadata_extractor.py) implementing per-participant VAD (VADAgent), event model (MetadataEvent), orchestration and persistence to S3-compatible storage (MetadataAgent), plus entrypoint and prewarm. Adds minio==7.2.15 to src/agents/pyproject.toml. Updates Helm values to provide AWS S3-compatible env vars, run the metadata_extractor start command, and change the SSL certificate mount path. Adjusts agents Dockerfile CMD to invoke multi_user_transcriber.py. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Pre-merge checks✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
src/agents/Dockerfile(1 hunks)src/agents/metadata_extractor.py(1 hunks)src/agents/pyproject.toml(1 hunks)src/helm/env.d/dev-keycloak/values.meet.yaml.gotmpl(1 hunks)
src/agents/metadata_extractor.py
Outdated
| AGENT_NAME = os.getenv("ROOM_METADATA_AGENT_NAME", "metadata-extractor") | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Remove unused AGENT_NAME constant.
Line 36 defines AGENT_NAME, but it is never referenced. Please drop it to avoid shipping dead code.
🤖 Prompt for AI Agents
In src/agents/metadata_extractor.py around lines 36 to 37, remove the unused
AGENT_NAME constant declaration (AGENT_NAME =
os.getenv("ROOM_METADATA_AGENT_NAME", "metadata-extractor")) to eliminate dead
code; ensure no other references exist to that name (search file/project) before
deleting, then run tests/lint to confirm there are no remaining usages or
import-order changes required.
src/agents/metadata_extractor.py
Outdated
| def __init__(self, ctx: JobContext): | ||
| """Initialize metadata agent.""" | ||
| self.minio_client = Minio( | ||
| endpoint=os.getenv("AWS_S3_ENDPOINT_URL"), | ||
| access_key=os.getenv("AWS_S3_ACCESS_KEY_ID"), | ||
| secret_key=os.getenv("AWS_S3_SECRET_ACCESS_KEY"), | ||
| secure=os.getenv("AWS_S3_SECURE_ACCESS", "False").lower() == "true", | ||
| ) | ||
|
|
||
| # todo - raise error if none | ||
| self.bucket_name = os.getenv("AWS_STORAGE_BUCKET_NAME") | ||
|
|
There was a problem hiding this comment.
Raise when S3 endpoint or bucket env vars are missing.
Lines 98-107 build the MinIO client before verifying the required configuration. If AWS_S3_ENDPOINT_URL or AWS_STORAGE_BUCKET_NAME is absent (the TODO on Line 105), the agent survives initialization but crashes at shutdown when save() calls put_object, making the failure harder to diagnose. Fail fast by validating these env vars upfront.
- self.minio_client = Minio(
- endpoint=os.getenv("AWS_S3_ENDPOINT_URL"),
+ endpoint = os.getenv("AWS_S3_ENDPOINT_URL")
+ if not endpoint:
+ raise RuntimeError(
+ "AWS_S3_ENDPOINT_URL is required for metadata persistence."
+ )
+
+ self.bucket_name = os.getenv("AWS_STORAGE_BUCKET_NAME")
+ if not self.bucket_name:
+ raise RuntimeError(
+ "AWS_STORAGE_BUCKET_NAME is required for metadata persistence."
+ )
+
+ self.minio_client = Minio(
+ endpoint=endpoint,
access_key=os.getenv("AWS_S3_ACCESS_KEY_ID"),
secret_key=os.getenv("AWS_S3_SECRET_ACCESS_KEY"),
secure=os.getenv("AWS_S3_SECURE_ACCESS", "False").lower() == "true",
)
-
- # todo - raise error if none
- self.bucket_name = os.getenv("AWS_STORAGE_BUCKET_NAME")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def __init__(self, ctx: JobContext): | |
| """Initialize metadata agent.""" | |
| self.minio_client = Minio( | |
| endpoint=os.getenv("AWS_S3_ENDPOINT_URL"), | |
| access_key=os.getenv("AWS_S3_ACCESS_KEY_ID"), | |
| secret_key=os.getenv("AWS_S3_SECRET_ACCESS_KEY"), | |
| secure=os.getenv("AWS_S3_SECURE_ACCESS", "False").lower() == "true", | |
| ) | |
| # todo - raise error if none | |
| self.bucket_name = os.getenv("AWS_STORAGE_BUCKET_NAME") | |
| def __init__(self, ctx: JobContext): | |
| """Initialize metadata agent.""" | |
| endpoint = os.getenv("AWS_S3_ENDPOINT_URL") | |
| if not endpoint: | |
| raise RuntimeError( | |
| "AWS_S3_ENDPOINT_URL is required for metadata persistence." | |
| ) | |
| self.bucket_name = os.getenv("AWS_STORAGE_BUCKET_NAME") | |
| if not self.bucket_name: | |
| raise RuntimeError( | |
| "AWS_STORAGE_BUCKET_NAME is required for metadata persistence." | |
| ) | |
| self.minio_client = Minio( | |
| endpoint=endpoint, | |
| access_key=os.getenv("AWS_S3_ACCESS_KEY_ID"), | |
| secret_key=os.getenv("AWS_S3_SECRET_ACCESS_KEY"), | |
| secure=os.getenv("AWS_S3_SECURE_ACCESS", "False").lower() == "true", | |
| ) |
🤖 Prompt for AI Agents
In src/agents/metadata_extractor.py around lines 96 to 107, the Minio client and
bucket_name are created without validating required environment variables; add
upfront validation to fail fast: check that AWS_S3_ENDPOINT_URL and
AWS_STORAGE_BUCKET_NAME (and optionally
AWS_S3_ACCESS_KEY_ID/AWS_S3_SECRET_ACCESS_KEY if required) are present and raise
a clear exception (ValueError or custom) if missing before constructing Minio,
so initialization fails immediately with an explanatory message rather than
crashing later during save().
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
src/agents/metadata_extractor.py (2)
38-38: AGENT_NAME is used, not dead code.Contrary to a previous review comment,
AGENT_NAMEis actively used on line 345 (to constructagent_identity) and line 381 (as a WorkerOptions parameter). The constant should remain.
100-110: Missing environment variable validation causes delayed failures.Lines 102-110 construct the MinIO client and retrieve the bucket name without validating that required environment variables are present. If
AWS_S3_ENDPOINT_URLorAWS_STORAGE_BUCKET_NAMEis missing, the agent initializes successfully but crashes later duringsave()(line 179), making the failure harder to diagnose. The TODO comment on line 109 acknowledges this gap.Fail fast by validating these variables upfront.
Apply this diff to add validation:
+ endpoint = os.getenv("AWS_S3_ENDPOINT_URL") + if not endpoint: + raise ValueError("AWS_S3_ENDPOINT_URL is required for metadata persistence") + + bucket_name = os.getenv("AWS_STORAGE_BUCKET_NAME") + if not bucket_name: + raise ValueError("AWS_STORAGE_BUCKET_NAME is required for metadata persistence") + self.minio_client = Minio( - endpoint=os.getenv("AWS_S3_ENDPOINT_URL"), + endpoint=endpoint, access_key=os.getenv("AWS_S3_ACCESS_KEY_ID"), secret_key=os.getenv("AWS_S3_SECRET_ACCESS_KEY"), secure=os.getenv("AWS_S3_SECURE_ACCESS", "False").lower() == "true", ) - # todo - raise error if none - self.bucket_name = os.getenv("AWS_STORAGE_BUCKET_NAME") + self.bucket_name = bucket_name
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
src/agents/Dockerfile(1 hunks)src/agents/metadata_extractor.py(1 hunks)src/agents/pyproject.toml(1 hunks)src/helm/env.d/dev-keycloak/values.meet.yaml.gotmpl(1 hunks)
🔇 Additional comments (3)
src/helm/env.d/dev-keycloak/values.meet.yaml.gotmpl (1)
284-285: Verify the certificate mount path change.The certificate mount path was changed from the Python certifi path to
/usr/lib/ssl/cert.pem. Ensure this path is recognized by the MinIO client and other components for SSL/TLS verification. If this change is intentional, consider documenting the rationale.src/agents/metadata_extractor.py (2)
342-367: LGTM!The job request handler correctly prevents duplicate agents by querying the LiveKit API for existing agents with the same identity before accepting. Error handling and logging are appropriate.
321-339: LGTM!The
entrypointfunction correctly initializes the metadata agent, connects to the room with audio-only subscription, processes existing participants, and registers a cleanup callback. Theprewarmfunction appropriately preloads the VAD model to reduce cold-start latency.Also applies to: 370-372
| COPY . . | ||
|
|
||
| CMD ["python", "multi-user-transcriber.py", "start"] | ||
| CMD ["python", "multi_user_transcriber.py", "start"] |
There was a problem hiding this comment.
Inconsistent startup command: Dockerfile vs. Helm configuration.
The Dockerfile invokes multi_user_transcriber.py start, but the Helm values (src/helm/env.d/dev-keycloak/values.meet.yaml.gotmpl, lines 276-279) override this with metadata_extractor.py start. This creates confusion about which script is the intended entrypoint for the metadata extraction agent.
For consistency and clarity, align the Dockerfile CMD with the intended agent script.
If metadata_extractor.py is the correct entrypoint, apply this diff:
-CMD ["python", "multi_user_transcriber.py", "start"]
+CMD ["python", "metadata_extractor.py", "start"]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| CMD ["python", "multi_user_transcriber.py", "start"] | |
| CMD ["python", "metadata_extractor.py", "start"] |
🤖 Prompt for AI Agents
In src/agents/Dockerfile around line 24, the Dockerfile CMD currently starts the
wrong script ("multi_user_transcriber.py start") which conflicts with the Helm
values that expect "metadata_extractor.py start"; update the Dockerfile CMD to
invoke "metadata_extractor.py start" so the container entrypoint matches the
Helm configuration and avoid startup inconsistencies.
| async def on_enter(self) -> None: | ||
| """Initialize VAD monitoring for this participant.""" | ||
|
|
||
| @self.session.on("user_state_changed") | ||
| def on_user_state(event): | ||
| timestamp = datetime.now(timezone.utc) | ||
|
|
||
| if event.new_state == "speaking": | ||
| event = MetadataEvent( | ||
| participant_id=self.participant_identity, | ||
| type="speech_start", | ||
| timestamp=timestamp, | ||
| ) | ||
| self.events.append(event) | ||
|
|
||
| elif event.old_state == "speaking": | ||
| event = MetadataEvent( | ||
| participant_id=self.participant_identity, | ||
| type="speech_end", | ||
| timestamp=timestamp, | ||
| ) | ||
| self.events.append(event) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Variable shadowing in VAD state change handler.
Lines 76 and 84 create MetadataEvent instances named event, which shadows the callback parameter event (line 72). While the code functions correctly, this shadowing reduces clarity.
Apply this diff to eliminate shadowing:
@self.session.on("user_state_changed")
def on_user_state(event):
timestamp = datetime.now(timezone.utc)
if event.new_state == "speaking":
- event = MetadataEvent(
+ speech_start = MetadataEvent(
participant_id=self.participant_identity,
type="speech_start",
timestamp=timestamp,
)
- self.events.append(event)
+ self.events.append(speech_start)
elif event.old_state == "speaking":
- event = MetadataEvent(
+ speech_end = MetadataEvent(
participant_id=self.participant_identity,
type="speech_end",
timestamp=timestamp,
)
- self.events.append(event)
+ self.events.append(speech_end)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async def on_enter(self) -> None: | |
| """Initialize VAD monitoring for this participant.""" | |
| @self.session.on("user_state_changed") | |
| def on_user_state(event): | |
| timestamp = datetime.now(timezone.utc) | |
| if event.new_state == "speaking": | |
| event = MetadataEvent( | |
| participant_id=self.participant_identity, | |
| type="speech_start", | |
| timestamp=timestamp, | |
| ) | |
| self.events.append(event) | |
| elif event.old_state == "speaking": | |
| event = MetadataEvent( | |
| participant_id=self.participant_identity, | |
| type="speech_end", | |
| timestamp=timestamp, | |
| ) | |
| self.events.append(event) | |
| async def on_enter(self) -> None: | |
| """Initialize VAD monitoring for this participant.""" | |
| @self.session.on("user_state_changed") | |
| def on_user_state(event): | |
| timestamp = datetime.now(timezone.utc) | |
| if event.new_state == "speaking": | |
| speech_start = MetadataEvent( | |
| participant_id=self.participant_identity, | |
| type="speech_start", | |
| timestamp=timestamp, | |
| ) | |
| self.events.append(speech_start) | |
| elif event.old_state == "speaking": | |
| speech_end = MetadataEvent( | |
| participant_id=self.participant_identity, | |
| type="speech_end", | |
| timestamp=timestamp, | |
| ) | |
| self.events.append(speech_end) |
🤖 Prompt for AI Agents
In src/agents/metadata_extractor.py around lines 68 to 89, the callback
parameter event is being shadowed by local variables named event when creating
MetadataEvent instances; rename those local variables (e.g., metadata_event or
mevt) so they do not overwrite the callback parameter, and update the subsequent
self.events.append(...) calls to append the new variable name to preserve
clarity and avoid shadowing.
| try: | ||
| self.minio_client.put_object( | ||
| self.bucket_name, | ||
| object_name, | ||
| stream, | ||
| length=len(data), | ||
| content_type="application/json", | ||
| ) | ||
| logger.info( | ||
| "Uploaded speaker meeting metadata", | ||
| ) | ||
| except S3Error: | ||
| logger.exception( | ||
| "Failed to upload meeting metadata", | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Broaden exception handling to prevent uncaught errors.
Line 189 catches only S3Error, but other exceptions (e.g., network errors, credential issues, or SDK bugs) may occur during the upload. Uncaught exceptions could crash the agent during cleanup.
Apply this diff to catch all exceptions:
try:
self.minio_client.put_object(
self.bucket_name,
object_name,
stream,
length=len(data),
content_type="application/json",
)
logger.info(
"Uploaded speaker meeting metadata",
)
- except S3Error:
+ except Exception:
logger.exception(
"Failed to upload meeting metadata",
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| self.minio_client.put_object( | |
| self.bucket_name, | |
| object_name, | |
| stream, | |
| length=len(data), | |
| content_type="application/json", | |
| ) | |
| logger.info( | |
| "Uploaded speaker meeting metadata", | |
| ) | |
| except S3Error: | |
| logger.exception( | |
| "Failed to upload meeting metadata", | |
| ) | |
| try: | |
| self.minio_client.put_object( | |
| self.bucket_name, | |
| object_name, | |
| stream, | |
| length=len(data), | |
| content_type="application/json", | |
| ) | |
| logger.info( | |
| "Uploaded speaker meeting metadata", | |
| ) | |
| except Exception: | |
| logger.exception( | |
| "Failed to upload meeting metadata", | |
| ) |
🤖 Prompt for AI Agents
In src/agents/metadata_extractor.py around lines 178 to 192, the current upload
block only catches S3Error which can leave other failures uncaught; change the
exception handling to catch all exceptions (except Exception as e) so
network/credential/SDK errors are handled, and log the full exception/traceback
(use logger.exception with a descriptive message) to avoid crashing during
cleanup while keeping the existing success log; do not re-raise so the agent can
continue cleanup.
| "livekit-plugins-silero==1.2.6", | ||
| "python-dotenv==1.1.1" | ||
| "python-dotenv==1.1.1", | ||
| "minio==7.2.15" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
Verify the minio package version for validity and security.
Since my knowledge cutoff is outdated, please verify that minio==7.2.15 is a valid, current, and secure version.
Run the following script to check for the latest version and any security advisories:
🏁 Script executed:
#!/bin/bash
# Description: Check PyPI for latest minio version and GitHub security advisories.
# Check PyPI for latest version
echo "=== Latest minio version on PyPI ==="
curl -s https://pypi.org/pypi/minio/json | jq -r '.info.version'
# Check for security advisories
echo -e "\n=== Security advisories for minio (Python) ==="
gh api graphql -f query='
{
securityVulnerabilities(first: 10, ecosystem: PIP, package: "minio") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'Length of output: 141
Upgrade minio dependency to 7.2.18
No security advisories were found; bump to the latest patch for bug fixes.
🤖 Prompt for AI Agents
In src/agents/pyproject.toml at line 11, the minio dependency is pinned to
"minio==7.2.15"; update this entry to "minio==7.2.18" to bump to the latest
patch release, then regenerate/update the lockfile (e.g., run your dependency
tool's lock/install command) to ensure the new version is reflected in the
lockfile and CI.
| LIVEKIT_API_KEY: {{ $key }} | ||
| {{- end }} | ||
| {{- end }} | ||
| AWS_S3_ENDPOINT_URL: minio.meet.svc.cluster.local:9000 |
There was a problem hiding this comment.
Inconsistent S3 endpoint URL format.
Line 265 specifies AWS_S3_ENDPOINT_URL: minio.meet.svc.cluster.local:9000 without a protocol prefix, while other sections (lines 60, 151, 187, 224) use http://minio.meet.svc.cluster.local:9000. The MinIO Python client may require an explicit protocol.
Apply this diff to ensure consistency and prevent connection errors:
- AWS_S3_ENDPOINT_URL: minio.meet.svc.cluster.local:9000
+ AWS_S3_ENDPOINT_URL: http://minio.meet.svc.cluster.local:9000📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| AWS_S3_ENDPOINT_URL: minio.meet.svc.cluster.local:9000 | |
| AWS_S3_ENDPOINT_URL: http://minio.meet.svc.cluster.local:9000 |
🤖 Prompt for AI Agents
In src/helm/env.d/dev-keycloak/values.meet.yaml.gotmpl around line 265, the
AWS_S3_ENDPOINT_URL is missing the protocol (currently
"minio.meet.svc.cluster.local:9000") while other entries use "http://...";
update this value to include the explicit protocol (e.g.
"http://minio.meet.svc.cluster.local:9000") to match the other entries and
ensure MinIO clients receive a full URL.
|
Copy the metadata agent from "Metadata agent #741" branch to have the VAD agent and the right output format for metadatas
/!\ not ready for merge, example of my researches for @GuittenyMartin.
The agent outputs the following data structure :
We could extend this data structure to capture even more event or other types of data.
Ex : when someone mutes its mic., raises hand, send a chat message.