Repository navigation
feat(extract,sink): add GitHub raw CDN downloader, modern dlt/polars/pydantic stack - #38
Conversation
…n pooling, and fix CLI docs
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configuration
⛔ Files ignored due to path filters: 1
📒 Files selected for processing: 28
✨ Finishing Touches
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| line = '20240328,0,Thu,LAN,NL,1,SDN,NL,1,5,2,54,D,,,,SEO01,15952,185,"000101003","000000020",37,11,1,0,0,5,0,0,0,1,0,3,1,0,0,0,8,6,2,2,0,0,27,11,1,0,0,0,31,4,0,0,0,2,0,0,1,0,0,7,2,0,0,0,9,5,5,5,1,0,27,9,2,0,0,0,hp_id,hp_name,1b_id,1b_name,2b_id,2b_name,3b_id,3b_name,lf_id,lf_name,rf_id,rf_name,v_mgr_id,v_mgr_name,h_mgr_id,h_mgr_name,w_pit_id,w_pit_name,l_pit_id,l_pit_name,s_pit_id,s_pit_name,gwrbi_id,gwrbi_name,v_sp_id,v_sp_name,h_sp_id,h_sp_name,v_b1_id,v_b1_name,8,v_b2_id,v_b2_name,6,v_b3_id,v_b3_name,7,v_b4_id,v_b4_name,3,v_b5_id,v_b5_name,9,v_b6_id,v_b6_name,4,v_b7_id,v_b7_name,5,v_b8_id,v_b8_name,2,v_b9_id,v_b9_name,1,h_b1_id,h_b1_name,8,h_b2_id,h_b2_name,6,h_b3_id,h_b3_name,7,h_b4_id,h_b4_name,3,h_b5_id,h_b5_name,9,h_b6_id,h_b6_name,4,h_b7_id,h_b7_name,5,h_b8_id,h_b8_name,2,h_b9_id,h_b9_name,1,additional_info,acquisition_info\n' | ||
|
|
||
| def mock_fetch(url: str) -> bytes: | ||
| assert "raw.githubusercontent.com" in url |
| ) | ||
|
|
||
| def mock_fetch(url: str) -> bytes: | ||
| if "raw.githubusercontent.com" in url: |
 [Medium impact] The changes reach the package’s download and data-loading workflows. Do not merge until credential disclosure, writes outside the cache, cache failures, and constructor compatibility are fixed. Findings
Fix with agent prompt### Issue 1
src/retrosheetpy/extract/downloader.py:51-56
If `GH_TOKEN` or `GITHUB_TOKEN` is set, `DownloaderConfig` reads it automatically and this code adds it to every request from the shared client. An uncached `transactions()` request always goes to `retrosheet.org`, even with the default source. Explicit Retrosheet requests and custom mirrors also receive the secret.
Remove the client-wide `Authorization` header and attach credentials only to trusted GitHub requests.
**How this was verified:** The environment token becomes a shared request header, and `get_transactions()` uses that client to request a Retrosheet URL without overriding the header.
### Issue 2
src/retrosheetpy/extract/downloader.py:170
If a malicious or compromised mirror supplies `/tmp/probe,N` in `TEAM2024`, returns 404 for the event request, then returns roster bytes, `roster_filename` becomes `/tmp/probe2024.ROS`. Joining this absolute path to `extracted_dir` discards the cache path, so the download can overwrite a writable file outside the cache.
Reject team codes containing path separators or absolute paths before fetching or writing files. Also check that each destination remains inside the season directory.
**How this was verified:** The downloaded manifest supplies `team_id` without a path check, and that value starts the filename passed directly to `write_bytes()`.
### Issue 3
src/retrosheetpy/extract/downloader.py:190-193
`get_events(force=True)` passes the existing season directory into `_get_events_github()`. Downloads overwrite files there, and a failed team request unconditionally removes the entire directory. With `source="github"`, the error then escapes with the previously good cache gone.
Download into a temporary directory and replace the cached directory only after the full download succeeds.
### Issue 4
src/retrosheetpy/extract/downloader.py:260-262
If the World Series file downloads and the next request times out with `source="github"`, `get_postseason()` raises but leaves the first file saved. The next call sees one `.EVE` file and returns immediately. `postseason()` then silently parses an incomplete season.
Stage these downloads and publish the directory only after all round requests finish.
### Issue 5
src/retrosheetpy/extract/downloader.py:31-33
Adding `config` before `timeout` breaks the existing positional constructor. A previously valid `Downloader(cache, manifest, 30.0)` now stores the float as `self.config` and fails when reading its attributes.
Keep `timeout` as the third argument and make the new options keyword-only.
### Issue 6
src/retrosheetpy/sink/dlt_sink.py:169-172
The new Arrow path calls `parse_files()` for the whole season before yielding anything. That function keeps every event as a Python dictionary, then builds Arrow arrays while those dictionaries still exist. This uses much more peak memory than the previous row-by-row output and delays the first batch until the season is parsed.
Build and yield bounded Arrow batches instead. The same change applies to postseason and All-Star resources.
### Issue 7
docs/index.md:18
The homepage promises automatic fallback by default, but the default is `source="github"`, which raises on a GitHub failure. Fallback only runs with `source="auto"`.
State that requirement here so users know how to enable it.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.SummaryAdds GitHub raw downloads, source selection across the API and CLI, pooled HTTP connections, and transaction warnings with local-file support.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if self.config.github_token: | ||
| headers["Authorization"] = f"Bearer {self.config.github_token}" | ||
| self._client = httpx.Client( | ||
| follow_redirects=True, | ||
| timeout=self.timeout, | ||
| headers=headers, |
There was a problem hiding this comment.
If GH_TOKEN or GITHUB_TOKEN is set, DownloaderConfig reads it automatically and this code adds it to every request from the shared client. An uncached transactions() request always goes to retrosheet.org, even with the default source. Explicit Retrosheet requests and custom mirrors also receive the secret.
Remove the client-wide Authorization header and attach credentials only to trusted GitHub requests.
How this was verified: The environment token becomes a shared request header, and get_transactions() uses that client to request a Retrosheet URL without overriding the header.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/retrosheetpy/extract/downloader.py
Line: 51-56
Comment:
**GitHub tokens leave GitHub**
If `GH_TOKEN` or `GITHUB_TOKEN` is set, `DownloaderConfig` reads it automatically and this code adds it to every request from the shared client. An uncached `transactions()` request always goes to `retrosheet.org`, even with the default source. Explicit Retrosheet requests and custom mirrors also receive the secret.
Remove the client-wide `Authorization` header and attach credentials only to trusted GitHub requests.
**How this was verified:** The environment token becomes a shared request header, and `get_transactions()` uses that client to request a Retrosheet URL without overriding the header.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| roster_bytes = self._fetch_bytes( | ||
| self.manifest.github_season_file_url(year, roster_filename) | ||
| ) | ||
| (extracted_dir / roster_filename).write_bytes(roster_bytes) |
There was a problem hiding this comment.
If a malicious or compromised mirror supplies /tmp/probe,N in TEAM2024, returns 404 for the event request, then returns roster bytes, roster_filename becomes /tmp/probe2024.ROS. Joining this absolute path to extracted_dir discards the cache path, so the download can overwrite a writable file outside the cache.
Reject team codes containing path separators or absolute paths before fetching or writing files. Also check that each destination remains inside the season directory.
How this was verified: The downloaded manifest supplies team_id without a path check, and that value starts the filename passed directly to write_bytes().
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/retrosheetpy/extract/downloader.py
Line: 170
Comment:
**Mirror files escape the cache**
If a malicious or compromised mirror supplies `/tmp/probe,N` in `TEAM2024`, returns 404 for the event request, then returns roster bytes, `roster_filename` becomes `/tmp/probe2024.ROS`. Joining this absolute path to `extracted_dir` discards the cache path, so the download can overwrite a writable file outside the cache.
Reject team codes containing path separators or absolute paths before fetching or writing files. Also check that each destination remains inside the season directory.
**How this was verified:** The downloaded manifest supplies `team_id` without a path check, and that value starts the filename passed directly to `write_bytes()`.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| except Exception: | ||
| # Clean up partial directory on failure to avoid cache poisoning | ||
| shutil.rmtree(extracted_dir, ignore_errors=True) | ||
| raise |
There was a problem hiding this comment.
get_events(force=True) passes the existing season directory into _get_events_github(). Downloads overwrite files there, and a failed team request unconditionally removes the entire directory. With source="github", the error then escapes with the previously good cache gone.
Download into a temporary directory and replace the cached directory only after the full download succeeds.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/retrosheetpy/extract/downloader.py
Line: 190-193
Comment:
**Failed refresh erases saved data**
`get_events(force=True)` passes the existing season directory into `_get_events_github()`. Downloads overwrite files there, and a failed team request unconditionally removes the entire directory. With `source="github"`, the error then escapes with the previously good cache gone.
Download into a temporary directory and replace the cached directory only after the full download succeeds.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| except Exception as e: | ||
| if self.source == "github": | ||
| raise |
There was a problem hiding this comment.
If the World Series file downloads and the next request times out with source="github", get_postseason() raises but leaves the first file saved. The next call sees one .EVE file and returns immediately. postseason() then silently parses an incomplete season.
Stage these downloads and publish the directory only after all round requests finish.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/retrosheetpy/extract/downloader.py
Line: 260-262
Comment:
**Postseason cache keeps incomplete downloads**
If the World Series file downloads and the next request times out with `source="github"`, `get_postseason()` raises but leaves the first file saved. The next call sees one `.EVE` file and returns immediately. `postseason()` then silently parses an incomplete season.
Stage these downloads and publish the directory only after all round requests finish.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| config: DownloaderConfig | None = None, | ||
| source: Literal["github", "retrosheet", "auto"] | None = None, | ||
| timeout: float | None = None, |
There was a problem hiding this comment.
Adding config before timeout breaks the existing positional constructor. A previously valid Downloader(cache, manifest, 30.0) now stores the float as self.config and fails when reading its attributes.
Keep timeout as the third argument and make the new options keyword-only.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/retrosheetpy/extract/downloader.py
Line: 31-33
Comment:
**Positional timeout calls now fail**
Adding `config` before `timeout` breaks the existing positional constructor. A previously valid `Downloader(cache, manifest, 30.0)` now stores the float as `self.config` and fails when reading its attributes.
Keep `timeout` as the third argument and make the new options keyword-only.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| if files: | ||
| tbl = parser.parse_files(files, year=y) | ||
| if tbl.num_rows > 0: | ||
| yield tbl |
There was a problem hiding this comment.
The new Arrow path calls parse_files() for the whole season before yielding anything. That function keeps every event as a Python dictionary, then builds Arrow arrays while those dictionaries still exist. This uses much more peak memory than the previous row-by-row output and delays the first batch until the season is parsed.
Build and yield bounded Arrow batches instead. The same change applies to postseason and All-Star resources.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/retrosheetpy/sink/dlt_sink.py
Line: 169-172
Comment:
**Event loads use more memory**
The new Arrow path calls `parse_files()` for the whole season before yielding anything. That function keeps every event as a Python dictionary, then builds Arrow arrays while those dictionaries still exist. This uses much more peak memory than the previous row-by-row output and delays the first batch until the season is parsed.
Build and yield bounded Arrow batches instead. The same change applies to postseason and All-Star resources.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| `retrosheetpy` brings modern data workflows to [Retrosheet](https://www.retrosheet.org) baseball archives. It enables baseball researchers, analysts, professors, and DBAs to download, parse, and ingest play-by-play events, historical game logs, rosters, and ballparks directly into **DuckDB, Parquet, Polars, pandas, and SQL databases** with **strongly-typed schemas** (integers, dates, booleans—never raw text dumps). | ||
|
|
||
| Powered by [`chadwickpy`](https://github.com/cbwinslow/chadwickpy) for multi-core play-by-play parsing with **zero C compiler dependencies**. | ||
| Downloads default to the fast, rate-limit-free GitHub Fastly raw CDN mirror (`chadwickbureau/retrosheet`) with automatic fallback to `retrosheet.org`. Powered by [`chadwickpy`](https://github.com/cbwinslow/chadwickpy) for multi-core play-by-play parsing with **zero C compiler dependencies**. |
There was a problem hiding this comment.
The homepage promises automatic fallback by default, but the default is source="github", which raises on a GitHub failure. Fallback only runs with source="auto".
State that requirement here so users know how to enable it.
Prompt To Fix With AI
This is a comment left during a code review.
Path: docs/index.md
Line: 18
Comment:
**Default fallback claim is wrong**
The homepage promises automatic fallback by default, but the default is `source="github"`, which raises on a GitHub failure. Fallback only runs with `source="auto"`.
State that requirement here so users know how to enable it.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Summary
etrosheet.org.