Repository navigation
Conversation
Remove the remaining SpooledTemporaryFile-to-BytesIO copy from detect_filetype(), keeping the spool rewound afterward. Ignore non-string file names when deriving the extension so a rolled-over spool falls back to metadata_file_path, and let convert_to_bytes() read any seekable binary stream. Release as 0.27.11 since 0.27.10 is already tagged.
There was a problem hiding this comment.
All reported issues were addressed across 11 files
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 2 files (changes from recent commits).
Shadow auto-approve: would not auto-approve. Auto-approval blocked by 1 unresolved issue from previous reviews.
Re-trigger cubic
There was a problem hiding this comment.
0 issues found across 3 files (changes from recent commits).
Shadow auto-approve: would not auto-approve. This PR does not meet the repository auto-approval settings.
Re-trigger cubic
There was a problem hiding this comment.
All reported issues were addressed across 7 files (changes from recent commits).
Shadow auto-approve: would not auto-approve because issues were found.
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
4 issues found across 14 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="test_unstructured/file_utils/test_filetype.py">
<violation number="1" location="test_unstructured/file_utils/test_filetype.py:517">
P2: These assertions make the new tests Unix-only: Windows rolled spools expose a string `.name`. Use a platform-neutral fixture/name assertion, or explicitly gate this Unix-specific check, so the supported Windows test suite does not fail.</violation>
</file>
<file name="CHANGELOG.md">
<violation number="1" location="CHANGELOG.md:5">
P3: `detect_filetype()` returns a `FileType`, not the input stream, so this says the wrong value is returned. Say the spooled file is left at position 0.</violation>
<violation number="2" location="CHANGELOG.md:7">
P3: This promises that every object with `read()` and `seek()` is rewound, but `BytesIO` and path-named `BufferedReader` inputs take separate branches that do not rewind the stream. Narrow this statement to the newly handled generic file-like streams.</violation>
</file>
<file name="unstructured/file_utils/filetype.py">
<violation number="1" location="unstructured/file_utils/filetype.py:515">
P2: Rolled spools on Windows expose a truthy temporary-path string without an extension, so this early return bypasses `metadata_file_path` and loses the supplied file type. Return the stream extension only when it is non-empty, then fall through to the metadata path.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
| with tempfile.SpooledTemporaryFile(max_size=1) as spooled_file: | ||
| spooled_file.write(b"# Heading\n\nSome *markdown* text.\n") | ||
| assert spooled_file._rolled | ||
| assert not isinstance(spooled_file.name, str) |
There was a problem hiding this comment.
P2: These assertions make the new tests Unix-only: Windows rolled spools expose a string .name. Use a platform-neutral fixture/name assertion, or explicitly gate this Unix-specific check, so the supported Windows test suite does not fail.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At test_unstructured/file_utils/test_filetype.py, line 517:
<comment>These assertions make the new tests Unix-only: Windows rolled spools expose a string `.name`. Use a platform-neutral fixture/name assertion, or explicitly gate this Unix-specific check, so the supported Windows test suite does not fail.</comment>
<file context>
@@ -489,6 +491,41 @@ def test_it_detects_EMPTY_from_empty_file_like_object():
+ with tempfile.SpooledTemporaryFile(max_size=1) as spooled_file:
+ spooled_file.write(b"# Heading\n\nSome *markdown* text.\n")
+ assert spooled_file._rolled
+ assert not isinstance(spooled_file.name, str)
+
+ assert detect_filetype(file=spooled_file, metadata_file_path="notes.md") == FileType.MD
</file context>
| return os.path.splitext(file.name)[1].lower() | ||
| # -- a temporary file (including a rolled-over `SpooledTemporaryFile`) can have an | ||
| # -- integer file-descriptor as its name, which carries no extension. | ||
| if isinstance(name := getattr(file, "name", None), (str, os.PathLike)) and name: |
There was a problem hiding this comment.
P2: Rolled spools on Windows expose a truthy temporary-path string without an extension, so this early return bypasses metadata_file_path and loses the supplied file type. Return the stream extension only when it is non-empty, then fall through to the metadata path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At unstructured/file_utils/filetype.py, line 515:
<comment>Rolled spools on Windows expose a truthy temporary-path string without an extension, so this early return bypasses `metadata_file_path` and loses the supplied file type. Return the stream extension only when it is non-empty, then fall through to the metadata path.</comment>
<file context>
@@ -511,8 +510,10 @@ def extension(self) -> str:
- return os.path.splitext(file.name)[1].lower()
+ # -- a temporary file (including a rolled-over `SpooledTemporaryFile`) can have an
+ # -- integer file-descriptor as its name, which carries no extension.
+ if isinstance(name := getattr(file, "name", None), (str, os.PathLike)) and name:
+ return os.path.splitext(name)[1].lower()
</file context>
| if isinstance(name := getattr(file, "name", None), (str, os.PathLike)) and name: | |
| if isinstance(name := getattr(file, "name", None), (str, os.PathLike)) and name: | |
| if extension := os.path.splitext(name)[1]: | |
| return extension.lower() |
|
|
||
| - **Avoid copying spooled uploads into memory**: `detect_filetype()` and the DOCX, PPTX, and shared partitioning paths now read `SpooledTemporaryFile` inputs in place instead of copying their complete contents into `BytesIO`. Large uploads remain disk-backed, avoiding a document-sized heap allocation without changing the partition API. A spooled file passed to `detect_filetype()` is still returned at read position 0. | ||
| - **Use `metadata_file_path` when a file's name is not a path**: a temporary file, including a rolled-over `SpooledTemporaryFile`, can report an integer file descriptor as its `.name`. File-type detection now ignores non-string names and falls back to `metadata_file_path` for the filename extension instead of raising `TypeError`. | ||
| - **Read any seekable binary stream in `convert_to_bytes()`**: streams such as `tempfile.TemporaryFile()` or file-like wrappers previously raised `ValueError("Invalid file-like object type")` from text-encoding detection and PDF page rendering. Any object with `read()` and `seek()` is now read from the start and rewound, including a `BufferedReader` whose `.name` is not a path (an in-memory buffer or a file descriptor), which previously raised `AttributeError` or closed the caller's descriptor. |
There was a problem hiding this comment.
P3: This promises that every object with read() and seek() is rewound, but BytesIO and path-named BufferedReader inputs take separate branches that do not rewind the stream. Narrow this statement to the newly handled generic file-like streams.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At CHANGELOG.md, line 7:
<comment>This promises that every object with `read()` and `seek()` is rewound, but `BytesIO` and path-named `BufferedReader` inputs take separate branches that do not rewind the stream. Narrow this statement to the newly handled generic file-like streams.</comment>
<file context>
@@ -1,3 +1,11 @@
+
+- **Avoid copying spooled uploads into memory**: `detect_filetype()` and the DOCX, PPTX, and shared partitioning paths now read `SpooledTemporaryFile` inputs in place instead of copying their complete contents into `BytesIO`. Large uploads remain disk-backed, avoiding a document-sized heap allocation without changing the partition API. A spooled file passed to `detect_filetype()` is still returned at read position 0.
+- **Use `metadata_file_path` when a file's name is not a path**: a temporary file, including a rolled-over `SpooledTemporaryFile`, can report an integer file descriptor as its `.name`. File-type detection now ignores non-string names and falls back to `metadata_file_path` for the filename extension instead of raising `TypeError`.
+- **Read any seekable binary stream in `convert_to_bytes()`**: streams such as `tempfile.TemporaryFile()` or file-like wrappers previously raised `ValueError("Invalid file-like object type")` from text-encoding detection and PDF page rendering. Any object with `read()` and `seek()` is now read from the start and rewound, including a `BufferedReader` whose `.name` is not a path (an in-memory buffer or a file descriptor), which previously raised `AttributeError` or closed the caller's descriptor.
+
## 0.27.11
</file context>
| - **Read any seekable binary stream in `convert_to_bytes()`**: streams such as `tempfile.TemporaryFile()` or file-like wrappers previously raised `ValueError("Invalid file-like object type")` from text-encoding detection and PDF page rendering. Any object with `read()` and `seek()` is now read from the start and rewound, including a `BufferedReader` whose `.name` is not a path (an in-memory buffer or a file descriptor), which previously raised `AttributeError` or closed the caller's descriptor. | |
| - **Read generic file-like streams in `convert_to_bytes()`**: streams such as `tempfile.TemporaryFile()` or file-like wrappers previously raised `ValueError("Invalid file-like object type")` from text-encoding detection and PDF page rendering. Previously unsupported file-like streams are now read from the start and rewound, including a `BufferedReader` whose `.name` is not a path (an in-memory buffer or a file descriptor), which previously raised `AttributeError` or closed the caller's descriptor. |
|
|
||
| ### Fixes | ||
|
|
||
| - **Avoid copying spooled uploads into memory**: `detect_filetype()` and the DOCX, PPTX, and shared partitioning paths now read `SpooledTemporaryFile` inputs in place instead of copying their complete contents into `BytesIO`. Large uploads remain disk-backed, avoiding a document-sized heap allocation without changing the partition API. A spooled file passed to `detect_filetype()` is still returned at read position 0. |
There was a problem hiding this comment.
P3: detect_filetype() returns a FileType, not the input stream, so this says the wrong value is returned. Say the spooled file is left at position 0.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At CHANGELOG.md, line 5:
<comment>`detect_filetype()` returns a `FileType`, not the input stream, so this says the wrong value is returned. Say the spooled file is left at position 0.</comment>
<file context>
@@ -1,3 +1,11 @@
+
+### Fixes
+
+- **Avoid copying spooled uploads into memory**: `detect_filetype()` and the DOCX, PPTX, and shared partitioning paths now read `SpooledTemporaryFile` inputs in place instead of copying their complete contents into `BytesIO`. Large uploads remain disk-backed, avoiding a document-sized heap allocation without changing the partition API. A spooled file passed to `detect_filetype()` is still returned at read position 0.
+- **Use `metadata_file_path` when a file's name is not a path**: a temporary file, including a rolled-over `SpooledTemporaryFile`, can report an integer file descriptor as its `.name`. File-type detection now ignores non-string names and falls back to `metadata_file_path` for the filename extension instead of raising `TypeError`.
+- **Read any seekable binary stream in `convert_to_bytes()`**: streams such as `tempfile.TemporaryFile()` or file-like wrappers previously raised `ValueError("Invalid file-like object type")` from text-encoding detection and PDF page rendering. Any object with `read()` and `seek()` is now read from the start and rewound, including a `BufferedReader` whose `.name` is not a path (an in-memory buffer or a file descriptor), which previously raised `AttributeError` or closed the caller's descriptor.
</file context>
| - **Avoid copying spooled uploads into memory**: `detect_filetype()` and the DOCX, PPTX, and shared partitioning paths now read `SpooledTemporaryFile` inputs in place instead of copying their complete contents into `BytesIO`. Large uploads remain disk-backed, avoiding a document-sized heap allocation without changing the partition API. A spooled file passed to `detect_filetype()` is still returned at read position 0. | |
| - **Avoid copying spooled uploads into memory**: `detect_filetype()` and the DOCX, PPTX, and shared partitioning paths now read `SpooledTemporaryFile` inputs in place instead of copying their complete contents into `BytesIO`. Large uploads remain disk-backed, avoiding a document-sized heap allocation without changing the partition API. A spooled file passed to `detect_filetype()` is left at read position 0. |
What
Partitioning and file-type detection read a
SpooledTemporaryFileupload in place. They no longer copy the whole document into aBytesIOfirst.Two related robustness fixes come with it, since removing the detector copy exposes both:
detect_filetype(file=<rolled-over spool>, metadata_file_path="a.md").nameis an int fd, so extension lookup raisesTypeError(previously hidden by the copy).mdtaken frommetadata_file_pathconvert_to_bytes(tempfile.TemporaryFile())or any file-like wrapperValueError: Invalid file-like object type(text-encoding detection, PDF page rendering)A spool passed to
detect_filetype()still comes back at position 0, as before.Why
Python 3.11+ (this package's minimum) gives
SpooledTemporaryFilethe full buffered-I/O interface thatzipfile,pandasand the detectors need. TheBytesIOcopies were a <3.11 workaround, and each one is an extra heap allocation the size of the document. Fixing it here removes the need for callers such as unstructured-api and core-product to wrap uploads in proxy objects to avoid the copy.Release
Bumps to 0.27.12. 0.27.11 is already released.
Impact
54.9 MB DOCX through the partition API: peak process RSS −51.2 MiB, peak container memory −52.6 MiB, median latency 9.343s → 9.197s.
Validation
convert_to_byteson generic streams. All fail without this change.file_utils,partition/common, DOCX, PPTX, text, XML, TSV, MD, HTML and auto suites: no new failures. The one existing test that read the detection context after closing the spool now asserts inside thewithblock.