feat: Enable remote chunking via unstructured-ingest #2905

Coniferish · 2024-04-18T22:03:03Z

Update: The cli shell script works when sending documents to the free api, but the paid api is down, so waiting to test against it.

The first commit adds docstrings and fixes type hints.
The second commit reorganizes test_unstructured_ingest so it matches the structure of unstructured/ingest.
The third commit contains the primary changes for this PR.
- The .chunk() method responsible for sending elements to the correct method is moved from ChunkingConfig to Chunker so that ChunkingConfig acts as a config object instead of containing implementation logic. Chunker.chunk() also now takes a json file instead of a list of elements. This is done to avoid redundant serialization if the file is to be sent to the api for chunking.

scanny

Okay, quite a few remarks.

I like what you did with the typing and docstrings but I wonder if you could pull that into a separate "prep" PR so the meaty changes are more focused.

I want to take another look in the morning but here are these initial comments in the meantime.

Btw, generally tests should appear in the same commit as the changes that make them pass. We should probably talk about this a little bit. It has to do with the test-driven approach, like first you write the test and then write the code to pass the test (and only the code it takes to pass the test). Let's chat about that next time we get together.

test_unstructured_ingest/unit/pipeline/reformat/test_chunking.py

scanny · 2024-04-19T04:11:52Z

test_unstructured_ingest/unit/pipeline/reformat/test_chunking.py

+            # -- "/.cache" of a user's profile.
+            # -- Define `work_dir` add the "/chunked" subdirectory to it:
+            chunker.pipeline_context.work_dir = tmpdir
+            os.makedirs(os.path.join(tmpdir, "chunked"), exist_ok=True)


Do you have to create the chunked directory? Doesn't ingest do that for itself?

No, without creating it FileNotFoundError: [Errno 2] No such file or directory is raised

test_unstructured_ingest/unit/pipeline/reformat/test_chunking.py

scanny · 2024-04-19T04:15:21Z

test_unstructured_ingest/unit/pipeline/reformat/test_chunking.py

+            os.makedirs(os.path.join(tmpdir, "chunked"), exist_ok=True)
+
+            filename = chunker.run(ELEMENTS_JSON_FILE)
+            head, tail = os.path.split(filename if filename else "")


Under what circumstances would filename be None? Seems like worth a separate assert, like:

assert filename is not None, "message explaining what this means and why it's a failure"

None is returned by Chunker.run() if that is somehow called without defining chunking_strategy. A message is logged, so I'll add a test for that.

test_unstructured_ingest/unit/pipeline/reformat/test_chunking.py

test_unstructured_ingest/unit/test_interfaces.py

unstructured/ingest/pipeline/reformat/chunking.py

unstructured/ingest/cli/cli.py

scanny

Ok, I think this is almost there. A few questions too.

Let me know when it's ready and I'll take a quick look and approve.

CHANGELOG.md

test_unstructured_ingest/unit/pipeline/reformat/test_chunking.py

