FXC-4925: Remove BatchDetail and BatchTask in webapi #3183
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # Use suppress_404 and catch all exceptions to handle unmocked endpoints in tests | ||
| try: | ||
| resp = http.get(f"tidy3d/tasks/{task_id}/detail") | ||
| except WebNotFoundError as e: | ||
| td.log.error(f"The requested task ID '{task_id}' does not exist.") | ||
| raise e | ||
| resp = http.get(f"rf/task/{task_id}/statistics", suppress_404=True) | ||
| if resp: | ||
| task_type = resp.get("taskType") if isinstance(resp, dict) else None | ||
| return SimulationTask(taskId=task_id, taskType=task_type) | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
style: Catching all exceptions with bare except Exception: could hide important errors during debugging
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!
Prompt To Fix With AI
This is a comment left during a code review.
Path: tidy3d/web/core/task_core.py
Line: 479:486
Comment:
**style:** Catching all exceptions with bare `except Exception:` could hide important errors during debugging
<sub>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!</sub>
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Pull request overview
This pull request consolidates the batch task and simulation task APIs by unifying BatchTask and BatchDetail into the existing SimulationTask and TaskInfo classes. The change simplifies the codebase by removing duplicate classes while maintaining backward compatibility through deprecated aliases.
Changes:
- Unified
TaskInfoto represent both simulation and batch tasks with optional fields for each type - Removed
BatchTaskclass and replaced it with a deprecated alias toSimulationTask - Removed
BatchDetailclass, with its fields merged intoTaskInfo - Added
_is_batch_type()method toSimulationTaskfor conditional API routing based on task type (RF, TERMINAL_CM, MODAL_CM) - Updated all API calls to use the unified classes with conditional endpoint routing
- Simplified
TaskFactoryto just delegate toSimulationTask.get()
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 11 comments.
| File | Description |
|---|---|
| tidy3d/web/core/task_info.py | Unified TaskInfo class to handle both simulation and batch tasks; removed BatchDetail class |
| tidy3d/web/core/task_core.py | Removed BatchTask class and added batch support to SimulationTask; simplified TaskFactory; added check() method |
| tidy3d/web/api/webapi.py | Updated to use unified API with _is_batch_type() checks; changed return type signatures |
| tests/test_web/test_webapi.py | Added mock for batch API endpoint to support unified task retrieval |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| except Exception: | ||
| pass |
There was a problem hiding this comment.
Using a bare 'except Exception' clause here is too broad. This will catch all exceptions including system exceptions (KeyboardInterrupt, SystemExit, etc.) that should generally not be caught. Consider catching specific exceptions like WebNotFoundError or HTTPError instead, or at minimum use 'except Exception:' with proper logging.
| # ───────────────────────────────────────────────────────────────────────── | ||
| # Common fields (used by both simulation tasks and batches) | ||
| # ───────────────────────────────────────────────────────────────────────── | ||
| taskId: str = None |
There was a problem hiding this comment.
The field 'taskId' has a default value of None, which means TaskInfo can be instantiated without a task ID. This could be problematic since a task ID should be required to uniquely identify a task. Consider making this field required by removing the default value or using Optional[str] with validation to ensure it's provided when needed.
| taskId: str = None | |
| taskId: Optional[str] |
| message: str = None | ||
| """Status message for the batch.""" | ||
|
|
||
| tasks: list[BatchMember] = None |
There was a problem hiding this comment.
The 'tasks' field defaults to None for simulation tasks but is expected to be a list for batch tasks. The code at line 525 coerces None to an empty list, but this field definition should explicitly handle this. Consider using a default value of None with explicit Optional typing, or use pydantic's Field with a default_factory to provide [] for batch tasks.
| tasks: list[BatchMember] = None | |
| tasks: Optional[list[BatchMember]] = None |
| # Check if task is a batch type (handle mocked objects that may not have the method) | ||
| is_batch = task is not None and getattr(task, "_is_batch_type", lambda: False)() |
There was a problem hiding this comment.
Using getattr with a lambda default is defensive but could hide underlying issues. If 'task' is not None but doesn't have '_is_batch_type', this indicates a type inconsistency that should be addressed at the source. Consider adding a type check or assertion here to ensure 'task' is actually a SimulationTask when not None, rather than silently handling the case where the method doesn't exist.
| # Check if task is a batch type (handle mocked objects that may not have the method) | |
| is_batch = task is not None and getattr(task, "_is_batch_type", lambda: False)() | |
| if task is not None and not isinstance(task, SimulationTask): | |
| raise TypeError( | |
| f"Expected 'SimulationTask' from TaskFactory.get for task_id={task_id!r}, " | |
| f"got {type(task).__name__!r} instead." | |
| ) | |
| is_batch = bool(task is not None and task._is_batch_type()) |
| @@ -792,10 +853,49 @@ def abort(self) -> requests.Response: | |||
| """Abort the current task on the server.""" | |||
| if not self.task_id: | |||
| raise ValueError("Task id not found.") | |||
There was a problem hiding this comment.
The error message refers to "batch ID" but the parameter and method signature use "task_id". For consistency with the unified API design where batch tasks are now handled as SimulationTasks, the error message should refer to "task ID" instead of "batch ID".
| raise ValueError("Task id not found.") | |
| raise ValueError("Task ID not found.") |
| """ | ||
| if not self._is_batch_type(): | ||
| raise NotImplementedError("The 'check' method is only available for batch tasks.") |
There was a problem hiding this comment.
The check() method now raises NotImplementedError for non-batch tasks, but this changes the API contract. This could break existing code that might attempt to call check() on simulation tasks. Consider whether this should log a warning or be a no-op instead of raising an exception, or ensure this is properly documented as a breaking change.
| """ | |
| if not self._is_batch_type(): | |
| raise NotImplementedError("The 'check' method is only available for batch tasks.") | |
| Notes | |
| ----- | |
| For non-batch tasks this method is a no-op for backward compatibility | |
| and will log a warning instead of raising an exception. | |
| """ | |
| if not self._is_batch_type(): | |
| # For non-batch tasks, keep this as a no-op to avoid breaking existing code. | |
| logger = get_logger_console(__name__) | |
| logger.warning( | |
| "WebTask.check() was called on a non-batch task (task_type=%r); " | |
| "this call is ignored. The 'check' method is only available for " | |
| "batch/modeler tasks.", | |
| getattr(self, "task_type", None), | |
| ) | |
| return None # type: ignore[return-value] |
| from tidy3d.web.core.task_core import ( | ||
| BatchDetail, | ||
| BatchTask, | ||
| BatchTask, # noqa: F401 - Deprecated alias, kept for backward compatibility |
There was a problem hiding this comment.
The BatchTask alias is deprecated and imported with noqa: F401 to suppress unused import warnings. However, this import should be explicitly documented in a docstring or comment explaining the deprecation timeline and migration path for users. Consider adding a deprecation warning when BatchTask is used, or at minimum document when this alias will be removed.
| BatchTask, # noqa: F401 - Deprecated alias, kept for backward compatibility | |
| BatchTask, # noqa: F401 - Deprecated alias. | |
| # NOTE: | |
| # `BatchTask` is imported here solely to preserve backward compatibility for | |
| # external code that previously imported `BatchTask` from | |
| # `tidy3d.web.api.webapi`. This alias is deprecated and will be removed in a | |
| # future major release. Users should instead import `BatchTask` directly from | |
| # `tidy3d.web.core.task_core`, or follow the migration guidance in the | |
| # tidy3d web API documentation to use the newer task abstractions. |
| "The 'pay_type' argument is not yet supported for batch tasks." | ||
| ) | ||
| if priority is not None: | ||
| raise NotImplementedError( | ||
| "The 'priority' argument is not yet supported for batch tasks." |
There was a problem hiding this comment.
The NotImplementedError messages for 'pay_type' and 'priority' arguments state they are "not yet supported for batch tasks", implying they may be supported in the future. Consider adding TODO comments with tracking issue numbers so this functionality isn't forgotten, or update the error messages if these features won't be supported.
| "The 'pay_type' argument is not yet supported for batch tasks." | |
| ) | |
| if priority is not None: | |
| raise NotImplementedError( | |
| "The 'priority' argument is not yet supported for batch tasks." | |
| "The 'pay_type' argument is not supported for batch tasks." | |
| ) | |
| if priority is not None: | |
| raise NotImplementedError( | |
| "The 'priority' argument is not supported for batch tasks." |
| def _is_batch_type(self) -> bool: | ||
| """Check if this task uses the batch/modeler API.""" | ||
| return self.task_type in self.BATCH_TASK_TYPES | ||
|
|
There was a problem hiding this comment.
The unified SimulationTask now handles both simulation and batch/modeler tasks with conditional API routing, but there don't appear to be dedicated tests for batch task scenarios (RF, TERMINAL_CM, MODAL_CM task types). Consider adding tests that verify the correct API endpoints are called for batch tasks, including tests for the check() method, submit() with batch tasks, detail() transformation, and abort() routing.
| except Exception: | ||
| pass |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
| except Exception: | |
| pass | |
| except Exception as exc: | |
| td.log.debug( | |
| "Fallback request to 'rf/task/%s/statistics' failed and will be ignored: %s", | |
| task_id, | |
| exc, | |
| ) |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.
| task = BatchTask.get(task_id=task_id) | ||
| task = SimulationTask.get(task_id=task_id) | ||
| detail = _get_batch_detail_handle_error_status(task) | ||
| name = detail.name or "modeler_batch" |
There was a problem hiding this comment.
Calling len() on potentially None tasks field
Medium Severity
The tasks field in TaskInfo defaults to None (changed from [] in the removed BatchDetail class), but _monitor_modeler_batch calls len(detail.tasks) at line 1237 without checking for None first. If the batch API response isn't a dict or the transformation in detail() is bypassed, tasks would be None, causing a TypeError: object of type 'NoneType' has no len().
Additional Locations (1)
Diff CoverageDiff: origin/develop...HEAD, staged and unstaged changes
Summary
tidy3d/web/api/webapi.pyLines 114-123 114 """
115
116 # TODO: test properly
117 try:
! 118 task = SimulationTask.get(resource_id)
! 119 batch_detail = task.detail()
120 status = batch_detail.status.lower()
121 except Exception as e:
122 log.error(f"Could not retrieve batch details for '{resource_id}': {e}")
123 raise WebError(f"Failed to retrieve status for batch '{resource_id}'.") from eLines 718-729 718
719
720 def _get_batch_detail_handle_error_status(task: SimulationTask) -> TaskInfo:
721 """Get batch detail and raise error if status is in ERROR_STATES."""
! 722 detail = task.detail()
723 status = detail.status.lower()
724 if status in ERROR_STATES:
! 725 _batch_detail_error(task.task_id)
726 return detail
727
728
729 def get_status(task_id: TaskId) -> str:Lines 734-742 734 task_id : str
735 Unique identifier of task on server. Returned by :meth:`upload`.
736 """
737 task = TaskFactory.get(task_id)
! 738 if task._is_batch_type():
739 return _get_batch_detail_handle_error_status(task).status
740 else:
741 task_info = get_info(task_id)
742 status = task_info.statusLines 1196-1206 1196 max_detail_tasks: int = 20,
1197 ) -> None:
1198 """Monitor modeler batch progress with aggregate and per-task views."""
1199 console = get_logging_console() if verbose else None
! 1200 task = SimulationTask.get(task_id=task_id)
1201 detail = _get_batch_detail_handle_error_status(task)
! 1202 name = detail.taskName or "modeler_batch"
1203 group_id = detail.groupId
1204 status = detail.status.lower()
1205
1206 # Non-verbose path: poll without progress bars then returnLines 1341-1349 1341 Optional callback function called when downloading file with ``bytes_in_chunk`` as argument.
1342
1343 """
1344 task = TaskFactory.get(task_id, verbose=False)
! 1345 if task._is_batch_type():
1346 raise NotImplementedError("Operation not implemented for modeler batches.")
1347 info = get_info(task_id, verbose=False)
1348 remote_sim_file = SIM_FILE_HDF5_GZ
1349 if info.taskType == "MODE_SOLVER":tidy3d/web/core/task_core.pyLines 158-166 158 """Check if this task uses the batch/modeler API.
159
160 Default implementation returns False. Overridden in SimulationTask.
161 """
! 162 return False
163
164 @classmethod
165 def create(
166 cls,Lines 478-488 478 # Fall back to batch/modeler API
479 # Use suppress_404 and catch all exceptions to handle unmocked endpoints in tests
480 try:
481 resp = http.get(f"rf/task/{task_id}/statistics", suppress_404=True)
! 482 if resp:
! 483 task_type = resp.get("taskType") if isinstance(resp, dict) else None
! 484 return SimulationTask(taskId=task_id, taskType=task_type)
485 except Exception:
486 pass
487
488 td.log.error(f"The requested task ID '{task_id}' does not exist.")Lines 511-530 511 TaskInfo
512 An object containing the task's latest data.
513 """
514 if self._is_batch_type():
! 515 resp = http.get(f"rf/task/{self.task_id}/statistics")
516 # Transform batch response to unified TaskInfo format
! 517 if isinstance(resp, dict):
518 # Map batch field names to unified TaskInfo field names
! 519 if "name" in resp:
! 520 resp["taskName"] = resp.pop("name")
521 # Add taskId from the object itself (not in batch API response)
! 522 resp["taskId"] = self.task_id
523 # Coerce null collection fields to sensible defaults
! 524 if resp.get("tasks") is None:
! 525 resp["tasks"] = []
! 526 return TaskInfo(**(resp or {}))
527 else:
528 resp = http.get(f"tidy3d/tasks/{self.task_id}/detail")
529 return TaskInfo(**{"taskId": self.task_id, "taskType": self.task_type, **resp})Lines 663-679 663 protocol_version = http_util.get_version()
664
665 if self._is_batch_type():
666 # TODO: add support for pay_type and priority arguments for batch tasks
! 667 if pay_type != PayType.AUTO:
! 668 raise NotImplementedError(
669 "The 'pay_type' argument is not yet supported for batch tasks."
670 )
! 671 if priority is not None:
! 672 raise NotImplementedError(
673 "The 'priority' argument is not yet supported for batch tasks."
674 )
! 675 http.post(
676 f"rf/task/{self.task_id}/submit",
677 {
678 "solverVersion": solver_version,
679 "protocolVersion": protocol_version,Lines 853-861 853 """Abort the current task on the server."""
854 if not self.task_id:
855 raise ValueError("Task id not found.")
856 if self._is_batch_type():
! 857 return http.put(f"rf/task/{self.task_id}/abort", {})
858 return http.put(
859 "tidy3d/tasks/abort", json={"taskType": self.task_type, "taskId": self.task_id}
860 )Lines 882-894 882 -------
883 requests.Response
884 The server's response to the check request.
885 """
! 886 if not self._is_batch_type():
! 887 raise NotImplementedError("The 'check' method is only available for batch tasks.")
! 888 if protocol_version is None:
! 889 protocol_version = _get_protocol_version()
! 890 return http.post(
891 f"rf/task/{self.task_id}/check",
892 {
893 "solverVersion": solver_version,
894 "protocolVersion": protocol_version, |
marcorudolphflex
left a comment
marcorudolphflex
left a comment
There was a problem hiding this comment.
As you already pointed out, it may be not be the most elegant solution - but still not bad to burn code. But don't have a strong opinion on that.
| except WebNotFoundError as e: | ||
| td.log.error(f"The requested task ID '{task_id}' does not exist.") | ||
| raise e | ||
| resp = http.get(f"rf/task/{task_id}/statistics", suppress_404=True) |
There was a problem hiding this comment.
why is that in a try block at all when we suppress 404? Do we want to let other exceptions pass?
| task = SimulationTask(**resp) if resp else None | ||
| return task | ||
| td.log.error(f"The requested task ID '{task_id}' does not exist.") | ||
| raise WebNotFoundError("Resource not found (HTTP 404).") |
There was a problem hiding this comment.
404 is not guaranteed here, is it?
| """ | ||
| # Try simulation API first (most common case) | ||
| # Use suppress_404 to avoid error logging when falling back to batch API | ||
| resp = http.get(f"tidy3d/tasks/{task_id}/detail", suppress_404=True) |
There was a problem hiding this comment.
can't we infer the endpoint from the task type?
This is an attempt simplify the webapi components that were introduced in the first component modeler iteration. Specifically, I have removed BatchDetail and BatchTask in favor of everything still being TaskInfo and SimulationTask, respectively. One advantage is that now we just store which task types correspond to a "batch" and the
task._is_batch_type():can be evaluated simply based on the storedtask.taskType, rather than calling the detail API every time (previously, this call was cached when the TaskFactory was used, but it was still awkward and would create unnecessary API call repetitions in some cases).I think I can completely remove BatchTask and TaskFactory (no need to keep backward compatibility of internally used components I would say, especially those that were added very recently for the CM) - but before doing more I wanted to get your opinion @daquintero (maybe also fyi @yaugenst )
I would say this is still not in an ideal state, but looks better to me. Generally, I am not opposed to having the different subclasses, but this was a bit of a mess. For example,
WebTask.createwas always creating aSimulationTaskinstead of aBatchTask, and it was a bit hard to figure out what was needed where and why.I think the main downside of this change is that the TaskInfo now has fields relevant to both regular simulations and to component modelers, so in some cases one part will be None while in other cases another part will be None. But seems OK to me.
Note
Unifies task handling by folding batch/modeler APIs into existing simulation abstractions.
BatchDetailandBatchTask; introducesBatchTask = SimulationTaskalias for backward compatibilitySimulationTaskto detect batch types (_is_batch_type) and route torf/task/...endpoints fordetail,submit,abort, andcheckTaskFactoryto always returnSimulationTask;get()now probes simulation API then falls back to batch API with proper 404 handlingTaskInfointo a unified schema including batch-specific fields;get_infonow returns onlyTaskInfo_is_batch_type()checks instead ofisinstance(BatchTask), adjust monitoring, status, download, load, and cost/real_cost flowsgroupId,version) and remove deprecated pathsWritten by Cursor Bugbot for commit c3b1eb4. This will update automatically on new commits. Configure here.