refactor

Author: sbassam

src/together/filemanager.py

@@ -9,7 +9,7 @@
 from concurrent.futures import ThreadPoolExecutor, as_completed
 from functools import partial
 from pathlib import Path
-from typing import Any, Dict, List, Tuple, cast
+from typing import Any, Dict, List, Tuple
 
 import requests
 from filelock import FileLock
@@ -34,6 +34,7 @@
     AuthenticationError,
     DownloadError,
     FileTypeError,
+    ResponseError,
 )
 from together.together_response import TogetherResponse
 from together.types import (
@@ -352,7 +353,7 @@ def upload(
                 )
             redirect_url, file_id = self.get_upload_url(url, file, purpose, filetype)
 
-        file_size = os.stat(file.as_posix()).st_size
+        file_size = os.stat(file).st_size
 
         with tqdm(
             total=file_size,
@@ -415,9 +416,8 @@ def upload(
     ) -> FileResponse:
         """Upload large file using multipart upload"""
 
-        file_size = os.stat(file.as_posix()).st_size
+        file_size = os.stat(file).st_size
 
-        # Validate file size limits
         file_size_gb = file_size / NUM_BYTES_IN_GB
         if file_size_gb > MAX_FILE_SIZE_GB:
             raise FileTypeError(
@@ -427,41 +427,42 @@ def upload(
         part_size, num_parts = self._calculate_parts(file_size)
 
         file_type = self._get_file_type(file)
+        upload_info = None
 
         try:
-            # Phase 1: Initiate multipart upload
             upload_info = self._initiate_upload(
                 url, file, file_size, num_parts, purpose, file_type
             )
 
-            # Phase 2: Upload parts concurrently
             completed_parts = self._upload_parts_concurrent(
                 file, upload_info, part_size
             )
 
-            # Phase 3: Complete upload
             return self._complete_upload(
                 url, upload_info["upload_id"], upload_info["file_id"], completed_parts
             )
 
         except Exception as e:
             # Cleanup on failure
-            if "upload_info" in locals():
+            if upload_info is not None:
                 self._abort_upload(
                     url, upload_info["upload_id"], upload_info["file_id"]
                 )
             raise e
 
     def _get_file_type(self, file: Path) -> str:
-        """Get file type from extension, defaulting to jsonl as discussed in feedback"""
+        """Get file type from extension, raising ValueError for unsupported extensions"""
         if file.suffix == ".jsonl":
             return "jsonl"
         elif file.suffix == ".parquet":
             return "parquet"
         elif file.suffix == ".csv":
             return "csv"
         else:
-            return "jsonl"
+            raise ValueError(
+                f"Unsupported file extension: '{file.suffix}'. "
+                f"Supported extensions: .jsonl, .parquet, .csv"
+            )
 
     def _calculate_parts(self, file_size: int) -> tuple[int, int]:
         """Calculate optimal part size and count"""
@@ -474,7 +475,6 @@ def _calculate_parts(self, file_size: int) -> tuple[int, int]:
         num_parts = min(MAX_MULTIPART_PARTS, math.ceil(file_size / target_part_size))
         part_size = math.ceil(file_size / num_parts)
 
-        # Ensure minimum part size
         if part_size < min_part_size:
             part_size = min_part_size
             num_parts = math.ceil(file_size / part_size)
@@ -489,7 +489,7 @@ def _initiate_upload(
         num_parts: int,
         purpose: FilePurpose,
         file_type: str,
-    ) -> Dict[str, Any]:
+    ) -> Any:
         """Initiate multipart upload with backend"""
 
         requestor = api_requestor.APIRequestor(client=self._client)
@@ -510,7 +510,7 @@ def _initiate_upload(
             ),
         )
 
-        return cast(Dict[str, Any], response.data)
+        return response.data
 
     def _upload_parts_concurrent(
         self, file: Path, upload_info: Dict[str, Any], part_size: int
@@ -526,11 +526,9 @@ def _upload_parts_concurrent(
 
                 with open(file, "rb") as f:
                     for part_info in parts:
-                        # Read part data
                         f.seek((part_info["part_number"] - 1) * part_size)
                         part_data = f.read(part_size)
 
-                        # Submit upload task
                         future = executor.submit(
                             self._upload_single_part, part_info, part_data
                         )
@@ -564,7 +562,7 @@ def _upload_single_part(self, part_info: Dict[str, Any], part_data: bytes) -> st
 
         etag = response.headers.get("ETag", "").strip('"')
         if not etag:
-            raise Exception(f"No ETag returned for part {part_info['part_number']}")
+            raise ResponseError(f"No ETag returned for part {part_info['part_number']}")
 
         return etag
 

src/together/resources/files.py

@@ -48,8 +48,7 @@ def upload(
 
         assert isinstance(purpose, FilePurpose)
 
-        # Size-based routing: use multipart for files > 5GB
-        file_size = os.stat(file.as_posix()).st_size
+        file_size = os.stat(file).st_size
         file_size_gb = file_size / NUM_BYTES_IN_GB
 
         if file_size_gb > MULTIPART_THRESHOLD_GB:

src/together/utils/files.py

@@ -65,7 +65,7 @@ def check_file(
     else:
         report_dict["found"] = True
 
-    file_size = os.stat(file.as_posix()).st_size
+    file_size = os.stat(file).st_size
 
     if file_size > MAX_FILE_SIZE_GB * NUM_BYTES_IN_GB:
         report_dict["message"] = (

tests/unit/test_multipart_upload_manager.py

@@ -14,7 +14,7 @@
 from together.types import FilePurpose, FileResponse
 from together.types.common import ObjectType
 from together.together_response import TogetherResponse
-from together.error import FileTypeError
+from together.error import FileTypeError, ResponseError
 
 
 class TestMultipartUploadManager:
@@ -51,10 +51,15 @@ def test_get_file_type_csv(self, manager):
         file = Path("test.csv")
         assert manager._get_file_type(file) == "csv"
 
-    def test_get_file_type_unknown_defaults_to_jsonl(self, manager):
-        """Test that unknown file types default to jsonl"""
+    def test_get_file_type_unknown_raises_error(self, manager):
+        """Test that unknown file types raise ValueError"""
         file = Path("test.txt")
-        assert manager._get_file_type(file) == "jsonl"
+        with pytest.raises(ValueError) as exc_info:
+            manager._get_file_type(file)
+
+        error_message = str(exc_info.value)
+        assert "Unsupported file extension: '.txt'" in error_message
+        assert "Supported extensions: .jsonl, .parquet, .csv" in error_message
 
     def test_calculate_parts_small_file(self, manager):
         """Test part calculation for files smaller than target part size"""
@@ -186,7 +191,7 @@ def test_upload_single_part_no_etag_error(self, mock_put, manager):
         part_data = b"test data"
 
         # Test
-        with pytest.raises(Exception, match="No ETag returned for part 1"):
+        with pytest.raises(ResponseError, match="No ETag returned for part 1"):
             manager._upload_single_part(part_info, part_data)
 
     @patch("together.filemanager.api_requestor.APIRequestor")