-
Notifications
You must be signed in to change notification settings - Fork 277
Add object store #299
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
Add object store #299
Conversation
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.
This is a really good start. I have a few general suggestions:
- One goal of this was to allow accessing these objects from the fastapi server. I think we need to update the server configuration options to support something like this:
# Upload static files to the object store
async def static_files(file: UploadFile):
object_store_client = builder.get_object_store_client(self.front_end_config.object_store)
file_data = await file.read()
await object_store_client.put_object(file.filename, ObjectStoreItem(data=file_data, content_type=file.content_type))
return {"filename": file.filename}
app.add_api_route(
path="/static",
endpoint=static_files,
methods=["POST"],
)
# Get static files from the object store
async def get_static_files(file_name: str):
object_store_client = builder.get_object_store_client(self.front_end_config.object_store)
file_data = await object_store_client.get_object(file_name)
return FileResponse(file_data.data, media_type=file_data.content_type)
app.add_api_route(
path="/static/{file_name}",
endpoint=get_static_files,
methods=["GET"],
)
- The functions in the user_report example should be generalizable and could be included in the main library since they will work with any data store.
- Are there other more complex data store types that we should consider to ensure the interface works. Redis? MySQL?
- Will need to update documentation
packages/aiqtoolkit_s3_object_store/src/aiq/plugins/s3_object_store/s3_object_store.py
Outdated
Show resolved
Hide resolved
packages/aiqtoolkit_s3_object_store/src/aiq/plugins/s3_object_store/s3_object_store.py
Outdated
Show resolved
Hide resolved
927b14f
to
3a434d6
Compare
@balvisio We will need the DCO checks to pass before we can merge. Can you force push according to this: https://github.com/NVIDIA/NeMo-Agent-Toolkit/pull/299/checks?check_run_id=45251496229 |
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.
This is looking really, really good! A few more changes but they are mostly naming and documentation related.
One thing we this is missing is some unit tests on the object store interface (can test with the in-memory object store).
packages/aiqtoolkit_s3_object_store/src/aiq/plugins/s3_object_store/s3_object_store.py
Outdated
Show resolved
Hide resolved
@balvisio I noticed you responded with "Done" on some of Michael's comments but I don't see a commit reflecting those changes yet. Are you still working on addressing the other comments? To reduce your effort, I'd be happy to take over on the remaining feedback. |
Hello @willkill07 , it is ok, I will finish addressing the last set of comments. Thank you!. I will let you know when I push the last changes. |
3a434d6
to
2dfa308
Compare
@mdemoret-nv @willkill07 I have addressed the review comments. Thanks! |
Signed-off-by: Bruno Alvisio <[email protected]>
2dfa308
to
723cb6d
Compare
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.
Please change all mentions of AIQ toolkit or AgentIQ to NeMo Agent toolkit.
Signed-off-by: Michael Demoret <[email protected]>
Signed-off-by: Michael Demoret <[email protected]>
Signed-off-by: Michael Demoret <[email protected]>
Co-authored-by: lvojtku <[email protected]> Signed-off-by: Michael Demoret <[email protected]>
Co-authored-by: lvojtku <[email protected]> Signed-off-by: Michael Demoret <[email protected]>
Signed-off-by: Michael Demoret <[email protected]>
…bject-store Signed-off-by: Michael Demoret <[email protected]>
Signed-off-by: Michael Demoret <[email protected]>
Signed-off-by: Michael Demoret <[email protected]>
/ok to test 1ca5cd4 |
Signed-off-by: Michael Demoret <[email protected]>
/ok to test 69bd707 |
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.
Directly pushed changes
- Updated index.md to include links for Object Store and Object Store Provider. - Enhanced tool_test_runner.py with mock methods for managing Object Store functionality. Signed-off-by: Michael Demoret <[email protected]>
Signed-off-by: Michael Demoret <[email protected]>
/ok to test e8d2317 |
…bject-store Signed-off-by: Michael Demoret <[email protected]>
Signed-off-by: Michael Demoret <[email protected]>
Signed-off-by: Michael Demoret <[email protected]>
/ok to test c9b261d |
Signed-off-by: Michael Demoret <[email protected]>
/ok to test b775fff |
…bject-store Signed-off-by: Michael Demoret <[email protected]>
Signed-off-by: Michael Demoret <[email protected]>
/ok to test a9704cd |
/merge |
Description
This PR addresses https://jirasw.nvidia.com/browse/AIQ-846
By Submitting this PR I confirm: