feat: allow upload of text file in memory#1042
feat: allow upload of text file in memory#1042Skander-encord wants to merge 4 commits intomasterfrom
Conversation
Summary of ChangesHello @Skander-encord, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a significant enhancement to the text upload functionality, allowing users to upload text content directly from memory. This change provides greater flexibility for programmatic text generation and reduces the need for temporary file creation, streamlining the data ingestion process. The underlying upload mechanisms have been generalized to accommodate various data sources, improving the overall robustness and usability of the text upload feature. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Unit test report (Python 3.9.24, Pydantic 1.10.22)309 tests 309 ✅ 11s ⏱️ Results for commit 9e54104. |
Unit test report (Python 3.9.24, Pydantic 2.12.3)309 tests 309 ✅ 11s ⏱️ Results for commit 9e54104. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request successfully introduces the capability to upload text content directly from memory (strings, bytes, or file-like objects) to the upload_text method. This enhancement improves flexibility by allowing users to provide text data without needing to save it to a file first. The changes involve updating internal helper functions like _upload_single_file and _upload_local_file to handle various data types, and modifying _guess_title to generate appropriate titles for in-memory content. Overall, the changes are well-implemented, but there are a couple of areas where the handling of in-memory text content could be improved for better user experience and consistency.
| assert text_contents | ||
| if isinstance(text_contents, BufferedReader): | ||
| return str(text_contents.peek(10)) # Ensure we **peek** as BufferedReader is stateful | ||
| return str(text_contents[:10]) |
There was a problem hiding this comment.
The current implementation of _guess_title for text_contents that are bytes or BufferedReader will result in titles prefixed with b' (e.g., b'Hello worl'). This is not ideal for user-facing titles. It would be better to decode these bytes into a human-readable string.
if isinstance(text_contents, BufferedReader):
# Peek and decode for a human-readable title
return text_contents.peek(10).decode("utf-8", errors="ignore")
elif isinstance(text_contents, bytes):
# Decode bytes for a human-readable title
return text_contents[:10].decode("utf-8", errors="ignore")
# If it's a string, just take the first 10 characters
return str(text_contents[:10])| backoff_factor=backoff_factor, | ||
| ) | ||
| else: | ||
| content_type = "text/plain" if item_type == StorageItemType.PLAIN_TEXT else "application/octet-stream" |
There was a problem hiding this comment.
When PLAIN_TEXT items are uploaded from memory (i.e., data is not a Path), the content_type is hardcoded to "text/plain". This is less specific than the logic used for file-based PLAIN_TEXT uploads, which attempts to guess more specific MIME types like application/json or application/xml using mimetypes.guess_type. This inconsistency might lead to less accurate content type metadata for in-memory text uploads. Consider if a more robust content-based MIME type detection is needed here, or if the user should be able to explicitly provide a content_type for in-memory uploads.
rad-cord
left a comment
rad-cord
left a comment
There was a problem hiding this comment.
Overall looks good but needs a test with every single type of data
3 participants
Allow passing string and/or bytes to the upload_text method