fix: replace broken S3 cleanup with LlamaStack files API#1210
fix: replace broken S3 cleanup with LlamaStack files API#1210jgarciao merged 2 commits intoopendatahub-io:mainfrom
Conversation
|
The following are automatically added/executed:
Available user actions:
Supported labels{'/wip', '/build-push-pr-image', '/lgtm', '/cherry-pick', '/verified', '/hold'} |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited) Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughReplaced S3-based test cleanup with client-driven cleanup: snapshot existing file IDs via Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Actionable Issues
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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 Tip CodeRabbit can approve the review once all CodeRabbit's comments are resolved.Enable the |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/llama_stack/conftest.py`:
- Around line 569-572: Narrow the broad except handlers in the cleanup function
to catch only llama_stack_client.APIError (or import APIError with from
llama_stack_client import APIError) instead of Exception; replace both "except
Exception as e" blocks that log via LOGGER.warning(f"Failed to delete file
{file.id}: {e}") and LOGGER.warning(f"Failed to clean up files: {e}") so they
only swallow SDK/API errors while letting other exceptions surface.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: f3812329-a938-44f6-8c20-37042d49e047
📒 Files selected for processing (1)
tests/llama_stack/conftest.py
Bobbins228
left a comment
Bobbins228
left a comment
There was a problem hiding this comment.
/lgtm bar code rabbit nit
were you able to verify the bucket had been cleaned?
Yes but as per description only cleans itself, so there is still files from the previous runs that is not touched. |
jiripetrlik
left a comment
jiripetrlik
left a comment
There was a problem hiding this comment.
@Ygnas
It often happens that Llama stack distribution does not start/work properly when we are testing it. If I understand correctly this clean up will not work when Llama stack does not work. Please correct me if I'm wrong. Maybe it would be good to keep clean up logic independent of Llama stack which we are testing.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/llama_stack/conftest.py`:
- Around line 544-548: The current snapshot-and-cleanup in the conftest fixture
(existing_file_ids = {f.id for f in client.files.list().data}; yield client;
_cleanup_files(client=client, existing_file_ids=existing_file_ids)) has a TOCTOU
race that can delete other sessions' files when tests run in parallel; fix it by
scoping cleanup to this test session: modify the fixture and _cleanup_files to
use a unique session/run identifier (e.g., generate a run_id at fixture setup
and require that created files include this run_id in their metadata or
filename), filter client.files.list() by that run_id when building
existing_file_ids and when deleting files so only files created by this session
are removed, and ensure the client/file-creation helpers in tests add the
run_id; alternatively, if preferring documentation, update the conftest fixture
docstring to explicitly state that tests must use isolated LlamaStack instances
when running with pytest-xdist instead of relying on snapshot cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: 0ed30bc4-5bf0-4674-9c41-92eb0d359e52
📒 Files selected for processing (1)
tests/llama_stack/conftest.py
If LlamaStack never starts, it blocks the fixture from yielding, so no tests run and no files are created - nothing to clean up? If I understand correctly. |
Not having the boto3 dependency was a bug in the automation. The files weren't being deleted because of that. I think this PR is correct. The only doubt I have is if the files should be cleared during the llama-stack-distribution or the client yield block. Let's discuss that in our meeting |
The S3 file cleanup was failing because boto3 is not a project dependency. Replace the direct S3 access with the LlamaStack client's files API (client.files.list/delete), which requires no additional dependencies. - Remove _cleanup_s3_files helper and its boto3 dependency - Remove S3 credential parameters from distribution fixtures - Add _cleanup_files using the LlamaStack files API - Snapshot existing file IDs before tests to only delete files created during the current test execution Signed-off-by: Ignas Baranauskas <ibaranau@redhat.com>
|
/cherry-pick 3.4ea1 |
The S3 file cleanup was failing because boto3 is not a project dependency. Replace the direct S3 access with the LlamaStack client's files API (client.files.list/delete), which requires no additional dependencies. - Remove _cleanup_s3_files helper and its boto3 dependency - Remove S3 credential parameters from distribution fixtures - Add _cleanup_files using the LlamaStack files API - Snapshot existing file IDs before tests to only delete files created during the current test execution Signed-off-by: Ignas Baranauskas <ibaranau@redhat.com> Co-authored-by: Jorge <jgarciao@users.noreply.github.com>
|
Status of building tag latest: success. |
|
Cherry pick action created PR #1223 successfully 🎉! |
|
/cherry-pick 3.3 |
|
Error cherry-picking. |
|
|
The S3 file cleanup was failing because boto3 is not a project dependency. Replace the direct S3 access with the LlamaStack client's files API (client.files.list/delete), which requires no additional dependencies. - Remove _cleanup_s3_files helper and its boto3 dependency - Remove S3 credential parameters from distribution fixtures - Add _cleanup_files using the LlamaStack files API - Snapshot existing file IDs before tests to only delete files created during the current test execution Signed-off-by: Ignas Baranauskas <ibaranau@redhat.com> Co-authored-by: Ignas Baranauskas <ibaranau@redhat.com> Co-authored-by: Jorge <jgarciao@users.noreply.github.com>
…-io#1210) The S3 file cleanup was failing because boto3 is not a project dependency. Replace the direct S3 access with the LlamaStack client's files API (client.files.list/delete), which requires no additional dependencies. - Remove _cleanup_s3_files helper and its boto3 dependency - Remove S3 credential parameters from distribution fixtures - Add _cleanup_files using the LlamaStack files API - Snapshot existing file IDs before tests to only delete files created during the current test execution Signed-off-by: Ignas Baranauskas <ibaranau@redhat.com> Co-authored-by: Jorge <jgarciao@users.noreply.github.com>
The S3 file cleanup was failing because boto3 is not a project dependency. Replace the direct S3 access with the LlamaStack client's files API (client.files.list/delete), which requires no additional dependencies. - Remove _cleanup_s3_files helper and its boto3 dependency - Remove S3 credential parameters from distribution fixtures - Add _cleanup_files using the LlamaStack files API - Snapshot existing file IDs before tests to only delete files created during the current test execution Signed-off-by: Ignas Baranauskas <ibaranau@redhat.com> Co-authored-by: Jorge <jgarciao@users.noreply.github.com>
…-io#1210) The S3 file cleanup was failing because boto3 is not a project dependency. Replace the direct S3 access with the LlamaStack client's files API (client.files.list/delete), which requires no additional dependencies. - Remove _cleanup_s3_files helper and its boto3 dependency - Remove S3 credential parameters from distribution fixtures - Add _cleanup_files using the LlamaStack files API - Snapshot existing file IDs before tests to only delete files created during the current test execution Signed-off-by: Ignas Baranauskas <ibaranau@redhat.com> Co-authored-by: Jorge <jgarciao@users.noreply.github.com> Signed-off-by: Shehan Saleem <ssaleem@redhat.com>
6 participants
The S3 file cleanup was failing because boto3 is not a project dependency. Replace the direct S3 access with the LlamaStack client's files API (client.files.list/delete), which requires no additional dependencies.
Pull Request
Summary
Related Issues
How it has been tested
Additional Requirements
Summary by CodeRabbit