Skip to content

refactor(core): deduplicate SimpleDirectoryReader load_file/aload_file - #22371

Open
gaurav0107 wants to merge 1 commit into
run-llama:mainfrom
gaurav0107:refactor/21428-dedup-simpledirectoryreader-load-file
Open

gaurav0107 wants to merge 1 commit into
run-llama:mainfrom
gaurav0107:refactor/21428-dedup-simpledirectoryreader-load-file

Conversation

@gaurav0107

Copy link
Copy Markdown
Contributor

Summary

SimpleDirectoryReader.load_file (sync) and SimpleDirectoryReader.aload_file
(async) in llama-index-core/llama_index/core/readers/file/base.py duplicated
the same ~55-line branching flow — metadata computation, file-reader
resolution/instantiation, error handling, filename_as_id doc-id assignment,
and the plain-text fallback read. Both carried a # TODO: make this less redundant marker.

This PR extracts the shared logic into four private static helpers and
rewrites the two public methods as thin wrappers:

  • _resolve_file_reader — pick (and cache) the BaseReader for a suffix, or
    None when the file should be read as plain text
  • _reader_load_kwargs — build the extra_info/fs kwargs for the reader
  • _build_reader_documents — apply filename_as_id ids to reader output
  • _read_file_as_document — the plain-text fallback read

The helpers are kept static so load_file stays picklable for the
multiprocessing num_workers path (load_data uses
partial(SimpleDirectoryReader.load_file, ...) + pool.imap).

Behavior is preserved, including the deliberate divergence between the two
entry points that the issue calls out: load_file wraps reader failures as
Exception("Error loading file") from e, while aload_file re-raises the
original exception. No public signature or default behavior changes.

Fixes #21428

Type of Change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

(Refactor / code-quality cleanup — non-breaking, behavior-preserving.)

How Has This Been Tested?

  • I added new unit tests to cover this change
  • I believe this change is already covered by existing unit tests

Ran pytest tests/readers/file/test_base.py in llama-index-core — 24/24 pass,
including the existing behavior-preservation tests (load_file,
load_file_error — which asserts the sync Exception("Error loading file")
wrap, load_file_unknown plain-text path, load_data, aload_data,
iter_data) and a new test_SimpleDirectoryReader_aload_file_error that
locks in the async path's divergent behavior (re-raises the original
exception, skips on raise_on_error=False, always re-raises ImportError).
Also verified the multiprocessing load_data(num_workers=2) and
filename_as_id paths, and ran ruff check, ruff format --check, and mypy
on the changed file — all clean.

Suggested Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added Google Colab support for the newly added notebooks.
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I ran uv run make format; uv run make lint to appease the lint gods

Version Bump?

  • Yes
  • No

llama-index-core is exempt from the per-package version-bump rule.

New Package?

  • Yes
  • No

load_file and aload_file duplicated the same branching flow (metadata
computation, reader resolution/instantiation, error handling,
filename_as_id id assignment, and the plain-text fallback read), each
marked "# TODO: make this less redundant".

Extract the shared logic into four private static helpers
(_resolve_file_reader, _reader_load_kwargs, _build_reader_documents,
_read_file_as_document) and rewrite both public methods as thin
wrappers. The helpers stay static so load_file remains picklable for
the multiprocessing num_workers path.

Behavior is preserved, including the deliberate divergence between the
two entry points: load_file wraps reader failures as
Exception("Error loading file") from e, while aload_file re-raises the
original exception. Add an async error-handling regression test that
locks in this divergence (previously only the sync path was covered).
@gaurav0107
gaurav0107 marked this pull request as ready for review July 15, 2026 22:31
@dosubot dosubot Bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Jul 15, 2026
@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

This PR is stale because it has been open 50 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions github-actions Bot added the stale Issue has not had recent activity or appears to be solved. Stale issues will be automatically closed label Sep 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:L This PR changes 100-499 lines, ignoring generated files. stale Issue has not had recent activity or appears to be solved. Stale issues will be automatically closed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Refactor]: Deduplicate SimpleDirectoryReader.load_file and aload_file

1 participant