feat(evaluation) 7/15: dataset upload, storage and deletion - #818
feat(evaluation) 7/15: dataset upload, storage and deletion#818Ahmath-Gadji wants to merge 1 commit into
Conversation
|
Warning Review limit reached
Next review available in: 59 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ac18d7c to
9c927e8
Compare
d8eec49 to
5ce2f07
Compare
|
@coderabbitai full review |
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 57 minutes. |
9c927e8 to
b7efc52
Compare
5ce2f07 to
4759352
Compare
b7efc52 to
f8a8257
Compare
4759352 to
64dc0d3
Compare
f8a8257 to
d04cd51
Compare
64dc0d3 to
c09367d
Compare
`EvaluationService` gains the dataset half: an admin uploads a corpus plus a test set, both land under `<data_dir>/eval/<dataset_id>/`, and a row records what is there. - The CSV is parsed at upload, so a malformed test set is rejected while the admin is still looking at the dialog rather than after a run has already spent minutes indexing the corpus. - Uploads arrive as open binary streams, not `bytes`. Each is copied to disk in fixed-size blocks under a running byte budget, so a 512 MB corpus is a disk cost rather than a RAM one. The blocking I/O runs on a worker thread so it cannot stall the event loop. - The cap is enforced by reading one byte past it, not by trusting a client-supplied `Content-Length`. - Duplicate corpus basenames are rejected rather than silently overwriting — that would leave `corpus_file_count` overstating what a run will actually index. Path components in a filename are flattened. - A partial write is cleaned up: any failure after the directory is created removes it before the error propagates. - Deleting a dataset an active run is using is refused. The runner reads the corpus off disk for the whole indexing phase, so removing it mid-run would surface as a FileNotFoundError instead of a clear conflict.
d04cd51 to
37d4787
Compare
c09367d to
54e7a03
Compare
hedhoud
left a comment
There was a problem hiding this comment.
I found three correctness issues that need handling before this is safe to use. The upload streaming, size limits, and cleanup path otherwise look good.
| if active is not None and active.dataset_id == dataset_id: | ||
| raise ConflictError(f"Evaluation run '{active.id}' is still using this dataset.") | ||
|
|
||
| if not await self._repo.delete_dataset(dataset_id): |
There was a problem hiding this comment.
[P1] Preserve run history when a dataset is deleted
The persistence layer currently cascades dataset deletion into eval_runs. This call therefore removes every completed run and its saved metrics, even though the product flow promises that past run results remain available. That is unrecoverable admin data loss, and the in-memory fake does not expose it. Please retain the historical run rows and cover this behavior with a repository-backed test.
| for the whole indexing phase, so removing it mid-run would surface as a | ||
| confusing FileNotFoundError instead of a clear conflict. | ||
| """ | ||
| active = await self._repo.active_run() |
There was a problem hiding this comment.
[P1] Make the active-run protection atomic
The active-run check and dataset deletion are separate database operations. A run can be created between them; the current cascade then removes its row and files while startup continues provisioning and dispatching it. The worker is left without its corpus or a run row to update. Please enforce the check and deletion atomically in the repository and add a test for this interleaving.
| budget = self._settings.max_corpus_bytes | ||
| for filename, stream in corpus: | ||
| # Flatten any path components a browser may have sent. | ||
| target = corpus_dir / Path(filename).name |
There was a problem hiding this comment.
[P2] Check duplicates after file ID normalization
The runner indexes files using their normalized file_id, but this check only compares the stored basenames. Distinct names such as report 1.txt and report?1.txt are both accepted here and later normalize to report_1.txt, so one document fails to index and source ground truth becomes ambiguous. Please reject collisions using the same normalization as the runner and add a regression test.
Part 7 of 15 of the split of #811. Targets
eval/06-persistence(#817).What
The dataset half of
EvaluationService:create_dataset,list_datasets,delete_dataset. Files land under<data_dir>/eval/<dataset_id>/(corpus/,testset.csv) and a row records what is there. Run dispatch is part 8.Notable
parse_testset(part 2) here, so a malformed test set is rejected while the admin is still looking at the upload dialog — not thirty minutes into an indexing phase.create_datasettakes open binary streams rather thanbytes, and copies each in 1 MB blocks under a running byte budget. A 512 MB corpus is therefore a disk cost, not a RAM one. The blocking I/O runs on a worker thread so it cannot stall the event loop.Content-Length, so an inflated header cannot get past it.test_an_oversized_test_set_is_rejected_without_buffering_it_allasserts the stream position, i.e. that the cap actually stopped the read.corpus_file_countoverstating what a run will actually index. Path components a browser may have sent are flattened.FileNotFoundErrorrather than a clear 409. Only an active run blocks — history keeps its results.Testing
3 unit tests against in-memory fakes: refused delete (and that the files survive it), allowed delete after an idle run, and the streaming size cap.
ruff, format check and the layer-import guard pass.