+            assert head.endswith("chunked")
+            assert tail.endswith(".json")
+
+    def it_logs_error_with_invalid_remote_chunking_args(


scanny · 2024-04-24T04:18:55Z

test_unstructured_ingest/unit/pipeline/reformat/test_chunking.py

+
+        chunker.run(ELEMENTS_JSON_FILE)
+
+        assert "Input should be 'basic', 'by_page', 'by_similarity'" in caplog.text


Why is this the output and where does it come from? Only the remote chunker can tell what chunking strategies it has registered. Seems like 'by_title' should be in the list too.

And Input should be ... isn't very helpful as an error message goes. Is there more that was cut off? I'd be looking to see something saying "chunking_strategy" to give a hint where the problem is. No reason this string can't be as long as it needs to be to clearly represent the log entry.

unstructured_client.models.errors.httpvalidationerror.HTTPValidationError raises the error and by_title is in the list. I was trying to keep it to one line while still conveying what the logged message entailed.

The full output:

'ERROR unstructured.ingest:chunking.py:83 failed to run chunking on file /Users/johnjennings/src/unstructured/example-docs/test_evaluate_files/unstructured_output/Bank Good Credit Loan.pptx.json, {"detail":[{"loc":["body","chunking_strategy"],"msg":"Input should be \'basic\', \'by_page\', \'by_similarity\' or \'by_title\'","type":"literal_error"}]} Traceback (most recent call last): File "/Users/johnjennings/src/unstructured/unstructured/ingest/pipeline/reformat/chunking.py", line 64, in run chunked_elements = self.chunk(elements_json) File "/Users/johnjennings/src/unstructured/unstructured/ingest/pipeline/reformat/chunking.py", line 109, in chunk return partition_via_api( File "/Users/johnjennings/src/unstructured/unstructured/partition/api.py", line 103, in partition_via_api\n response = sdk.general.partition(req) File "/Users/johnjennings/.pyenv/versions/unstructured/lib/python3.10/site-packages/unstructured_client/utils/_human_utils.py", line 86, in wrapper return func(*args, **kwargs) File "/Users/johnjennings/.pyenv/versions/unstructured/lib/python3.10/site-packages/unstructured_client/utils/_human_split_pdf.py", line 40, in wrapper return func(*args, **kwargs) File "/Users/johnjennings/.pyenv/versions/unstructured/lib/python3.10/site-packages/unstructured_client/general.py", line 90, in partition raise out unstructured_client.models.errors.httpvalidationerror.HTTPValidationError: {"detail":[{"loc":["body","chunking_strategy"],"msg":"Input should be \'basic\', \'by_page\', \'by_similarity\' or \'by_title\'","type":"literal_error"}]}'

unstructured/ingest/pipeline/reformat/chunking.py

scanny · 2024-04-24T04:29:27Z

unstructured/ingest/pipeline/reformat/chunking.py

+                # -- These are not currently supported by the sdk and raise an error.
+                # -- They will need to be updated once we bump the client version.
+                # combine_text_under_n_chars=self.chunking_config.combine_text_under_n_chars,
+                # include_orig_elements=self.chunking_config.include_orig_elements,


What does being support by unstructured-client have to do with calling partition_via_api() via the unstructured library? unstructured-client doesn't even need to be installed, does it?

unstructured_via_api() uses unstructured-client to call the api.

I opened this as a follow-up issue:
#2924

scanny · 2024-04-24T04:30:51Z

unstructured/ingest/pipeline/reformat/chunking.py

+                # -- pinned to <=0.18.0
+                # overlap=self.chunking_config.overlap,
+                # overlap_all=self.chunking_config.overlap_all,
+            )


What is the behavior when there is no chunking strategy registered with the requested name on the remote host?

Like how does the user get a message like: No such chunking strategy: 'foobar'?

This is when the SDK raises an error. Example test: it_logs_error_on_invalid_remote_chunking_strategy()

scanny

LGTM :)

…tured-client

Update: The cli shell script works when sending documents to the free api, but the paid api is down, so waiting to test against it. - The first commit adds docstrings and fixes type hints. - The second commit reorganizes `test_unstructured_ingest` so it matches the structure of `unstructured/ingest`. - The third commit contains the primary changes for this PR. - The `.chunk()` method responsible for sending elements to the correct method is moved from `ChunkingConfig` to `Chunker` so that `ChunkingConfig` acts as a config object instead of containing implementation logic. `Chunker.chunk()` also now takes a json file instead of a list of elements. This is done to avoid redundant serialization if the file is to be sent to the api for chunking. --------- Co-authored-by: Ahmet Melek <39141206+ahmetmeleq@users.noreply.github.com>

Coniferish requested review from scanny and rbiseck3 April 18, 2024 22:04

Coniferish force-pushed the jj/feature-gapped-chunking-cli branch from 61f25db to e012272 Compare April 19, 2024 02:08

scanny reviewed Apr 19, 2024

View reviewed changes

Coniferish force-pushed the jj/feature-gapped-chunking-cli branch 2 times, most recently from 162a919 to e0da9c8 Compare April 19, 2024 14:36

