Repository navigation
Conversation
|
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.
All reported issues were addressed across 8 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 1 file (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.
0 issues found across 2 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
Values without a string longer than the fragment size are emitted with one json.dumps call, so typical original elements no longer pay the per-node generator cost. Large strings are still escaped in bounded fragments.
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
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="unstructured/staging/base.py">
<violation number="1" location="unstructured/staging/base.py:284">
P2: `_has_large_string` ignores dictionary keys, so a large key in valid metadata such as `data_source.record_locator` bypasses bounded fragmentation and allocates the entire JSON value in one fragment. Include keys in this traversal.</violation>
</file>
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
There was a problem hiding this comment.
0 issues found across 2 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
|
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.
1 issue found across 8 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="scripts/performance/benchmark_chunking_memory.py">
<violation number="1" location="scripts/performance/benchmark_chunking_memory.py:26">
P3: The `--case originals` benchmark never exercises the >1 MiB temp-disk spill path it is meant to validate. `image_base64` is `x` repeated, and that payload dominates the stream; runs of an identical byte compress to near nothing with `zlib.compressobj`, so the compressed output written to `SpooledTemporaryFile(max_size=1 MiB)` stays far below 1 MiB (128 images' metadata compresses to only tens of KB), and the spooled buffer never rolls over to disk. The peak-RSS numbers and PR claim about the disk-spill optimization are therefore untested for the large-payload case that motivated them. Use an incompressible payload (e.g. base64 of random bytes) so the compressed buffer actually exceeds the 1 MiB roolover threshold.</violation>
</file>
Shadow auto-approve: would not auto-approve because issues were found.
Re-trigger cubic
| metadata=ElementMetadata( | ||
| image_base64="x" * (args.payload_mib * 1024 * 1024), |
There was a problem hiding this comment.
P3: The --case originals benchmark never exercises the >1 MiB temp-disk spill path it is meant to validate. image_base64 is x repeated, and that payload dominates the stream; runs of an identical byte compress to near nothing with zlib.compressobj, so the compressed output written to SpooledTemporaryFile(max_size=1 MiB) stays far below 1 MiB (128 images' metadata compresses to only tens of KB), and the spooled buffer never rolls over to disk. The peak-RSS numbers and PR claim about the disk-spill optimization are therefore untested for the large-payload case that motivated them. Use an incompressible payload (e.g. base64 of random bytes) so the compressed buffer actually exceeds the 1 MiB roolover threshold.
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 scripts/performance/benchmark_chunking_memory.py, line 26:
<comment>The `--case originals` benchmark never exercises the >1 MiB temp-disk spill path it is meant to validate. `image_base64` is `x` repeated, and that payload dominates the stream; runs of an identical byte compress to near nothing with `zlib.compressobj`, so the compressed output written to `SpooledTemporaryFile(max_size=1 MiB)` stays far below 1 MiB (128 images' metadata compresses to only tens of KB), and the spooled buffer never rolls over to disk. The peak-RSS numbers and PR claim about the disk-spill optimization are therefore untested for the large-payload case that motivated them. Use an incompressible payload (e.g. base64 of random bytes) so the compressed buffer actually exceeds the 1 MiB roolover threshold.</comment>
<file context>
@@ -0,0 +1,62 @@
+ yield Image(
+ text="",
+ element_id=f"image-{index}",
+ metadata=ElementMetadata(
+ image_base64="x" * (args.payload_mib * 1024 * 1024),
+ filename="streamed.pdf",
</file context>
| metadata=ElementMetadata( | |
| image_base64="x" * (args.payload_mib * 1024 * 1024), | |
| image_base64=base64.b64encode(os.urandom(args.payload_mib * 1024 * 1024)).decode(), |
Summary
A stream of empty images can hold every base64 payload until a later text element even when
include_orig_elements=Falseand those image fields are dropped from output. Fold metadata incrementally for contiguous empty-image runs and release excluded image payloads immediately, preserving metadata order and chunk boundaries.Original-element serialization now adjusts and encodes one element at a time, escapes large JSON strings in bounded fragments (values without large strings still use one standard
json.dumpscall), and spills compressed buffers above 1 MiB to temporary disk. Avoid the redundant deep copy of the whole original-element graph inElementMetadata.to_dict().Proposed library release: 0.27.14, together with the other pending library memory changes.
Validation
include_orig_elements=True.Operational impact
Compressed original-element buffers above 1 MiB use temporary disk. Final serialized output and metadata actually included in chunks still require memory proportional to output.
include_orig_elements=Truecontinues retaining original objects; changing that identity contract would require a separate API decision.