Fix/make client openhtf method upload files#27
Conversation
WalkthroughThe changes update the Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant C as TofuPilotClient
participant FS as File System
participant S as Server
participant A as Attachment Uploader
U->>C: upload_and_create_from_openhtf_report(file_path)
C->>FS: Validate and read file
FS-->>C: File data or error
C->>S: Create run (include procedure_version)
alt Attachments Exist
loop For each attachment
C->>A: Upload attachment
A-->>C: Upload success
end
end
S-->>C: Return run ID
C-->>U: Run ID
Poem
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
tofupilot/utils/exception.py (1)
1-6: Consider adding type hints and docstring.While the implementation is correct, consider adding:
- Type hints for the instance variables
- A docstring describing the purpose and parameters
Apply this diff to improve type safety and documentation:
class RunCreationError(Exception): + """Exception raised when run creation fails. + + Args: + message: A description of the error. + warnings: Optional list of warning messages. + status_code: Optional HTTP status code or similar. + """ def __init__(self, message: str, warnings: list = None, status_code: int = None): self.message = message - self.warnings = warnings - self.status_code = status_code + self.warnings: list | None = warnings + self.status_code: int | None = status_code super().__init__(message)tofupilot/openhtf/upload.py (1)
94-100: Consider improving error handling.The file deletion in the
finallyblock could fail silently. Consider adding error handling to log any issues during cleanup.Apply this diff to improve error handling:
try: # Call create_run_from_report with the generated file path run_id = self.client.upload_and_create_from_openhtf_report(filename) finally: # Ensure the file is deleted after processing if os.path.exists(filename): - os.remove(filename) + try: + os.remove(filename) + except OSError as e: + self._logger.warning("Failed to delete temporary file %s: %s", filename, e)tofupilot/client.py (4)
92-93: Clarify docstring usage
Consider elaborating on howprocedure_versioninteracts withprocedure_idorprocedure_name. This can help users understand version precedence.
179-179: Consider refactoring or deprecation
create_run_from_openhtf_reportnow delegates toupload_and_create_from_openhtf_report. For consistency, consider deprecating or renaming this method to align with the new naming convention.
198-207: Use logger for exception messages
Currently, these errors are printed to stdout. Consider usingself._loggerfor consistency and better monitoring.Apply this diff:
- print(f"Error: The file '{file_path}' was not found.") - print(f"Error: The file '{file_path}' contains invalid JSON.") - print(f"Error: Insufficient permissions to read '{file_path}'.") - print(f"Unexpected error: {e}") + self._logger.error("File '%s' was not found.", file_path) + self._logger.error("File '%s' contains invalid JSON.", file_path) + self._logger.error("Insufficient permissions to read '%s'.", file_path) + self._logger.error("Unexpected error: %s", e)
412-469: Consolidate error handling and return values
upload_and_create_from_openhtf_reportis a useful abstraction. However, it raisesRunCreationErrorand otherwise returns an empty string on failure. Consider a consistent approach—either always raise or return a descriptive result.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
tofupilot/client.py(8 hunks)tofupilot/openhtf/upload.py(3 hunks)tofupilot/utils/__init__.py(2 hunks)tofupilot/utils/exception.py(1 hunks)tofupilot/utils/network.py(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test (windows-latest)
- GitHub Check: test (ubuntu-latest)
🔇 Additional comments (16)
tofupilot/utils/__init__.py (1)
21-21: LGTM! The new exception class is properly exposed.The
RunCreationErrorclass is correctly imported and added to the module's public interface through__all__.Also applies to: 38-38
tofupilot/openhtf/upload.py (3)
25-25: LGTM! Documentation is up-to-date with implementation changes.The documentation has been correctly updated to reflect:
- The new method name in the docstring
- The full import path in the example
- The simplified example code
Also applies to: 31-31, 39-39
72-72: LGTM! Timestamp formatting is simplified.The timestamp formatting is now more concise without the line break.
96-96: LGTM! Method call updated to match client API changes.The method call has been updated to use the new
upload_and_create_from_openhtf_reportmethod.tofupilot/client.py (8)
10-11: No issues with new imports
These imports (json, base64) are necessary for parsing the OpenHTF report and decoding attachments.
32-32: Good usage ofnotify_server
Includingnotify_serverfrom.utilsensures the server is properly updated post-upload.
71-71: Validate backward compatibility
Addingprocedure_versionis beneficial for versioning. Please ensure existing calls tocreate_runwithout this parameter remain functional.
126-126: Ensure server support
Adding"procedure_version": procedure_versionto the payload appears correct. Verify that the backend can handle this field.
194-195: Seamless integration with new method
Centralizing the upload logic by callingupload_and_create_from_openhtf_reporthelps reduce duplication.
209-221: Validate existence of 'phases' and 'attachments'
Iftest_recordorphase.get("attachments")isNone, iterating over them may cause errors. Consider handling these cases gracefully.
224-239: Initialization logic looks solid
The upload initialization request andresponse.raise_for_status()usage correctly handle potential server errors at this stage.
254-257: Check.successlogger method
self._logger.success(...)suggests a custom log level. Make sure it’s genuinely supported insetup_loggerto avoid runtime errors.tofupilot/utils/network.py (4)
5-5: Approved import
ImportingRunCreationErroris necessary for the new exception-based error approach.
43-43: Switched from returning error data to raising
This function no longer returns a dict. Verify all callers are updated to handle the thrownRunCreationError.
65-65: Consistent use ofRunCreationError
RaisingRunCreationErrorimproves clarity by grouping status code, warnings, and message together.
68-72: Raised exception for network issues
Switching from returning error data to throwingRunCreationErrorensures consistent exception-based handling. Verify callers catch or handle these exceptions.
| data = base64.b64decode(attachment["data"]) | ||
|
|
||
| requests.put( | ||
| upload_url, | ||
| data=data, | ||
| headers={ | ||
| "Content-Type": attachment["mimetype"] | ||
| or "application/octet-stream", # Default to binary if mimetype is missing | ||
| }, | ||
| timeout=SECONDS_BEFORE_TIMEOUT, | ||
| ) | ||
|
|
||
| notify_server(self._headers, self._url, upload_id, run_id) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle potential PUT request errors
The PUT request’s response isn’t captured. If it fails, the error might be missed. Please verify success by capturing and checking the response:
data = base64.b64decode(attachment["data"])
-requests.put(
+response = requests.put(
upload_url,
data=data,
...
)
+response.raise_for_status()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| data = base64.b64decode(attachment["data"]) | |
| requests.put( | |
| upload_url, | |
| data=data, | |
| headers={ | |
| "Content-Type": attachment["mimetype"] | |
| or "application/octet-stream", # Default to binary if mimetype is missing | |
| }, | |
| timeout=SECONDS_BEFORE_TIMEOUT, | |
| ) | |
| notify_server(self._headers, self._url, upload_id, run_id) | |
| data = base64.b64decode(attachment["data"]) | |
| response = requests.put( | |
| upload_url, | |
| data=data, | |
| headers={ | |
| "Content-Type": attachment["mimetype"] | |
| or "application/octet-stream", # Default to binary if mimetype is missing | |
| }, | |
| timeout=SECONDS_BEFORE_TIMEOUT, | |
| ) | |
| response.raise_for_status() | |
| notify_server(self._headers, self._url, upload_id, run_id) |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
tofupilot/client.py (1)
242-250:⚠️ Potential issueHandle potential PUT request errors.
The PUT request's response isn't captured. If it fails, the error might be missed. Please verify success by capturing and checking the response.
-requests.put( +response = requests.put( upload_url, data=data, headers={ "Content-Type": attachment["mimetype"] or "application/octet-stream", # Default to binary if mimetype is missing }, timeout=SECONDS_BEFORE_TIMEOUT, ) +response.raise_for_status()
🧹 Nitpick comments (1)
tofupilot/client.py (1)
209-258: Consider extracting attachment handling logic.The attachment handling logic is quite complex and could be moved to a separate method for better maintainability and reusability.
+def _handle_openhtf_attachments(self, test_record: dict, run_id: str) -> None: + """Handle attachments from OpenHTF test record.""" + number_of_attachments = 0 + for phase in test_record.get("phases"): + if number_of_attachments >= self._max_attachments: + self._logger.warning( + "Too many attachments, trimming to %d attachments.", + self._max_attachments, + ) + break + for attachment_name, attachment in phase.get("attachments").items(): + number_of_attachments += 1 + self._upload_attachment(attachment_name, attachment, run_id) + +def _upload_attachment(self, name: str, attachment: dict, run_id: str) -> None: + """Upload a single attachment and link it to the run.""" + self._logger.info("Uploading %s...", name) + + # Upload initialization + initialize_url = f"{self._url}/uploads/initialize" + payload = {"name": name} + + response = requests.post( + initialize_url, + data=json.dumps(payload), + headers=self._headers, + timeout=SECONDS_BEFORE_TIMEOUT, + ) + + response.raise_for_status() + response_json = response.json() + upload_url = response_json.get("uploadUrl") + upload_id = response_json.get("id") + + data = base64.b64decode(attachment["data"]) + + response = requests.put( + upload_url, + data=data, + headers={ + "Content-Type": attachment["mimetype"] + or "application/octet-stream", + }, + timeout=SECONDS_BEFORE_TIMEOUT, + ) + response.raise_for_status() + + notify_server(self._headers, self._url, upload_id, run_id) + + self._logger.success( + "Attachment %s successfully uploaded and linked to run.", + name, + ) def create_run_from_openhtf_report(self, file_path: str): # Upload report and create run from file_path run_id = self.upload_and_create_from_openhtf_report(file_path) try: with open(file_path, "r", encoding="utf-8") as file: test_record = json.load(file) except FileNotFoundError: print(f"Error: The file '{file_path}' was not found.") except json.JSONDecodeError: print(f"Error: The file '{file_path}' contains invalid JSON.") except PermissionError: print(f"Error: Insufficient permissions to read '{file_path}'.") except Exception as e: print(f"Unexpected error: {e}") if run_id and test_record: - number_of_attachments = 0 - for phase in test_record.get("phases"): - # Keep only max number of attachments - if number_of_attachments >= self._max_attachments: - self._logger.warning( - "Too many attachments, trimming to %d attachments.", - self._max_attachments, - ) - break - for attachment_name, attachment in phase.get("attachments").items(): - number_of_attachments += 1 - - self._logger.info("Uploading %s...", attachment_name) - - # Upload initialization - initialize_url = f"{self._url}/uploads/initialize" - payload = {"name": attachment_name} - - response = requests.post( - initialize_url, - data=json.dumps(payload), - headers=self._headers, - timeout=SECONDS_BEFORE_TIMEOUT, - ) - - response.raise_for_status() - response_json = response.json() - upload_url = response_json.get("uploadUrl") - upload_id = response_json.get("id") - - data = base64.b64decode(attachment["data"]) - - requests.put( - upload_url, - data=data, - headers={ - "Content-Type": attachment["mimetype"] - or "application/octet-stream", - }, - timeout=SECONDS_BEFORE_TIMEOUT, - ) - - notify_server(self._headers, self._url, upload_id, run_id) - - self._logger.success( - "Attachment %s successfully uploaded and linked to run.", - attachment_name, - ) + self._handle_openhtf_attachments(test_record, run_id)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tofupilot/client.py(8 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: test (windows-latest)
- GitHub Check: test (ubuntu-latest)
🔇 Additional comments (2)
tofupilot/client.py (2)
71-71: LGTM! Well-documented parameter addition.The
procedure_versionparameter is properly type-hinted, documented, and integrated into the payload.Also applies to: 92-93, 126-126
412-467: LGTM! Well-structured method with proper error handling.The method effectively handles file validation, upload, and run creation with appropriate error handling and clear documentation.
1 participant
Summary by CodeRabbit
New Features
Refactor