Coniferish had a problem deploying to ci April 19, 2024 17:29 — with GitHub Actions Failure

Coniferish had a problem deploying to ci April 19, 2024 17:29 — with GitHub Actions Error

Coniferish temporarily deployed to ci April 19, 2024 18:02 — with GitHub Actions Inactive

Coniferish temporarily deployed to ci April 19, 2024 20:26 — with GitHub Actions Inactive

Coniferish force-pushed the jj/feature-gapped-chunking-cli branch from fb599cb to 9453fd8 Compare April 19, 2024 20:29

Coniferish temporarily deployed to ci April 19, 2024 20:31 — with GitHub Actions Inactive

Coniferish force-pushed the jj/feature-gapped-chunking-cli branch from 9453fd8 to 766f180 Compare April 19, 2024 20:34

Coniferish temporarily deployed to ci April 19, 2024 20:44 — with GitHub Actions Inactive

Coniferish temporarily deployed to ci April 21, 2024 01:43 — with GitHub Actions Inactive

Coniferish temporarily deployed to ci April 23, 2024 03:29 — with GitHub Actions Inactive

Coniferish requested a review from scanny April 23, 2024 14:49

Coniferish marked this pull request as ready for review April 23, 2024 15:02

Coniferish temporarily deployed to ci April 23, 2024 15:15 — with GitHub Actions Inactive

scanny requested changes Apr 24, 2024

View reviewed changes

scanny approved these changes Apr 24, 2024

View reviewed changes

ahmetmeleq and others added 2 commits April 24, 2024 17:30

add docstrings and comments, and fix types

c280ba8

organize test files

27553d1

Coniferish added 7 commits April 24, 2024 17:32

move implementation logic out of ChunkingConfig and add tests

1cacd4e

address review comments

2755b13

changelog and version

86e07e1

add default to fix failed test

34306e1

drop newer chunking params not supported by pinned version of unstruc…

1f1110f

…tured-client

update command line test with chunking and fix bugs

ce87f75

address comments v2

d9bbfc2

Coniferish force-pushed the jj/feature-gapped-chunking-cli branch from aeb7106 to d9bbfc2 Compare April 24, 2024 22:34

Coniferish temporarily deployed to ci April 24, 2024 22:43 — with GitHub Actions Inactive

Coniferish had a problem deploying to ci April 24, 2024 22:43 — with GitHub Actions Failure

Coniferish temporarily deployed to ci April 24, 2024 23:16 — with GitHub Actions Inactive

scanny added this pull request to the merge queue Apr 25, 2024

Merged via the queue into main with commit 3843af6 Apr 25, 2024
42 checks passed

scanny deleted the jj/feature-gapped-chunking-cli branch April 25, 2024 00:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Enable remote chunking via unstructured-ingest #2905

feat: Enable remote chunking via unstructured-ingest #2905

Coniferish commented Apr 18, 2024 •

edited

scanny left a comment

scanny Apr 19, 2024

Coniferish Apr 19, 2024

scanny Apr 19, 2024

Coniferish Apr 19, 2024 •

edited

scanny left a comment

This comment was marked as resolved.

scanny Apr 24, 2024

Coniferish Apr 24, 2024

scanny Apr 24, 2024

Coniferish Apr 24, 2024

Coniferish Apr 24, 2024

scanny Apr 24, 2024

Coniferish Apr 24, 2024

scanny left a comment


		chunker.run(ELEMENTS_JSON_FILE)

		assert "Input should be 'basic', 'by_page', 'by_similarity'" in caplog.text

feat: Enable remote chunking via unstructured-ingest #2905

feat: Enable remote chunking via unstructured-ingest #2905

Conversation

Coniferish commented Apr 18, 2024 • edited

scanny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Coniferish Apr 19, 2024 • edited

Choose a reason for hiding this comment

scanny left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scanny left a comment

Choose a reason for hiding this comment

Coniferish commented Apr 18, 2024 •

edited

Coniferish Apr 19, 2024 •

edited