fix(cli): drop tqdm from file uploader, use rich progress (DX-458) - #523
fix(cli): drop tqdm from file uploader, use rich progress (DX-458)#523blainekasten wants to merge 12 commits into
Conversation
Broly Security ScanNote Summary 6 actionable finding(s) in this PR
5 highest-priority actionable rows in the table below (critical/high first, then top medium). 1 finding(s) below the No finding is at or above
Dismiss false positivesTick a box to dismiss the finding; untick it to bring the finding back. That is the same as replying
Note Re-scan this PR anytime with
|
|
@cursoragent i still see references to tqdm. Please remove all usage and apply proper code changes to keep progress visibility to CLI callers. |
|
Pushed. All real
PR: #523   |
@cursoragent please address this security issue as well |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| file_path, file_size = await AsyncDownloadManager(config.client).download( | ||
| url=url, | ||
| output=output, | ||
| remote_name=ft_job.x_model_output_name, | ||
| fetch_metadata=True, | ||
| progress_callback=progress_callback, | ||
| ) |
Make UploadManager/AsyncUploadManager presentation-free by reporting FileUploadProgress via callback instead of printing with tqdm. Wire rich Progress into `files upload` and `fine-tuning create`. Co-authored-by: Blaine Kasten <blainekasten@gmail.com>
Drop tqdm from download managers and check_file, report download progress via callbacks, render Rich progress in fine-tuning download, and remove the tqdm dependency. Co-authored-by: Blaine Kasten <blainekasten@gmail.com>
036ae55 to
ea6e6a0
Compare
|
@cursoragent instead of making a new upload progress, can we reuse the UploadProgressTracker from src/together/lib/cli/api/beta/models/upload.py? Also make sure tests/lint passes |
|
Done.
PR: #523 |
Move UploadProgressTracker/DownloadProgressTracker into shared CLI components and wire files/fine-tuning upload+download through them. Also validate upload redirect URLs/file IDs to mitigate SSRF. Co-authored-by: Blaine Kasten <blainekasten@gmail.com>
| callback_response = await self._client._client.put( | ||
| url=redirect_url, | ||
| content=_aiter_file_upload_chunks( | ||
| file, | ||
| total_bytes=file_size, | ||
| progress_callback=progress_callback, | ||
| ), | ||
| headers={"Content-Length": str(file_size)}, | ||
| ) |
There was a problem hiding this comment.
The PUT body went from bytes to a one-shot generator, but self._client._client is built with follow_redirects=True (_base_client.py:826).
If the presigned endpoint answers with a 307 — S3 does this for cross-region/newly-created buckets, and GCS resumable endpoints do it too — httpx will try to replay the request body and blow up with StreamConsumed instead of just re-PUTing like it used to when content was a plain bytes object.
Same for any transport-level retry that happens after the first byte is written. Worth passing follow_redirects=False here and handling the redirect ourselves, or at least re-creating the iterator per attempt.
| callback_response = self._client._client.put( | ||
| url=redirect_url, | ||
| content=_iter_file_upload_chunks( | ||
| file, | ||
| total_bytes=file_size, | ||
| progress_callback=progress_callback, | ||
| ), | ||
| headers={"Content-Length": str(file_size)}, | ||
| ) |
There was a problem hiding this comment.
The PUT body went from bytes to a one-shot generator, but self._client._client is built with follow_redirects=True (_base_client.py:826).
If the presigned endpoint answers with a 307 — S3 does this for cross-region/newly-created buckets, and GCS resumable endpoints do it too — httpx will try to replay the request body and blow up with StreamConsumed instead of just re-PUTing like it used to when content was a plain bytes object.
Same for any transport-level retry that happens after the first byte is written. Worth passing follow_redirects=False here and handling the redirect ourselves, or at least re-creating the iterator per attempt.
|
|
||
|
|
||
| def _validate_upload_file_id(file_id: str) -> str: | ||
| if not _SAFE_FILE_ID_RE.fullmatch(file_id): |
There was a problem hiding this comment.
_SAFE_FILE_ID_RE allows ., so a bare .. passes validation, and this value goes straight into self.callback(f"{url}/{file_id}/preprocess") at line 711. An X-Together-File-Id: .. header gets us to /files/../preprocess, which httpx normalizes to /preprocess — exactly the traversal the check was added to stop. The test only covers ../evil, which is caught by the /. Can we tighten this to something like ^file-[A-Za-z0-9-]+$, or at minimum reject any segment that is . or ..?
| _SAFE_FILE_ID_RE = re.compile(r"^[A-Za-z0-9._:-]+$") | ||
|
|
||
|
|
||
| def _validate_upload_redirect_url(redirect_url: str) -> str: |
There was a problem hiding this comment.
Two things on _validate_upload_redirect_url:
(a) ipaddress.ip_address() only recognizes dotted-quad, so https://2130706433/x or https://0x7f000001/x sails through and then resolves to 127.0.0.1 at connect time. Same for any DNS name pointing at a private range (127.0.0.1.nip.io). If we're calling this an SSRF mitigation it's worth a comment saying it's best-effort at the URL layer, or resolving the host before deciding.
(b) The hard https-only rule is a behavior change for anyone pointing TOGETHER_BASE_URL at a local/on-prem/staging gateway that returns an http:// Location. The sync get_upload_url has no mock-server escape hatch (the async one at :1035 does), so tg files upload against a local stack now fails outright. Could we allow http when the client's own base_url is http, or gate it behind an env opt-out?
| if _check_path_exists(training_args["training_file"]): | ||
| file_upload = await config.client.files.upload(Path(training_args["training_file"]), purpose="fine-tune") | ||
| training_path = Path(training_args["training_file"]) | ||
| file_upload = await upload_file_with_progress( |
There was a problem hiding this comment.
upload_file_with_progress starts the Rich progress bar and then awaits files.upload, which still runs check_file first because check defaults to True here. For a multi-GB training file the user stares at a live Uploading training file foo.jsonl bar pinned at 0% for the whole validation pass, which reads as a hang. Either run the check before entering the tracker, or give validation its own phase/description.
| if _check_path_exists(training_args["validation_file"]): | ||
| file_upload = await config.client.files.upload(Path(training_args["validation_file"]), purpose="fine-tune") | ||
| validation_path = Path(training_args["validation_file"]) | ||
| file_upload = await upload_file_with_progress( |
There was a problem hiding this comment.
upload_file_with_progress starts the Rich progress bar and then awaits files.upload, which still runs check_file first because check defaults to True here. For a multi-GB training file the user stares at a live Uploading training file foo.jsonl bar pinned at 0% for the whole validation pass, which reads as a hang. Either run the check before entering the tracker, or give validation its own phase/description.
| unit=" lines", | ||
| disable=bool(DISABLE_TQDM), | ||
| ): | ||
| for line_index, raw_line in enumerate(f): |
There was a problem hiding this comment.
Dropping tqdm here removed the only feedback validation ever had, and nothing replaced it. tg files check and tg files upload on a large jsonl now sit completely silent while we stream the whole file. Given the point of the PR is better progress reporting, this feels like the one place that got worse — a Rich spinner or a per-N-lines update would keep parity.
| unit=" lines", | ||
| disable=bool(DISABLE_TQDM), | ||
| ): | ||
| for idx, line in enumerate(f): |
There was a problem hiding this comment.
Dropping tqdm here removed the only feedback validation ever had, and nothing replaced it. tg files check and tg files upload on a large jsonl now sit completely silent while we stream the whole file. Given the point of the PR is better progress reporting, this feels like the one place that got worse — a Rich spinner or a per-N-lines update would keep parity.
| def _validate_upload_redirect_url(redirect_url: str, *, allow_http: bool = False) -> str: | ||
| """Best-effort URL-layer filter for upload PUT targets. | ||
|
|
||
| This is not a complete SSRF defense: we do not resolve DNS, so a name like | ||
| ``127.0.0.1.nip.io`` still passes. We do reject non-public dotted-quad / IPv6 | ||
| literals, IPv4-mapped loopback (``::ffff:127.0.0.1``), and dword decimal/hex | ||
| IPv4 (``2130706433``, ``0x7f000001``) that getaddrinfo will treat as 127.0.0.1. | ||
|
|
||
| HTTPS is required unless ``allow_http`` is set — either the client's | ||
| ``base_url`` is ``http`` (local/on-prem stacks) or | ||
| ``TOGETHER_ALLOW_INSECURE_UPLOAD_REDIRECTS`` is enabled. When ``allow_http`` | ||
| is set, localhost and non-public literals are also allowed so MinIO / | ||
| LocalStack endpoints like ``http://127.0.0.1:9000/...`` work. | ||
| """ | ||
|
|
||
| parsed = urlparse(redirect_url) | ||
| if parsed.scheme == "https" or (parsed.scheme == "http" and allow_http): | ||
| pass | ||
| else: | ||
| raise ValueError(f"Refusing non-HTTPS upload redirect URL: {redirect_url!r}") | ||
|
|
||
| hostname = parsed.hostname | ||
| if not hostname: | ||
| raise ValueError(f"Upload redirect URL is missing a hostname: {redirect_url!r}") | ||
|
|
||
| lowered = hostname.lower() | ||
| if lowered == "metadata.google.internal": | ||
| raise ValueError(f"Refusing upload redirect to local host: {redirect_url!r}") | ||
|
|
||
| if not allow_http: | ||
| if lowered == "localhost" or lowered.endswith(".localhost"): | ||
| raise ValueError(f"Refusing upload redirect to local host: {redirect_url!r}") | ||
|
|
||
| ip = _parse_hostname_ip(hostname) | ||
| if ip is not None and _ip_is_non_public(ip): | ||
| raise ValueError(f"Refusing upload redirect to non-public address: {redirect_url!r}") | ||
|
|
||
| return redirect_url |
| return os.environ.get(name, "").strip().lower() in {"1", "true", "yes"} | ||
|
|
||
|
|
||
| def _allow_http_upload_redirects(client: Any) -> bool: |
4 participants
Summary
Resolves DX-458: make file transfer logic presentation-free and render progress with Rich in the CLI.
Changes
tqdmusage from upload/download managers andcheck_filetqdm/types-tqdmdependenciesFileUploadProgress/FileDownloadProgress); SDK stays quiet by defaultUploadProgressTracker/DownloadProgressTrackerCLI components (extracted from beta models upload/download) for:tg files uploadtg fine-tuning create(local training/validation uploads)tg fine-tuning downloadtg beta models upload/ download flowshttpsonly, reject localhost/private IPs) and file IDs to mitigate SSRFTOGETHER_DISABLE_TQDMCLI env toggles (no longer needed)DISABLE_TQDM = Falseexport for compatibilityTests
./scripts/lint+ related CLI/unit suites pass locallyLinear Issue: DX-458