fix(pipelines): read code blobs from raw URLs and fail the crawl on unreadable paths - #246
Open
Neilblaze wants to merge 1 commit into
Open
fix(pipelines): read code blobs from raw URLs and fail the crawl on unreadable paths#246Neilblaze wants to merge 1 commit into
Neilblaze wants to merge 1 commit into
Conversation
…nreadable paths Signed-off-by: neilblaze <putubanerjee23@gmail.com>
google-oss-prow
Bot
requested review from
chasecadet and
franciscojavierarceo
September 8, 2026 04:33
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #245
Why
code-pipeline.pyfeedscode_rag, the collection behindsearch_kubeflow_code. Three things make it index far less than it reports, none of which fail the run.Three of four default directories 404.
kubeflow/manifestsmovedapps/toapplications/, soapps/pipeline/upstream,apps/katibandapps/jupyterare gone.api_requestreturnsNoneon a non-200,get_files_recursiveturns that into[], and the component printsFound 0 filesand exits 0. Onlycommon/istiostill resolves: 84 files against 570 for the corrected paths. The same path swallows 5xx and exhausted rate limits.Files over 1 MiB are stored empty. The Contents API only base64-encodes blobs under 1 MiB; above that it answers 200 with
"encoding": "none"and an emptycontent. The"content" in file_respguard passes,b64decode("")yieldsb"", and the file is dropped downstream as "tiny". Five YAML files in the repo are affected, 22.1 MB, all KServe/Spark/Ray install manifests.Embedding responses are not length-checked, unlike
utils.embed_texts. A short batch leaves records with noembedding, which fails one component later as a bareKeyError.What
download_url. Directory listings already carrydownload_urlandhtml_urlfor every entry, including ones over the cap, and the listing'shtml_urlis identical to the per-file one, so citations are unchanged. This drops the per-file Contents API call: for the default directories the REST requests go from 780 to 210. The 570 blob reads move toraw.githubusercontent.com, which costs no quota at all (measured:core.usedis unchanged across raw fetches). Note the old repo name redirects, and GitHub bills the redirect too, so those 210 requests still cost 420 of the 5,000/hour budget rather than 210 -- down from 1,560.request_with_retriesstill waits out rate limits and now retries 5xx (which the old code never did), but raises once the retries are spent instead of returningNone, so a directory or file it cannot read ends the run. Rate-limit detection now covers 429 andRetry-After, not just 403 with a zeroedX-RateLimit-Remaining— GitHub uses both, and a secondary limit only setsRetry-After. A directory that exists but matches no extensions prints a warning rather than passing unremarked.applications/pipeline,applications/katib,applications/notebooks-v1,common/istio.applications/notebooks-v1is the direct rename target ofapps/jupyter;applications/workspacesis Notebooks 2.0, a separate tree, and is left out. Dropping the/upstreamsuffix on the pipeline path adds three overlay files (267 vs 264).utils.embed_textsalready applies.tests/eval/updated to match.reposstill sayskubeflow/manifests, which redirects tokubeflow/community-distribution. Switching to the canonical name would halve the quota cost, but it would also changefile_unique_idon every record and orphan the existing rows, sincestore_code_milvusdeletes by that key. That trade is a separate call from this fix, so the name is left alone.Verification
Against
mainall four new tests fail, each on the symptom described above:Directory and size facts checked live against
kubeflow/manifests@master; the 570-vs-84 file counts come from the Git tree API with the pipeline's own extension filter.pytest: 147 passed with the KFP SDK installed, against 143 onmainpython code-pipeline.pycompiles the pipeline;compileallcleanruff check/ruff format: no new findings. Both already fail onmain(issue Bug: PR Safety workflow failing on main — ruff format check fails for tests/test_docs_pipeline.py #236, PR fix(ci): satisfy ruff lint and format checks on main #241)Notes for reviewers
Where the new tests run. All four need the KFP SDK.
PR Safetyinstalls onlyrequirements-test.txt, so it skips them (137 passed / 9 skipped, against 137 / 6 onmain).Build, Test, and Deploy to OKEinstallskfp kfp-kubernetesand runs on every pull request, so they do execute there.Size the pod before crawling KServe. These manifests were previously dropped, so reading them is a real increase:
kserve.yamlalone becomes 7,872 chunks and 984 TEI batches at the defaultchunk_size=1000/embedding_batch_size=8. No component inpipelines/sets a memory limit.Behaviour change worth naming. The per-file Contents API call used to resolve symlinks; reading the raw URL stores the link target path instead.
kubeflow/manifestscontains no symlinks or submodules, so nothing in the default crawl is affected.Likely conflicts. No open PR edits
code-pipeline.pyin place, but #231 movesdocs-agent-mcp/tokagent-feast mcp/and rewritestests/test_code_utils.pywholesale, and #205/#206 add both files at the pre-movepipelines/path.Not addressed here
The same missing embedding check exists in
kubeflow-pipeline.py,issues-pipeline.pyandincremental-pipeline.py; #212 owns the issues pipeline and #244 the incremental one, butkubeflow-pipeline.pyis unowned and worth a follow-up.chunk_and_embed_codealso still hardcodesmax_tei_chars = 1000while the docs pipeline made it a parameter in #234 — the same kind of change a reviewer asked for there, and a reasonable follow-up once #212 settles the shared value.