nw9663644-eng commented on PR #356: URL: https://github.com/apache/hugegraph-ai/pull/356#issuecomment-4632614640
> Thanks for the updates. The PDF upload implementation itself looks good to me: it adds `pypdf`-based extraction in the shared `read_documents()` path, handles mixed-case suffixes, returns clear Gradio errors for encrypted/unreadable/no-text PDFs, updates the upload copy, and adds coverage for TXT/DOCX/PDF plus both `build_vector_index()` and `extract_graph()` entrypoints.感谢您提供的更新。PDF 上传功能的实现本身看起来不错：它在共享的 `read_documents()` 路径中添加了基于 `pypdf` 提取功能，能够处理大小写混合的后缀，对于加密/不可读/无文本的 PDF 文件返回清晰的 Gradio 错误信息，更新了上传副本，并增加了对 TXT/DOCX/PDF 格式的支持，同时还添加了 `build_vector_index()` 和 `extract_graph()` 这两个入口点。 > > I have one blocking scope/dependency concern before merge:合并前我有一个阻塞性的范围/依赖关系问题： > > `hugegraph-llm/pyproject.toml` also changes `pycgraph` from `3.2.2` to `3.2.4`, including the aarch64 git source tag. This does not appear related to #345 or PDF upload support. `pycgraph` is the workflow engine used across many HugeGraph-LLM flows, so this broadens the PR blast radius beyond the document upload path. The current tests verify the PDF/document behavior, but they do not establish that the scheduler/flow dependency upgrade is safe across RAG, graph extraction, Text2Gremlin, import/update flows, and the aarch64 source path.`hugegraph-llm/pyproject.toml` 还将 `pycgraph` 从 `3.2.2` 更新到 `3.2.4` ，包括 aarch64 的 git 源代码标签。这似乎与 #345 或 PDF 上传支持无关。pycgraph 是许多 HugeGraph-LLM 流程中使用的工作流引擎，因此此次 `pycgraph` 扩大了 PR 的影响范围，使其超出文档上传路径。目前的测试验证了 PDF/文档的行为，但并未证明调度器/流程依赖项的升级在 RAG、图提取、Text2G remlin、导入/更新流程以及 aarch64 源代码路径中是安全的。 > > Please either revert the `pycgraph` changes from this PR, or explain why PDF support requires this upgrade and add the corresponding broader flow/integration verification. My preference is to keep this PR focused on PDF upload support and move any `pycgraph` upgrade to a separate PR.请要么撤销此 PR 中对 `pycgraph` 更改，要么解释为什么 PDF 支持需要此次升级，并添加相应的更全面的流程/集成验证。我倾向于让此 PR 专注于 PDF 上传支持，并将任何 `pycgraph` 升级移至单独的 PR 中。 > > Non-blocking improvement: the PDF tests are already sufficient for the main upload path, but they could be strengthened with a real encrypted PDF fixture, e.g. using `PdfWriter.encrypt()`, and a multi-page PDF test that asserts page-order-preserving extraction. This would make the issue’s page-by-page/stable-order acceptance criteria more explicit, but I do not think it needs to block this PR once the unrelated dependency bump is resolved.非阻塞性改进：目前针对主上传路径的 PDF 测试已经足够完善，但可以通过添加真正的加密 PDF 测试用例（例如使用 `PdfWriter.encrypt()` 以及一个多页 PDF 测试用例来增强其安全性，该测试用例能够确保提取过程保持页面顺序。这将使问题的逐页/稳定顺序验收标准更加明确，但我认为一旦无关的依赖项增加问题得到解决，它就不需要阻塞此 PR。 Thanks for the review. I reverted the unrelated `pycgraph` version/source-tag update from this PDF upload PR and kept the PDF-specific dependency changes only, including the root `pypdf~=6.12.0` constraint. Local checks passed: - `uv run ruff format --check .` - `uv run pytest hugegraph-llm/src/tests/document/test_vector_index_utils.py` -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
